Skip to content

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

Merged
merged 2 commits into from
Mar 13, 2019

Conversation

daMupfel
Copy link
Contributor

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 .

@daMupfel
Copy link
Contributor Author

daMupfel commented Nov 10, 2018

Any info on what i can do to bring this forward?

@sgotti
Copy link
Member

sgotti commented Nov 13, 2018

@daMupfel Are you willing to adopt and develop #455? If you look at its review comments you'll see that one of the main missing things are integration tests. Without them I won't merge it.

@daMupfel
Copy link
Contributor Author

@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
@daMupfel daMupfel force-pushed the add-advertise-address-and-port branch from fc8fea3 to bbbade8 Compare November 20, 2018 11:30
@daMupfel
Copy link
Contributor Author

daMupfel commented Nov 20, 2018

@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

@daMupfel daMupfel force-pushed the add-advertise-address-and-port branch from bbbade8 to c0f8964 Compare November 20, 2018 11:34
@daMupfel
Copy link
Contributor Author

Any feedback?

@sgotti
Copy link
Member

sgotti commented Dec 20, 2018

@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.

@daMupfel
Copy link
Contributor Author

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?

@daMupfel
Copy link
Contributor Author

Any updates?

@sgotti
Copy link
Member

sgotti commented Mar 13, 2019

@daMupfel sorry for the delay. Merging.

@sgotti sgotti merged commit e3e24f4 into sorintlab:master Mar 13, 2019
@daMupfel
Copy link
Contributor Author

@sgotti No problems, thank you very much 👍 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants