Skip to content
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

repmgr extension fixes #2

Open
wants to merge 8 commits into
base: add-repmgr-extension
Choose a base branch
from

Conversation

AnthonyCaetano
Copy link

Hi

Please find the following 3 issues addressed (I can't log issues on your repo), but they are:

The use of the existing repmgr_config_directory variable as the location for repmgr.conf rather than using postgresql_conf_directory.

Allow repmgr standby clone to work even if postgresql is running on a non-default port

Don't assume that the first host in the play (i.e. ansible_play_host[0] is the master on first config) -- rather use existing variable repmgr_master. The host names from the inventory are not semantically sort for "first" and "second" by role in some organizations, and the first host in the play might need to be the standby.

Protect the pgpass updates with no_log: True.

Regards
Anthony Caetano

@rbjorklin
Copy link
Collaborator

Hi @AnthonyCaetano! Thanks a lot for your contribution! Everything looks good to me but could you explain what purpose the regex serves in pgpass before I merge this?

@AnthonyCaetano
Copy link
Author

AnthonyCaetano commented Feb 5, 2020

Hi @rbjorklin
Pleasure, and thanks for all your work on this!
I think I have screwed up my pull request, that patch wasn't intended to be part of it.

I added the regexes so that the .pgpass contains both the FQDN and the shortname of the host so that auth works in both cases -- in our deployment the inventory contains FQDNs, but some of the names configured are the short names and they get auth denied.

I only meant for these commits be included in the PR:
Use the repmgr_config_directory for the location of the repmgr.conf n… 9562b17
Use postgresql_port in the 'repmgr standby clone' for postgres instan… fd8fcf9
Don't assume the first host in the play is the master, use the config… fa0529a
Don't log .pgpass updates as they contain passwords 0b73202

We do require these other 4 commits for our environment, but I didn't mean for them to be included as part of the PR:
Use postgresql_conf_directory for the location of the postgresqq.conf… b1a4dba
add server short name along with fqdn to the .pgpass auth file 2f48419
move defaults files under ./defaults/main/ so that AWX/ansible finds … a0e473d
add debug print of repmgr_master 4180400

My bad...
I see I should have created another branch just for the other commits.
Shall I redo it?
Are you happy with all of them?
I expect that you wouldn't want to commit a0e473d "move defaults files under ./defaults/main/ so that AWX/ansible finds" since you are pushing to get your work included upstream and therefore into the main config file, right?

@rbjorklin
Copy link
Collaborator

Hi @AnthonyCaetano
If you could clean this up as you outlined above I'd be happy to merge it. You're correct, my intention with this is to get it merged back into the official postgres role.

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