-
Notifications
You must be signed in to change notification settings - Fork 283
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
fix(postgres.manage): only reload modules if needed; fixes #214 #265
base: master
Are you sure you want to change the base?
Conversation
…formulas#214 Based on the great work of @RobRuana in saltstack-formulas#216.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@alxwr Thanks for picking up the issue.
I would like to explain the functionality which caused so many confusions. Below I will suggest a better way to handle this.
First of all, the postgres-reload-modules
state changes is only required for one and only one thing:
set a flag that PG client binaries are now available. Because if they're not, all subsequent states would fail.
The main purpose is to be able to test Pillar values using state.apply mock=True
call during initial state run. Since Salt will not do any job, there would be no binaries, and we'd get failures. No way to distinguish the failures caused by bad supplied data and missing requirements. So, to be able to verify that formula is reading your pillar data right, we need to have all states return True
. And the only way to achieve that is trigger needed states only after some "fake" changes.
Depending on just reloading Salt modules doesn't make any difference, since Salt doesn't return any result back on that. It is always success. In my opinion, the proper way to fix it is to subscribe on changes only when the binaries aren't available on rendering time.
@@ -71,6 +71,8 @@ postgres: | |||
|
|||
bake_image: False | |||
|
|||
manage_force_reload_modules: False |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to have this configurable. We will make the default behavior correct, so there would be no undesired state changes.
@@ -25,7 +25,7 @@ | |||
{{ state }}-{{ name }}: | |||
{{ state }}.{{ ensure|default('present') }}: | |||
{{- format_kwargs(kwarg) }} | |||
- onchanges: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of making the hard requirement, I'd suggest to put a conditional like this:
{%- if needs_client_binaries %}
- onchanges:
- test: postgres-reload-modules
{%- endif %}
This would make changes only when the binaries were not available before, which is valid behavior.
Otherwise, there would not be any dependency on the test state, since it would actually do nothing.
@@ -18,7 +20,12 @@ include: | |||
# Ensure that Salt is able to use postgres modules | |||
|
|||
postgres-reload-modules: | |||
test.succeed_with_changes: | |||
test.configurable_test_state: | |||
- changes: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's just make this very simple:
- changes: {{ needs_client_binaries }}
Should work as expected.
@@ -35,7 +42,7 @@ postgres-reload-modules: | |||
|
|||
{{ format_state(name, 'postgres_tablespace', tblspace) }} | |||
{%- if 'owner' in tblspace %} | |||
- require: | |||
{#- - require: #} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then, all requisite should be returned back.
Based on the great work of @RobRuana in #216.
Tested on FreeBSD 11.2 with Salt 2019.2.0.
postgres-reload-modules
only report changes (and reloads the modules) if necessarytest.configurable_test_state
require
is always present in theformat_state
macro, so we're just using it. (This part is not as clean as I want it to be, but I regard it as a trade-off. Maybe there are better ideas.)