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 FQCN import tasks include their relevant files for checking #1854

Merged
merged 6 commits into from
Feb 17, 2022
Merged

Make FQCN import tasks include their relevant files for checking #1854

merged 6 commits into from
Feb 17, 2022

Conversation

ryaner
Copy link
Contributor

@ryaner ryaner commented Feb 7, 2022

Tasks can come in in either the short format or the FQCN format
as this is called before noramlize_task functions.

@ryaner ryaner requested a review from a team as a code owner February 7, 2022 21:05
@ryaner ryaner requested review from relrod, ganeshrn and priyamsahoo and removed request for a team February 7, 2022 21:05
@ryaner
Copy link
Contributor Author

ryaner commented Feb 7, 2022

Another option for fixing would be to split the string down on L328 (k, v) = item.
k can either be import_tasks or ansible.builtin.import_tasks.
Including the cases where the functions have FQCNs is slightly faster given the number of times the function is called.

@ryaner ryaner mentioned this pull request Feb 7, 2022
Closed
@ssbarnea ssbarnea added the bug label Feb 8, 2022
ssbarnea
ssbarnea requested changes Feb 8, 2022
View reviewed changes
Copy link
Member

@ssbarnea ssbarnea left a comment

Choose a reason for hiding this comment

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

The fix seems logic and thanks for creating the PR.

Also, please update the tests in order to include a regression prevention measure. Think about a change, even to existing test covering the same functionality, that would fail if we remove your fix.

ryaner added 2 commits February 8, 2022 17:49
@ryaner
Make FQCN import tasks include their relevant files for checking
b3c59dc 
Tasks can come in in either the short format or the FQCN format
as this is called before noramlize_task functions.
@ryaner
Tests for FQCN import_tasks + fix for ansible#1615 import_ should be …
9e82849 
…named

Includes the FQCN in the in the other locations that are called before
normalize functions. This triggers the proper includes in all paths as
well as fixing the `All tasks should be named` error when the FCQN
version is used.
@ryaner
Copy link
Contributor Author

ryaner commented Feb 8, 2022

I combined in the fix for #1615 as adding the FQCN into the tests caused the All tasks should be named to trigger during the tests. If you'd prefer I separate things I can do so, making this dependant on a PR for #1615.

This does not fix the fact that import_ calls do not trigger the FQCN ruleset. I don't see a way to pass the tasks to only a subset of the rulesets.

@ssbarnea ssbarnea requested a review from tadeboro February 9, 2022 11:55
@ssbarnea ssbarnea mentioned this pull request Feb 9, 2022
Merged
@ssbarnea ssbarnea merged commit ce89ad8 into ansible:main Feb 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ssbarnea ssbarnea ssbarnea approved these changes

@relrod relrod Awaiting requested review from relrod relrod was automatically assigned from ansible-community/devtools

@ganeshrn ganeshrn Awaiting requested review from ganeshrn ganeshrn was automatically assigned from ansible-community/devtools

@priyamsahoo priyamsahoo Awaiting requested review from priyamsahoo priyamsahoo was automatically assigned from ansible-community/devtools

@tadeboro tadeboro Awaiting requested review from tadeboro

Assignees
No one assigned
Labels
bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ryaner @ssbarnea