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

ansible_parent_role_name/path variables for when a role is being included by another role #46687

Merged
merged 1 commit into from
Apr 23, 2019
Merged

ansible_parent_role_name/path variables for when a role is being included by another role #46687

merged 1 commit into from
Apr 23, 2019

Conversation

Xaroth
Copy link
Contributor

@Xaroth Xaroth commented Oct 9, 2018

SUMMARY

Currently it is not possible for roles to detect when they are being directly included by another role.

This PR does the following:

  • When a role is being included, set the ansible_parent_role_name to the name of the role that included this role.
  • When a role is being included, set the ansible_parent_role_path to the path of the role that included this role.
  • Document the role_name magic variable. It existed, but there was no existing documentation. Its companion role_path was already documented.

The main rationale for this is that we currently have a large set of roles that, in addition of providing with a state that is being managed, also expose a set of extra callable task files to allow other roles to easily perform tasks related to that role, without having to duplicate logic several dozen times.

During these includes, it is often required to send an identifier along, so that comments can indicate which role/feature requested these actions. Currently this is done by passing variables to each include_role action, but this proves to be prone to error.
The role_name variable already exists, but since this only stores the current executing role, it doesn't help trace down the source if the role was included otherwise.

The proposed new magic variables do specifically that; letting the included roles know who included them, so that it's easier to build conditions that only run (or should never run) when tasks are being included, and to be able to better debug the source of configuration.

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

Magic Variables

ANSIBLE VERSION
ansible 2.7.0

but this affects all versions.

ADDITIONAL INFORMATION

@ansibot
Copy link
Contributor

ansibot commented Oct 9, 2018

@Xaroth: thank you for submitting this pull-request!

cc @acozine
click here for bot help

@ansibot ansibot added affects_2.8 This issue/PR affects Ansible v2.8 docs This issue/PR relates to or includes documentation. feature This issue/PR relates to a feature request. needs_triage Needs a first human triage before being processed. new_contributor This PR is the first contribution by a new community member. support:community This issue/PR relates to code supported by the Ansible community. support:core This issue/PR relates to code supported by the Ansible Engineering Team. labels Oct 9, 2018
@Xaroth
Copy link
Contributor Author

Xaroth commented Oct 9, 2018

From what I can see the fails in the CI are not related to the code.

@webknjaz
Copy link
Member

webknjaz commented Oct 9, 2018

Hi @Xaroth,

It looks like an architectural change, which I believe requires a broader discussion.
Could you please go ahead and create a proposal in https://github.com/ansible/proposals/issues/new pointing to this PR as a reference implementation?

Thanks in advance!

@webknjaz webknjaz removed the needs_triage Needs a first human triage before being processed. label Oct 9, 2018
@ansibot ansibot added the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. label Oct 9, 2018
@Xaroth
Copy link
Contributor Author

Xaroth commented Oct 9, 2018

@webknjaz Thanks, proposal sent in.

@ansibot ansibot removed the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. label Oct 9, 2018
Copy link
Contributor

@gundalow gundalow left a comment

Choose a reason for hiding this comment

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

Would be good to add some tests to defend this functionality

@bcoca
Copy link
Member

bcoca commented Oct 12, 2018

i would make this a list so you can trace back the full stack, not just the immediate parent

@Xaroth
Copy link
Contributor Author

Xaroth commented Oct 15, 2018

@bcoca I updated the PR with your comments. I also added tests as per the request of @gundalow

@ansibot ansibot added 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 Oct 24, 2018
@ansibot ansibot added needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html 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. needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Nov 9, 2018
@ansibot ansibot added needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. and removed core_review In order to be merged, this PR must follow the core review workflow. needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html 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 Mar 29, 2019
@samdoran samdoran self-assigned this Apr 2, 2019
@ansibot ansibot added core_review In order to be merged, this PR must follow the core review workflow. and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Apr 2, 2019
@Xaroth
Copy link
Contributor Author

Xaroth commented Apr 3, 2019

I added an extra test case to ensure that include_tasks and import_tasks does not affect the ansible_parent_role_names vars.

Copy link
Contributor

@jamescassell jamescassell left a comment

Choose a reason for hiding this comment

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

shipit

This will bring very useful functionality, and appears to be well tested and documented. For bonus points, the code change itself is trivial.

@ansibot ansibot added 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. labels Apr 4, 2019
-Add: Test cases for ansible_parent_role_names and ansible_parent_role_paths
-Add: ansible_parent_role_names/paths variables for when a role is being included by another role.
@Xaroth
Copy link
Contributor Author

Xaroth commented Apr 4, 2019

Added changes requested by @samdoran:

  • Make documentation reflect that both include_role as well as import_role cause this behavior.
  • Add links to the include_role and import_role modules from that documentation, so that it's easier to find those module specifics
  • Added tests for ansible_parent_role_paths. Currently only checking for lenght equality to ansible_parent_role_names as the tempdirs created by ansible-test make it difficult to fully check path values.
  • Added tests to ensure that both ansible_parent_role_names and ansible_parent_role_paths are undefined prior to roles being included/imported, and after they have been included/imported.
  • Added tests to ensure that the values reset to their original values after importing/including a role.

These changes add a lot of tests to the mix (around 50 new actions for the special_vars tests), but this shouldn't affect test runtime by much.

Copy link
Contributor

@samdoran samdoran left a comment

Choose a reason for hiding this comment

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

I'll merge this for 2.9 after we create a branch for 2.8.

@samdoran samdoran added this to TODO: Backlog, anything can go here. Anyone can work on this after their approved work is done. in Ansible 2.9 Apr 4, 2019
@ansibot ansibot added core_review In order to be merged, this PR must follow the core review workflow. and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Apr 4, 2019
@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 12, 2019
@ansibot ansibot removed 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 23, 2019
@samdoran samdoran merged commit a9f24e0 into ansible:devel Apr 23, 2019
@bcoca bcoca moved this from TODO: Backlog, anything can go here. Anyone can work on this after their approved work is done. to Merged: PR has been merged but still blocks release until someone checks whether it has all the release criteria in Ansible 2.9 May 15, 2019
ndclt pushed a commit to ndclt/ansible that referenced this pull request Jun 13, 2019
-Add: Test cases for ansible_parent_role_names and ansible_parent_role_paths
-Add: ansible_parent_role_names/paths variables for when a role is being included by another role.
@ansible ansible locked and limited conversation to collaborators Jul 25, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
affects_2.8 This issue/PR affects Ansible v2.8 core_review In order to be merged, this PR must follow the core review workflow. docs This issue/PR relates to or includes documentation. docsite This issue/PR relates to the documentation website. feature This issue/PR relates to a feature request. include_role support:community This issue/PR relates to code supported by the Ansible community. support:core This issue/PR relates to code supported by the Ansible Engineering Team. utilities Utilities category
Projects
No open projects
Ansible 2.9
Merged: PR has been merged but still ...
Development

Successfully merging this pull request may close these issues.

None yet