-
Notifications
You must be signed in to change notification settings - Fork 446
Add advertise address and port config #581
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add advertise address and port config #581
Conversation
Any info on what i can do to bring this forward? |
@sgotti I will try. I might need some help/hints though. Will come back to you if that is the case. |
The advertise address:port will be publish in the keeper information to be used by client, sentinel, proxy, ctl or other keeper. The listening address/port will be used only for postgres instance configuration. Fix : - localhost not need for listenAddress postgres instance - check init flag : listenaddress must be explicit, advertise listen address will use listenaddres if not specified - check init flag : use specified flag for warn/error message - check init flag : don't touch init pgport and just set advertise pg port if not specified
fc8fea3
to
bbbade8
Compare
@sgotti I had a little bit time left today so i finally tried to implement the integration test. I updated my pull request accordingly. I am not sure how you expect the failover tests to work (this is why i did not touch them yet). Since you could only test them by setting up some local nat (or listen on * and advertise localhost to see the difference?)? I have not seen any test that check wether the config values given are correct. I implement now a test that checks that postgres is listening on the right ports but the advertise address and port are set in clusterdata for the proxies to listen to. Please tell if this is how you imagined the integration test. Regards, David |
bbbade8
to
c0f8964
Compare
Any feedback? |
@daMupfel That's a good start for an integration test. Thanks. I'm not sure it's covering all the cases but more tests should be added in the future. I'll start merging this as if you don't have more work to do. |
I hope there was not a language barrier here :). As far as i understood i don't have to do anything for this to get merged, correct? |
Any updates? |
@daMupfel sorry for the delay. Merging. |
@sgotti No problems, thank you very much 👍 😄 |
This pull request is here to allow separate configuration for listen address and advertise address.
This is the same pull request as here (but fixed for master branch):
#455 .