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

attempt to correct docs on role deduplication #1123

Draft
wants to merge 1 commit into
base: devel
Choose a base branch
from

Conversation

acozine
Copy link
Contributor

@acozine acozine commented Feb 12, 2024

Possible fix for #90.

If I've read the various issues and comments correctly, this should reflect the current behavior of Ansible. If it does not, I hope comments and corrections on this PR will get us to the correct documentation.

@ansible-documentation-bot ansible-documentation-bot bot added the new_contributor This PR is the first contribution by a new community member. label Feb 12, 2024
@ansible-documentation-bot
Copy link
Contributor

Thanks for your Ansible docs contribution! We talk about Ansible documentation on matrix 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.

@acozine
Copy link
Contributor Author

acozine commented Feb 12, 2024

Related to ansible/ansible#82677.

@acozine acozine marked this pull request as draft February 13, 2024 16:40
@acozine
Copy link
Contributor Author

acozine commented Feb 13, 2024

The code has been updated since the original issue was filed. Since this change both import_role and include_role are affected by allow_duplicates. This PR may not be necessary at all, or we may want to update some bits of this documentation.

@oraNod oraNod added DaWGs Good discussion item for the DaWGs techreview needs technical review labels Feb 13, 2024
@nitzmahone nitzmahone requested a review from sivel February 29, 2024 19:17
@@ -156,7 +156,7 @@ When using ``vars:`` within the ``roles:`` section of a playbook, the variables
Including roles: dynamic reuse
------------------------------

You can reuse roles dynamically anywhere in the ``tasks`` section of a play using ``include_role``. While roles added in a ``roles`` section run before any other tasks in a play, included roles run in the order they are defined. If there are other tasks before an ``include_role`` task, the other tasks will run first.
You can reuse roles dynamically anywhere in the ``tasks`` section of a play using ``include_role``. While roles added in a ``roles`` section run before any other tasks in a play, included roles run in the order they are defined. If there are other tasks before an ``include_role`` task, the other tasks will run first. Ansible does not deduplicate included roles, even if they have the exact same parameters, and setting ``allow_duplicates: false`` has no effect on ``include_role``.
Copy link
Member

Choose a reason for hiding this comment

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

It does but it is a bit limited, it includes the vars context for the 'signature' to dedupe on, which makes it very hard for the include_role signatures to match. import_role, being run in a 'preprocess' state don't have the variability present so the signature is a lot more stable, matching the behvior of roles:.

@@ -246,6 +246,8 @@ You can pass other keywords, including variables and tags when importing roles:

When you add a tag to an ``import_role`` statement, Ansible applies the tag to `all` tasks within the role. See :ref:`tag_inheritance` for details.

Ansible does not deduplicate imported roles, even if they have the exact same parameters, and setting ``allow_duplicates: false`` has no effect on ``import_role``.

.. _role_argument_spec:
Copy link
Member

Choose a reason for hiding this comment

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

see comment above, it should dedupe now in most cases

-----------------------------------------

Ansible does not deduplicate imported or included roles. When you use ``include_role`` or ``import_role``, Ansible runs each role, as long as any conditions on those tasks are met. This is true even if the parameters are the same on multiple included or imported roles.

Copy link
Member

Choose a reason for hiding this comment

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

they have a allow_duplicates parameter that overrides the one from the role's meta/main.yml

@Akasurde Akasurde added the core_approval This PR requires approval from the Ansible Core team label Apr 16, 2024
@@ -69,6 +69,8 @@ Task include and import statements can be used at arbitrary depth.

You can still use the bare :ref:`roles <roles_keyword>` keyword at the play level to incorporate a role in a playbook statically. However, the bare :ref:`include <include_module>` keyword, once used for both task files and playbook-level includes, is now deprecated.

Ansible runs all roles reused with ``include_role`` and ``import_role``, even if the parameters on the roles are the same. See :ref:`run_role_twice` for more details.
Copy link
Contributor

@s-hertel s-hertel Apr 18, 2024

Choose a reason for hiding this comment

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

The behavior on devel, which I believe is correct, runs 5 of the 8 role invocations, deduplicating the other 3, for this playbook:

- hosts: localhost
  gather_facts: no
  roles:
    - name: role  # runs
    - name: role  # does not run (unless role has allow_duplicates: true in the meta/main.yml, the default is false)
    - name: role  # runs again regardless of the meta/main.yml value
      allow_duplicates: true
  tasks:
    # include_role and import_role do not use allow_duplicates in the meta/main.yml
    # the allow_duplicates parameter overrides it, and defaults to true for these
    
    - import_role:
        name: role  # runs (not a duplicate of roles keyword even with allow_duplicates=false)
    - import_role:
        name: role  # runs again (allow_duplicates defaults to true)
    - import_role:
        name: role  # does not run
        allow_duplicates: false

    - include_role:
        name: role  # does not run (duplicate of import_role)
        allow_duplicates: false
    - include_role:
        name: role  # runs again

@mattclay mattclay removed the techreview needs technical review label Jun 6, 2024
@oraNod
Copy link
Contributor

oraNod commented Sep 12, 2024

Hi @acozine We've made some changes to the underlying readthedocs project for this repository. I'm going to close and re-open this issue to kick off a new PR preview build.

You might notice there will be two checks for readthedocs. One for docs/readthedocs.org:stage-ansible-core and another for docs/readthedocs.org:ansible-core.

The stage-ansible-core check is for the old project and will result in a 404. Please ignore that and use the ansible-core check only.

Thank you for your patience and understanding.

@oraNod oraNod closed this Sep 12, 2024
@oraNod oraNod reopened this Sep 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core_approval This PR requires approval from the Ansible Core team DaWGs Good discussion item for the DaWGs new_contributor This PR is the first contribution by a new community member.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants