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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

fix(postgres.manage): only reload modules if needed; fixes #214 #265

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions postgres/defaults.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,8 @@ postgres:

bake_image: False

manage_force_reload_modules: False
Copy link
Contributor

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.


fromrepo: ''

users: {}
Expand Down
2 changes: 1 addition & 1 deletion postgres/macros.jinja
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@
{{ state }}-{{ name }}:
{{ state }}.{{ ensure|default('present') }}:
{{- format_kwargs(kwarg) }}
- onchanges:
Copy link
Contributor

@vutny vutny May 10, 2019 •
edited
Loading

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.

- require:
- test: postgres-reload-modules

{%- endmacro %}
Expand Down
25 changes: 15 additions & 10 deletions postgres/manage.sls
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
{%- from tpldir + "/map.jinja" import postgres with context -%}
{%- from tpldir + "/macros.jinja" import format_state with context -%}

{%- if salt['postgres.user_create']|default(none) is not callable %}
{%- set needs_client_binaries = salt['postgres.user_create']|default(none) is not callable -%}

{%- if needs_client_binaries %}

# Salt states for managing PostgreSQL is not available,
# need to provision client binaries first
Expand All @@ -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:
Copy link
Contributor

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.

{%- if needs_client_binaries or postgres.manage_force_reload_modules %} True
{%- else %} False
{%- endif %}
- result: True
- reload_modules: True

# User states
Expand All @@ -35,7 +42,7 @@ postgres-reload-modules:

{{ format_state(name, 'postgres_tablespace', tblspace) }}
{%- if 'owner' in tblspace %}
- require:
{#- - require: #}
Copy link
Contributor

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.

- postgres_user: postgres_user-{{ tblspace.owner }}
{%- endif %}

Expand All @@ -49,7 +56,7 @@ postgres-reload-modules:
{%- do extension.update({'name': ext_name, 'maintenance_db': name}) %}

{{ format_state( name + '-' + ext_name, 'postgres_extension', extension) }}
- require:
{#- - require: #}
- postgres_database: postgres_database-{{ name }}
{%- if 'schema' in extension and 'schemas' in postgres %}
- postgres_schema: postgres_schema-{{ name }}-{{ extension.schema }}
Expand All @@ -62,15 +69,15 @@ postgres-reload-modules:
{%- do schema.update({'name': schema_name, 'dbname': name }) %}

{{ format_state( name + '-' + schema_name, 'postgres_schema', schema) }}
- require:
{#- - require: #}
- postgres_database: postgres_database-{{ name }}

{%- endfor %}
{%- endif %}

{{ format_state(name, 'postgres_database', db) }}
{%- if 'owner' in db or 'tablespace' in db %}
- require:
{#- - require: #}
{%- endif %}
{%- if 'owner' in db %}
- postgres_user: postgres_user-{{ db.owner }}
Expand All @@ -86,7 +93,7 @@ postgres-reload-modules:
{%- for name, schema in postgres.schemas|dictsort() %}

{{ format_state(name, 'postgres_schema', schema) }}
- require:
{#- - require: #}
- postgres_database-{{ schema.dbname }}
{%- if 'owner' in schema %}
- postgres_user: postgres_user-{{ schema.owner }}
Expand All @@ -99,9 +106,7 @@ postgres-reload-modules:
{%- for name, extension in postgres.extensions|dictsort() %}

{{ format_state(name, 'postgres_extension', extension) }}
{%- if 'maintenance_db' in extension or 'schema' in extension %}
- require:
{%- endif %}
{#- - require: #}
{%- if 'maintenance_db' in extension %}
- postgres_database: postgres_database-{{ extension.maintenance_db }}
{%- endif %}
Expand Down