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

Remove needless get_next_task_for_host calls #74174

Merged
merged 1 commit into from Apr 28, 2021
Merged

Remove needless get_next_task_for_host calls #74174

merged 1 commit into from Apr 28, 2021

Conversation

mkrizek
Copy link
Contributor

@mkrizek mkrizek commented Apr 7, 2021 •
edited

SUMMARY

Called with peek=True and the return values are not used.

ci_complete

Introduced in c669a38

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

lib/ansible/plugins/strategy/__init__.py

@mkrizek
Remove needless get_next_task_for_host calls
fa87b3a 
Called with `peek=True` and the return values are not used.

ci_complete
@ansibot ansibot added WIP This issue/PR is a work in progress. Nevertheless it was shared for getting input from peers. affects_2.12 bug This issue/PR relates to a bug. docs_only All changes are to files within the docs/docsite/ directory 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 7, 2021
@mkrizek mkrizek marked this pull request as ready for review April 7, 2021 15:08
@mkrizek mkrizek mentioned this pull request Apr 7, 2021
Closed
@ansibot ansibot added core_review In order to be merged, this PR must follow the core review workflow. has_issue and removed WIP This issue/PR is a work in progress. Nevertheless it was shared for getting input from peers. docs_only All changes are to files within the docs/docsite/ directory labels Apr 7, 2021
@samdoran samdoran removed the needs_triage Needs a first human triage before being processed. label Apr 8, 2021
@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 Apr 16, 2021
@mkrizek mkrizek requested a review from Shrews April 22, 2021 14:17
Shrews
Shrews approved these changes Apr 22, 2021
View reviewed changes
Copy link
Contributor

@Shrews Shrews left a comment

Choose a reason for hiding this comment

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

FWIW, I was not able to find a scenario where this would break anything. I tried testing with a playbook with multiple hosts that failed a run_once task to trigger this code, but I saw no breakage with various alterations to the test playbook.

@mkrizek
Copy link
Contributor Author

mkrizek commented Apr 28, 2021

For the record, there was a concern that removing those get_next_task_for_host calls may cause issues because get_host_state which is called from get_next_task_for_host contains a side-effect. Concerns about methods having side-effects in general aside, this should not be an issue here because get_host_state is called from mark_host_failed anyway.

@mkrizek
Copy link
Contributor Author

mkrizek commented Apr 28, 2021

rebuild_merge

@ansibot ansibot added shipit This PR is ready to be merged by Core 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. stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. shipit This PR is ready to be merged by Core needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Apr 28, 2021
@mkrizek
Copy link
Contributor Author

mkrizek commented Apr 28, 2021

bot_status

@ansibot
Copy link
Contributor

ansibot commented Apr 28, 2021

Components

lib/ansible/plugins/strategy/init.py
support: core
maintainers:

Metadata

waiting_on: ansible
changes_requested_by: null
needs_info: False
needs_revision: False
needs_rebase: False
merge_commits: []
too many files or commits: False
mergeable_state: clean
ci_status: success
maintainer_shipits (module maintainers): 0
community_shipits (namespace maintainers): 0
ansible_shipits (core team members): 1
shipit_actors (maintainers or core team members): mkrizek
shipit_actors_other: []
automerge: automerge community support test failed

click here for bot help

@ansibot ansibot merged commit 68accca into ansible:devel Apr 28, 2021
@mkrizek mkrizek deleted the needless-get_next_task_for_host branch April 28, 2021 11:22
maximmasiutin pushed a commit to maximmasiutin/ansible that referenced this pull request Apr 28, 2021
@mkrizek @maximmasiutin
Remove needless get_next_task_for_host calls (ansible#74174)
041f492 
Called with `peek=True` and the return values are not used.

ci_complete
samdoran added a commit to samdoran/ansible that referenced this pull request May 3, 2021
@samdoran
Revert "Remove needless get_next_task_for_host calls (ansible#74174)"
713acc7 
This reverts commit 68accca.
@ansible ansible locked and limited conversation to collaborators May 26, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@Shrews Shrews Shrews approved these changes

Assignees
No one assigned
Labels
affects_2.12 bug This issue/PR relates to a bug. has_issue shipit This PR is ready to be merged by Core small_patch support:core This issue/PR relates to code supported by the Ansible Engineering Team.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@mkrizek @ansibot @Shrews @samdoran