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

Mentions 'meta: flush_handlers' task #79542

Merged
merged 3 commits into from Feb 2, 2023
Merged

Conversation

JaroslavKlech
Copy link
Contributor

@JaroslavKlech JaroslavKlech commented Dec 6, 2022

SUMMARY

Encourage users to use meta: flush_handlers task in the test role itself

ISSUE TYPE
  • Docs Pull Request
COMPONENT NAME

docs.ansible.com

Fixes #59109

@ansibot
Copy link
Contributor

ansibot commented Dec 6, 2022

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.

click here for bot help

@ansibot ansibot added affects_2.15 docs This issue/PR relates to or includes documentation. docs_only All changes are to files within the docs/docsite/ directory needs_triage Needs a first human triage before being processed. small_patch labels Dec 6, 2022
@JaroslavKlech
Copy link
Contributor Author

Hi @oraNod, @samccann,

feel free to review my updates.

@mkrizek mkrizek removed the needs_triage Needs a first human triage before being processed. label Dec 6, 2022
@@ -188,6 +188,8 @@ This is the great culmination of embedded tests:
ansible.builtin.command: /usr/bin/add_back_to_pool {{ inventory_hostname }}
delegate_to: 127.0.0.1

If you need a handler to run before a particular role, use the ``meta`` module with the ``flush_handlers`` option. Consider placing this task in your test role itself. For more information, see :ref:`handlers`.
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems to be out of context. We do document the flush_handlers meta task in the appropriate sections.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

okay rereading this and the issue that spawned this PR. Seems I put the wrong info in the issue. I think this new line likely belongs at https://github.com/ansible/ansible/blame/devel/docs/docsite/rst/reference_appendices/test_strategies.rst#L194 instead. What do you think @mkrizek ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think @samccann meant this https://docs.ansible.com/ansible/latest/collections/ansible/builtin/meta_module.html

@mkrizek, do you think we should remove the paragraph you highlighted and instead put there a reference to https://docs.ansible.com/ansible/latest/collections/ansible/builtin/meta_module.html as it mentions "flush_handlers makes Ansible run any handler tasks which have..." ?

Thanks

Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if it would be better to just fix the example playbook rather than mention what could go wrong with it?
We could rewrite the playbook and move a "call" to apply_testing_checks role into post_tasks via include_role (since flush_handlers is implicitly called before post_tasks is executed). Or run flush_handlers explicitly to make the intent more obvious.

What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

cc: @mkrizek
I wonder if it would be better to just fix the example playbook rather... you mean to update the play/playbook that immediately precedes?

  ---
    - hosts: webservers
      serial: 5
      pre_tasks:
        - name: take out of load balancer pool
          ansible.builtin.command: /usr/bin/take_out_of_pool {{ inventory_hostname }}
          delegate_to: 127.0.0.1
      roles:
         - common
         - webserver
         - apply_testing_checks
      post_tasks:
        - name: add back to load balancer pool
          ansible.builtin.command: /usr/bin/add_back_to_pool {{ inventory_hostname }}
          delegate_to: 127.0.0.1

It sounds good that we update this playbook to make it explicit how flush_handlers should work.

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 for updating the example

@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 Dec 16, 2022
@JaroslavKlech
Copy link
Contributor Author

JaroslavKlech commented Jan 30, 2023

I removed the sentence "If you need a handler to run before...", updated the post_tasks section. As to the flushing of handlers, I wonder if I should add also a task like this somewhere:

- name: Flush handlers
  meta:
    flush_handlers: yes

cc: @mkrizek

@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 Jan 30, 2023
@mkrizek
Copy link
Contributor

mkrizek commented Jan 30, 2023

So the two options I see are:

In this example the flush_handlers is run implicitly before post_tasks.

- hosts: webservers
  serial: 5
  pre_tasks:
    - name: take out of load balancer pool
      ansible.builtin.command: /usr/bin/take_out_of_pool {{ inventory_hostname }}
      delegate_to: 127.0.0.1
  roles:
      - common
      - webserver
  post_tasks:
    - name: test the configuration
      ansible.builtin.include_role:
        name: apply_testing_checks

    - name: add back to load balancer pool
      ansible.builtin.command: /usr/bin/add_back_to_pool {{ inventory_hostname }}
      delegate_to: 127.0.0.1

or run all roles via include_role and call flush_handlers explicitly.

- hosts: webservers
  serial: 5
  pre_tasks:
    - name: take out of load balancer pool
      ansible.builtin.command: /usr/bin/take_out_of_pool {{ inventory_hostname }}
      delegate_to: 127.0.0.1
  tasks:
    - ansible.builtin.include_role:
        name: "{{ item }}"
      loop:
        - common
        - webserver
    
    - name: run any notified handlers
      ansible.builtin.meta: flush_handlers
    
    - name: test the configuration
      ansible.builtin.include_role:
        name: apply_testing_checks
  post_tasks:
    - name: add back to load balancer pool
      ansible.builtin.command: /usr/bin/add_back_to_pool {{ inventory_hostname }}
      delegate_to: 127.0.0.1

I think the second example that runs flush_handlers explicitly documents the scenario better.

@JaroslavKlech
Copy link
Contributor Author

I went with the 2nd option -- run all roles via include_role and call flush_handlers explicitly.

@JaroslavKlech
Copy link
Contributor Author

Hi @samccann,

if @mkrizek does not have any objections i think we can finish this pull request as I implemented his feedback.

@samccann samccann merged commit d8dc76e into ansible:devel Feb 2, 2023
@samccann
Copy link
Contributor

samccann commented Feb 2, 2023

Thanks @JaroslavKlech for the Ansible docs fix!

samccann pushed a commit to samccann/ansible that referenced this pull request Feb 2, 2023
@samccann samccann mentioned this pull request Feb 2, 2023
nitzmahone pushed a commit that referenced this pull request Feb 3, 2023
* [Docs] add easyfix/good first issue/docs links (#79830)

(cherry picked from commit 722fc05)

* [Docs] add doc links to documentation_contributions.rst (#79840)

(cherry picked from commit 58f0950)

* Remove dev_guide stubs (#79795)

* Remove dev_guide stubs

* Remove Cisco ACI Dev Guide

(cherry picked from commit 10f0e5f)

* maintainers_guidelines.rst: add a link to collection release guidelines (#79859)

(cherry picked from commit fa38267)

* Mentions 'meta: flush_handlers' task (#79542)

(cherry picked from commit d8dc76e)

* Description for changing User ID to match user value (#79470)

(cherry picked from commit 913e486)

* Remove irrelevant line (#79865)

Remove irrelevant comment line form example code

(cherry picked from commit 1c01eab)

---------

Co-authored-by: Andrew Klychkov <aaklychkov@mail.ru>
Co-authored-by: Mario Lenz <m@riolenz.de>
Co-authored-by: Andrew Klychkov <aklychko@redhat.com>
Co-authored-by: JaroslavKlech <klechh@gmail.com>
Co-authored-by: Tabah Baridule M <dulemartins07@gmail.com>
Co-authored-by: Konrad Gawda <konrad.gawda.opensource@gmail.com>
@ansible ansible locked and limited conversation to collaborators Feb 9, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
affects_2.15 docs_only All changes are to files within the docs/docsite/ directory docs This issue/PR relates to or includes documentation. has_issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bad advice on "Integrating Testing With Rolling Updates"
4 participants