search

allegro / marathon-consul

Integrates Marathon apps with Consul service discovery.
Apache License 2.0
191 stars 33 forks source link

support leader discovery logic so can support port other than 8080 #264

Closed tokenrain closed 7 years ago

tokenrain commented 7 years ago

I have no go experience but I thought I would give this a shot. Basically we run Marathon in port 8800 and can not use the marathon-consul feature of having it sync only on the master because marathon-consul can only support finding masters if they run on port 8080 due to the hardcoding of that in the resolveHostname function. This may not be the exact code that you would use but I hope at least I have come close. Thanks!

adamdubiel commented 7 years ago

Getting rid of hardcoded configuration options is always a good thing. However could you fix test code as well? Try make build on localhost and follow the output.

janisz commented 7 years ago

@tokenrain Good catch! Thanks! I have no idea how it could pass our review. I left some comments on the code. We definitely want to merge it.

janisz commented 7 years ago

@tokenrain Would you like to work on this? I can fix issues for you if have no time.

tokenrain commented 7 years ago

Sorry for the very late response. I was away and not focusing on technology stuff. I would be happy for you to take care of this as I am quite busy. Let me know if you can else I will try and get some time this week to take a stab. Many Thanks!

janisz commented 7 years ago

No problem. I will take care of it. Thanks for update and once again thanks for catching this bug.

tokenrain commented 7 years ago

No thank you for writing and maintaining such a valuable piece for software!!!!!

janisz commented 7 years ago

I created separate PR for this #274