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

Refactor upgrade role to not include "specification" at top level #2485

Closed
6 of 10 tasks
to-bar opened this issue Aug 4, 2021 · 1 comment
Closed
6 of 10 tasks

Refactor upgrade role to not include "specification" at top level #2485

to-bar opened this issue Aug 4, 2021 · 1 comment

Comments

@to-bar
Copy link
Contributor

to-bar commented Aug 4, 2021

Describe the bug
In upgrade role there are include_vars tasks used without the name parameter what loads variables at top level with higher precedence than task vars.

So having in upgrade role:

- name: Node Exporter | Include specification vars from node_exporter role
  include_vars:
    file: roles/node_exporter/vars/main.yml

causes that specification of node_exporter is global with high precedence.

Then the following task (postgresql upgrade is run after node_exporter) has unexpected content (from node_exporter) in specification var:

- name: repmgr for PG {{ pg_version }} | Replace repmgr config file
  template:
    src: repmgr.conf.j2
    dest: "{{ upgrade_defaults.repmgr.config_dir[ansible_os_family] }}/repmgr.conf"
    owner: postgres
    group: postgres
    mode: u=rw,g=,o=
  vars:
    specification:
      extensions:
        replication:
          replication_user_name: "{{ postgresql_manifest.specification.extensions.replication.replication_user_name }}"
          repmgr_database:       "{{ postgresql_manifest.specification.extensions.replication.repmgr_database }}"

Expected behavior
Task include_vars should be used with name parameter, for example:

- name: Node Exporter | Include specification vars from node_exporter role
  include_vars:
    file: roles/node_exporter/vars/main.yml
    name: node_exporter_vars

then specification would not override task vars but be available under node_exporter_vars.specification.

Additional context
Having globally loaded vars with high precedence may cause difficult to debug errors.

For local vars we can use underscore in names, e.g. _specification.


DoD checklist

  • Changelog updated (if affected version was released)
  • COMPONENTS.md updated / doesn't need to be updated
  • Automated tests passed (QA pipelines)
    • apply
    • upgrade
  • Case covered by automated test (if possible)
  • Idempotency tested
  • Documentation updated / doesn't need to be updated
  • All conversations in PR resolved
  • Backport tasks created / doesn't need to be backported
@przemyslavic
Copy link
Collaborator

✔️ Tested upgrade with affected components enabled (kafka, node exporter, logging, rabbitmq, repmgr for postgresql).

@plirglo plirglo closed this as completed Aug 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants