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.

Sign up for GitHub

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

Jump to bottom

Make using blocks as handlers a parser error #79993

Merged
merged 2 commits into from Feb 14, 2023
Merged

Make using blocks as handlers a parser error #79993

merged 2 commits into from Feb 14, 2023

Conversation

mkrizek
Copy link
Contributor

@mkrizek mkrizek commented Feb 14, 2023

SUMMARY

Fixes #79968

ci_complete

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

lib/ansible/playbook/helpers.py

@ansibot ansibot added WIP This issue/PR is a work in progress. Nevertheless it was shared for getting input from peers. affects_2.15 bug This issue/PR relates to a bug. needs_triage Needs a first human triage before being processed. labels Feb 14, 2023
@mkrizek mkrizek removed the needs_triage Needs a first human triage before being processed. label Feb 14, 2023
@mkrizek mkrizek marked this pull request as ready for review February 14, 2023 15:02
@ansibot ansibot added 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. labels Feb 14, 2023
sivel
sivel approved these changes Feb 14, 2023
View reviewed changes
Copy link
Member

@sivel sivel left a comment

Choose a reason for hiding this comment

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

This looks fine to me, but I simultaneously question whether there is technically a bug in _get_next_task_from_state since it is returning something that is not a Task. Or maybe that task is just poorly named at this point. In either case, this change is fine on its own.

@ansibot ansibot removed the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. label Feb 14, 2023
@mkrizek
Copy link
Contributor Author

mkrizek commented Feb 14, 2023

This looks fine to me, but I simultaneously question whether there is technically a bug in _get_next_task_from_state since it is returning something that is not a Task.

Normally if a state sees a block it recursively enters a new nested state and takes a task from that block. Since we don't support that for handlers we could, in addition to this PR, also raise an assertion error if/when IteratingHandlers would have returned a block - just in case there is another code path that this PR doesn't prevent from blocks being stored into Play.handlers ¯_(ツ)_/¯

@sivel
Copy link
Member

sivel commented Feb 14, 2023

Yep, like I said, I'm good with the current state of this PR, but maybe something to look at in the future.

@ansibot ansibot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Feb 14, 2023
@sivel
Copy link
Member

sivel commented Feb 14, 2023

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@ansibot ansibot removed the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. label Feb 14, 2023
@sivel sivel merged commit bd329dc into ansible:devel Feb 14, 2023
@mkrizek mkrizek deleted the issue-79968 branch February 15, 2023 08:40
mkrizek added a commit to mkrizek/ansible that referenced this pull request Feb 15, 2023
@mkrizek
Make using blocks as handlers a parser error (ansible#79993)
38ab6fd 
Fixes ansible#79968

(cherry picked from commit bd329dc)
@mkrizek mkrizek mentioned this pull request Feb 15, 2023
Merged
mkrizek added a commit to mkrizek/ansible that referenced this pull request Feb 16, 2023
@mkrizek
Make using blocks as handlers a parser error (ansible#79993)
222117a 
Fixes ansible#79968

(cherry picked from commit bd329dc)
sivel pushed a commit that referenced this pull request Feb 16, 2023
@mkrizek
Make using blocks as handlers a parser error (#79993) (#80010)
02d9336 
Fixes #79968

(cherry picked from commit bd329dc)
@ansible ansible locked and limited conversation to collaborators Feb 21, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@sivel sivel sivel approved these changes

Assignees
No one assigned
Labels
affects_2.15 bug This issue/PR relates to a bug.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

nested block in handler causes Exception
3 participants
@mkrizek @sivel @ansibot