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

Fix notifying handlers by using an exact match #55624

Merged
merged 14 commits into from
Jun 27, 2019

Conversation

s-hertel
Copy link
Contributor

SUMMARY

Fixes #55575

Introduced in 2a469fd.

'string' in 'stringwithmore' is True, so make sure to wrap listeners in a list to get only exact matches: 'string' in ['stringwithmore'] is False.

ISSUE TYPE
  • Bugfix Pull Request

@ansibot ansibot added affects_2.9 This issue/PR affects Ansible v2.9 bug This issue/PR relates to a bug. core_review In order to be merged, this PR must follow the core review workflow. needs_triage Needs a first human triage before being processed. small_patch support:core This issue/PR relates to code supported by the Ansible Engineering Team. labels Apr 22, 2019
@s-hertel s-hertel changed the title Fix notifying handlers by using an exact match [WIP] Fix notifying handlers by using an exact match Apr 22, 2019
@ansibot ansibot added the WIP This issue/PR is a work in progress. Nevertheless it was shared for getting input from peers. label Apr 22, 2019
@s-hertel s-hertel removed the needs_triage Needs a first human triage before being processed. label Apr 23, 2019
@s-hertel s-hertel changed the title [WIP] Fix notifying handlers by using an exact match Fix notifying handlers by using an exact match Apr 23, 2019
@ansibot ansibot added support:community This issue/PR relates to code supported by the Ansible community. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed WIP This issue/PR is a work in progress. Nevertheless it was shared for getting input from peers. core_review In order to be merged, this PR must follow the core review workflow. labels Apr 23, 2019
@ansibot ansibot added core_review In order to be merged, this PR must follow the core review workflow. needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI. 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 needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI. stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. core_review In order to be merged, this PR must follow the core review workflow. labels Apr 24, 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 26, 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 May 16, 2019
@twouters
Copy link
Contributor

Can this be merged? I think this is a serious bug in ansible 2.8

@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 Jun 14, 2019
@s-hertel
Copy link
Contributor Author

I talked to @bcoca a little more about this one and have come to the conclusion that there is parity with the templating behavior on devel and previous versions of Ansible. I added tests to make sure that remains the same and a couple notes to the documentation to clarify.

@ansibot
Copy link
Contributor

ansibot commented Jun 14, 2019

@ansibot ansibot added the docs This issue/PR relates to or includes documentation. label Jun 14, 2019
@@ -367,14 +367,17 @@ def get_original_host(host_name):
else:
return self._inventory.get_host(host_name)

def get_handler_templar(handler_task):
Copy link
Member

Choose a reason for hiding this comment

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

Does this need to be a local function? Several of the others are, because they are using context from _process_pending_results. I don't think this one needs to though, it would just require passing in iterator also.

Not a blocker, just a potential improvement.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the suggestion, fixed that.

@ansibot ansibot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. core_review In order to be merged, this PR must follow the core review workflow. and removed core_review In order to be merged, this PR must follow the core review workflow. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Jun 14, 2019
Copy link
Contributor

@samccann samccann left a comment

Choose a reason for hiding this comment

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

docs portion LGTM

@ansibot ansibot added shipit This PR is ready to be merged by Core and removed core_review In order to be merged, this PR must follow the core review workflow. labels Jun 21, 2019
@s-hertel s-hertel merged commit ec1287c into ansible:devel Jun 27, 2019
s-hertel added a commit to s-hertel/ansible that referenced this pull request Jun 27, 2019
* Fix notifying handlers by using an exact match rather than a string subset if listen is text rather than a list

* Enforce better type checking for listeners

* Share code for validating handler listeners

* Add test for handlers without names

* Add test for templating in handlers

* Add test for include_role

* Add a couple notes about 'listen' for handlers

* changelog

(cherry picked from commit ec1287c)
abadger pushed a commit that referenced this pull request Jun 28, 2019
* Fix notifying handlers by using an exact match rather than a string subset if listen is text rather than a list

* Enforce better type checking for listeners

* Share code for validating handler listeners

* Add test for handlers without names

* Add test for templating in handlers

* Add test for include_role

* Add a couple notes about 'listen' for handlers

* changelog

(cherry picked from commit ec1287c)
agowa pushed a commit to agowa/ansible-1 that referenced this pull request Jun 30, 2019
* Fix notifying handlers by using an exact match rather than a string subset if listen is text rather than a list

* Enforce better type checking for listeners

* Share code for validating handler listeners

* Add test for handlers without names

* Add test for templating in handlers

* Add test for include_role

* Add a couple notes about 'listen' for handlers

* changelog

* Add a test for handlers without names

* Test templating in handlers

* changelog

* Add some tests for include_role

* Add a couple notes about 'listen' for handlers

* make more sense

* move local function into a class method
@ansible ansible locked and limited conversation to collaborators Aug 5, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
affects_2.9 This issue/PR affects Ansible v2.9 bug This issue/PR relates to a bug. docs This issue/PR relates to or includes documentation. shipit This PR is ready to be merged by Core 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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

notify calls an unexpected listener
6 participants