Use copy state instead of salt-call #256
Conversation
There was a problem hiding this comment.
Hi @thomasrossetto !
That was me who introduced the local file backup technique to this state. This a tricky problem, though.
The require state is not suitable here, because on the initial state run when the config file is not present (which is the case on some distros and upstream PG combination), nothing to backup.
Also, if the backup extension is not static, the new backup file would be made on each state state run.
You probably want to backup it only when changes are going to be made.
I suggest to look at prereq requisite, because I also consider that raw salt-call command doesn't look nice at all.
When I initially implemented it, I stumbled into some issues with older Salt releases. Probably for now it may not be the case anymore. See issue #148 and PR #149 .
Thanks!
|
Hi @vutny ! |
|
No worries @thomasrossetto ! Just update the PR with your new changes and I think we'd make it through. |
There was a problem hiding this comment.
Thanks for the update, @thomasrossetto !
I would like you to do some small clean up, and this would be good to go.
| @@ -164,13 +164,12 @@ postgresql-pg_hba: | |||
| # Create the empty file before managing to overcome the limitation of check_cmd | |||
| - onlyif: test -f {{ pg_hba_path }} || touch {{ pg_hba_path }} | |||
There was a problem hiding this comment.
This is no longer needed since we are going to remove check_cmd parameter. Same applies for the pg_ident.
| {%- endif %} | ||
| {%- else %} | ||
| - replace: False | ||
| {%- endif %} | ||
| - prereq: |
There was a problem hiding this comment.
I guess we must wrap this with {%- if postgres.config_backup %} ... {%- endif %} statements, instead of onlyif and check_cmd, because no need to trigger backup if it was disabled.
| @@ -214,6 +211,18 @@ postgresql-pg_ident: | |||
| - service: postgresql-running | |||
| {%- endif %} | |||
|
|
|||
| backup_pg_ident: | |||
There was a problem hiding this comment.
Like above, we need the if statement here. These states also should be applied only when postgres.config_backup variable would contain any meaningful value.
|
@thomasrossetto Would you be able to finish this PR and address the comments? |
|
Good work @thomasrossetto! I've been working on getting the Travis-based Kitchen tests established for this formula but the https://travis-ci.org/myii/postgres-formula/jobs/524280903#L1427: ID: postgresql-pg_hba
Function: file.managed
Name: /var/lib/pgsql/data/pg_hba.conf
Result: False
Comment: check_cmd execution failedApplied these commits and it's now working. https://travis-ci.org/myii/postgres-formula/jobs/524291445#L1439: ID: postgresql-pg_hba
Function: file.managed
Name: /var/lib/pgsql/data/pg_hba.conf
Result: True
Comment: File /var/lib/pgsql/data/pg_hba.conf updatedWhile this might not be what your intention was when you submitted this PR, it's a nice bonus. |
|
@vutny After setting up the CI PR (#262), the https://travis-ci.com/saltstack-formulas/postgres-formula/jobs/195597494#L1552: ID: postgresql-pg_hba
Function: file.managed
Name: /var/lib/pgsql/data/pg_hba.conf
Result: True
Comment: File /var/lib/pgsql/data/pg_hba.conf updatedSo no need to merge this PR prematurely, it seems. |
|
Great, thank you @myii ! |
|
Thanks, @thomasrossetto @myii @vutny |
Now it works also with salt-ssh.