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

Adding loop_control.notify_scope #56817

Open
wants to merge 2 commits into
base: devel
Choose a base branch
from
Open

Conversation

jtyr
Copy link
Contributor

@jtyr jtyr commented May 22, 2019

SUMMARY

When using notify in a loop, notification targets for all loop items are triggered even if only single loop item has been changed. This PR is adding a loop_control toggle which allows to trigger notification targets only for changed items of the loop.

To the attention of @bcoca.

ISSUE TYPE

Feature Pull Request

COMPONENT NAME

loop_control

ADDITIONAL INFORMATION

See more details and examples in the documentation included in the PR.

@ansibot
Copy link
Contributor

ansibot commented May 22, 2019

@ansibot ansibot added affects_2.9 This issue/PR affects Ansible v2.9 core_review In order to be merged, this PR must follow the core review workflow. 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. 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 May 22, 2019
@bcoca bcoca added P3 Priority 3 - Approved, No Time Limitation and removed needs_triage Needs a first human triage before being processed. labels May 28, 2019
Copy link
Member

@bcoca bcoca left a comment

Choose a reason for hiding this comment

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

Also brought up at irc meeting, general consensus is +1 on feature, you might get people bikeshedding on name later though

lib/ansible/executor/task_executor.py Outdated Show resolved Hide resolved
@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 May 28, 2019
@jtyr jtyr force-pushed the jtyr-notify_scope branch 2 times, most recently from 1634bf4 to 2065f8d Compare May 29, 2019 23:20
@@ -497,7 +499,7 @@ def _squash_items(self, items, loop_var, variables):
self._task.args['name'] = name
return items

def _execute(self, variables=None):
def _execute(self, variables=None, notify_scope='task'):
Copy link
Contributor

Choose a reason for hiding this comment

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

I would turn notify_scope parameter into an instance variable, self._notify_scope. That way you don't have to pass it around.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍 to making this an instance attribute.

self._task.notify is not None and (
notify_scope != 'per_loop_item' or (
notify_scope == 'per_loop_item' and
result['changed']))):
Copy link
Contributor

Choose a reason for hiding this comment

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

This is probably just me but I had a hard time parsing that. I would change notify_scope != 'per_loop_item' into notify_scope == 'task' and put it all on the one or two lines maybe. Not a blocker, of course.

Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't account for invalid values. I'm wondering if this would be better as a boolean value rather than a string field. Something like notify_per_item with a default of False.

Are there other values for this option that would make it better suited to a string than a boolean?

If we do keep it as a string value, maybe the options could be task, item or task, per_item. Just throwing out ideas.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is easier to read:

if self._task.notify is not None:
    if notify_per_loop_item:
        if result['changed']:
            result['_ansible_notify'] = self._task.notify
    else:
        result['_ansible_notify'] = self._task.notify

Or using the existing string option:

if self._task.notify is not None:
    if notify_scope not in ['task', 'per_loop_item']:
        raise AnsibleError("Invalid option '{0}' given for notify_scope. Valid options are "
                           "'task' or 'per_loop_item'".format(to_text(notify_scope)))
    elif notify_scope == 'per_loop_item':
        if result['changed']:
            result['_ansible_notify'] = self._task.notify
    else:
        result['_ansible_notify'] = self._task.notify

Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't account for invalid values.

Yeah, I realized that when suggested notify_per_item below, too ;-) I was wondering if we had anything like choices for FieldAttribute so we didn't have to check for invalid types here in the code but not that I could find.

Copy link
Member

Choose a reason for hiding this comment

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

The way we have done this before is via something like _validate_debugger. That method effectively does this, but for debugger

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice. So I'd go with _validate_$whatever_name_we_bikeshed method to check for invalid values. Unless we decide to use a boolean.

@@ -402,6 +402,45 @@ Variable Description
loop_control:
extended: yes

Notification scope in the loop
Copy link
Contributor

Choose a reason for hiding this comment

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

@acozine @samccann would you mind reviewing the docs part please? Thanks!

@mkrizek
Copy link
Contributor

mkrizek commented Jun 6, 2019

Bikeshedding: can we maybe do something like loop_control.notify_per_item which would be bool, no by default as the current behavior?

Unless we plan to add more options in the future.

@ansibot ansibot added stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. stale_review Updates were made after the last review and the last review is more than 7 days old. labels Jun 7, 2019
@@ -497,7 +499,7 @@ def _squash_items(self, items, loop_var, variables):
self._task.args['name'] = name
return items

def _execute(self, variables=None):
def _execute(self, variables=None, notify_scope='task'):
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 to making this an instance attribute.

self._task.notify is not None and (
notify_scope != 'per_loop_item' or (
notify_scope == 'per_loop_item' and
result['changed']))):
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't account for invalid values. I'm wondering if this would be better as a boolean value rather than a string field. Something like notify_per_item with a default of False.

Are there other values for this option that would make it better suited to a string than a boolean?

If we do keep it as a string value, maybe the options could be task, item or task, per_item. Just throwing out ideas.

self._task.notify is not None and (
notify_scope != 'per_loop_item' or (
notify_scope == 'per_loop_item' and
result['changed']))):
Copy link
Contributor

Choose a reason for hiding this comment

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

This is easier to read:

if self._task.notify is not None:
    if notify_per_loop_item:
        if result['changed']:
            result['_ansible_notify'] = self._task.notify
    else:
        result['_ansible_notify'] = self._task.notify

Or using the existing string option:

if self._task.notify is not None:
    if notify_scope not in ['task', 'per_loop_item']:
        raise AnsibleError("Invalid option '{0}' given for notify_scope. Valid options are "
                           "'task' or 'per_loop_item'".format(to_text(notify_scope)))
    elif notify_scope == 'per_loop_item':
        if result['changed']:
            result['_ansible_notify'] = self._task.notify
    else:
        result['_ansible_notify'] = self._task.notify


- name: Set handler variables to known value
set_fact:
handler_one: null
Copy link
Contributor

Choose a reason for hiding this comment

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

You could use ~ rather than null.

- name: Test all handlers were run
assert:
that:
- handler_one != None
Copy link
Contributor

@samdoran samdoran Jun 7, 2019

Choose a reason for hiding this comment

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

Using a test rather than a comparison is a bit more reliable. Types with Jijna can be a pain.

Suggested change
- handler_one != None
- handler_one is not none

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.

one minor doc nit.

------------------------------
.. versionadded:: 2.9

By default, if each loop item triggers different notification target, all targets for all items get triggered even if
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
By default, if each loop item triggers different notification target, all targets for all items get triggered even if
By default, if each loop item triggers a different notification target, all targets for all items get triggered even if

@ansibot ansibot removed the stale_review Updates were made after the last review and the last review is more than 7 days old. label Jun 28, 2019
@ansibot ansibot added the stale_review Updates were made after the last review and the last review is more than 7 days old. label Jul 6, 2019
@ansibot ansibot added needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html and removed stale_review Updates were made after the last review and the last review is more than 7 days old. labels Jul 14, 2019
@ansibot ansibot added pre_azp This PR was last tested before migration to Azure Pipelines. 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 Dec 5, 2020
@samdoran samdoran removed their assignment Jan 7, 2021
@ansibot ansibot removed the has_issue label Jul 12, 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 labels Oct 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affects_2.9 This issue/PR affects Ansible v2.9 docs This issue/PR relates to or includes documentation. feature This issue/PR relates to a feature request. 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. P3 Priority 3 - Approved, No Time Limitation pre_azp This PR was last tested before migration to Azure Pipelines. 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.

None yet

7 participants