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

Be much more explicit about variable precedence as it applies to roles #73939

Closed
wants to merge 7 commits into from

Conversation

sivel
Copy link
Member

@sivel sivel commented Mar 17, 2021

SUMMARY

Be much more explicit about variable precedence as it applies to roles

Also fixes #43543

ISSUE TYPE
  • Docs Pull Request
COMPONENT NAME
docs/docsite/rst/user_guide/playbooks_variables.rst
ADDITIONAL INFORMATION

@ansibot ansibot added affects_2.11 core_review In order to be merged, this PR must follow the core review workflow. docs This issue/PR relates to or includes documentation. needs_triage Needs a first human triage before being processed. support:core This issue/PR relates to code supported by the Ansible Engineering Team. labels Mar 17, 2021
@mattclay mattclay removed the needs_triage Needs a first human triage before being processed. label Mar 18, 2021
Copy link
Contributor

@acozine acozine left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Precedence is, indeed, complicated and difficult to describe clearly. I've added a few questions and suggestions.

Co-authored-by: Alicia Cozine <879121+acozine@users.noreply.github.com>
@ansibot ansibot added the docs_only All changes are to files within the docs/docsite/ directory label Mar 23, 2021
@Shrews
Copy link
Contributor

Shrews commented Mar 31, 2021

Related PR: #72808

@ansibot ansibot added the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Apr 8, 2021
@ansibot ansibot added needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed core_review In order to be merged, this PR must follow the core review workflow. stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. labels Apr 8, 2021
Co-authored-by: Alicia Cozine <879121+acozine@users.noreply.github.com>
@ansibot ansibot added core_review In order to be merged, this PR must follow the core review workflow. and removed needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Apr 8, 2021
#. play ``vars``
#. play ``vars_prompt``
#. play ``vars_files``
#. role vars from all roles (defined in ``roles/*/vars/main.yml`` or ``vars:``)
Copy link
Contributor

@SimonHeimberg SimonHeimberg Apr 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should vars have a colon here, but not for similar cases in lines above (play vars, play vars_prompt, play vars_files)?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the colon is useful to visually cue the reader that it should be defined in an ansible/playbook/task/whatever file rather than in a standalone vars file.

However, as a reader of the docs, I would not know where this vars: would be defined. Where would you put vars: inside a role? On a task? Does vars: on a task count as a roles var here? Does this mean if I have a list of roles defined in a playbook, and set a variable for one role, the other roles get that variable too?

Comment on lines +315 to +318
#. inventory file relative ``group_vars/all`` [3]_
#. playbook file relative ``group_vars/all`` [3]_
#. inventory file relative ``group_vars/*`` [3]_
#. playbook file relative ``group_vars/*`` [3]_
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might read better reversed like:

#. ``group_vars/all`` relative to inventory file [3]_
#. ``group_vars/all`` relative to playbook file [3]_
#. ``group_vars/*`` relative to inventory file [3]_
#. ``group_vars/*`` relative to playbook file [3]_

Comment on lines +320 to +321
#. inventory file relative ``host_vars/*`` [3]_
#. playbook file relative ``host_vars/*`` [3]_
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might read better reversed like:

#. ``host_vars/*`` relative to inventory file [3]_
#. ``host_vars/*`` relative to playbook file [3]_

#. play ``vars``
#. play ``vars_prompt``
#. play ``vars_files``
#. role vars from all roles (defined in ``roles/*/vars/main.yml`` or ``vars:``)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the colon is useful to visually cue the reader that it should be defined in an ansible/playbook/task/whatever file rather than in a standalone vars file.

However, as a reader of the docs, I would not know where this vars: would be defined. Where would you put vars: inside a role? On a task? Does vars: on a task count as a roles var here? Does this mean if I have a list of roles defined in a playbook, and set a variable for one role, the other roles get that variable too?

#. role vars from all roles (defined in ``roles/*/vars/main.yml`` or ``vars:``)
#. role dependency vars for tasks from a role (defined in ``roles/{dependent_roles}/vars/main.yml``)
#. role vars for tasks from a role (defined in ``role/vars/main.yml``) [5]_ [8]_ [9]_
#. block ``vars`` (only for tasks in block)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing a between "in" and "block".

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or make block plural

@ansibot ansibot added the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Apr 18, 2021
@sivel sivel marked this pull request as draft June 1, 2021 15:30
@ansibot ansibot added WIP This issue/PR is a work in progress. Nevertheless it was shared for getting input from peers. and removed core_review In order to be merged, this PR must follow the core review workflow. labels Jun 1, 2021
@ansibot
Copy link
Contributor

ansibot commented Mar 26, 2022

Thanks for your Ansible docs contribution! We talk about Ansible documentation on maxtrix at #docs:ansible.im and on libera IRC at #ansible-docs if you ever want to join us and chat about the docs! We meet there on Tuesdays (see the Ansible calendar) and welcome additions to our weekly agenda items - scroll down to find the upcoming agenda and add a comment to put something new on that agenda.

click here for bot help

@ansibot ansibot added the needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html label Jul 25, 2022
@acozine
Copy link
Contributor

acozine commented Apr 14, 2023

@samccann @oraNod @sivel I'm doing a little GitHub tidying today. I will unassign myself from this PR since I no longer have merge privileges.

@acozine acozine removed their assignment Apr 14, 2023
@ansibot ansibot added needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI. and removed stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. labels Jun 25, 2023
@ansibot ansibot added needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html and removed needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI. labels Oct 24, 2023
@ansibot ansibot added the stale_pr This PR has not been pushed to for more than one year. label Jan 28, 2025
@sivel sivel added the unimportant_ci This PR does not need to have healthy CI status and should be ignored by the CI infra maintainers. label Jan 29, 2025
@sivel sivel closed this Feb 18, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affects_2.11 docs_only All changes are to files within the docs/docsite/ directory docs This issue/PR relates to or includes documentation. has_issue needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html stale_pr This PR has not been pushed to for more than one year. support:core This issue/PR relates to code supported by the Ansible Engineering Team. unimportant_ci This PR does not need to have healthy CI status and should be ignored by the CI infra maintainers. WIP This issue/PR is a work in progress. Nevertheless it was shared for getting input from peers.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dependency variables declared in 'meta/main.yml' overwrite default variables
7 participants