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

bugfix collection role module target_teams and instance_groups options #14119

Merged

Conversation

sean-m-sullivan
Copy link
Contributor

SUMMARY

Bugfix for #14113

Previously the target teams would be added to actor data, the two options to fix was this, or sepreating their lookup.

ISSUE TYPE
  • Bug, Docs Fix or other nominal change
COMPONENT NAME
  • Collection
AWX VERSION
22.3.0

@github-actions github-actions bot added the component:awx_collection issues related to the collection for controlling AWX label Jun 13, 2023
@sean-m-sullivan sean-m-sullivan force-pushed the collection_role_update_bugfix branch 2 times, most recently from fcb5bdd to 11c894a Compare June 13, 2023 23:51
@@ -268,18 +266,23 @@ def main():
for resource in value:
# Attempt to look up project based on the provided name or ID and lookup data
if key in resources:
Copy link
Member

Choose a reason for hiding this comment

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

I think you're dancing around some defunct code. Consider the main loop/condition

    for key, value in resources.items():
        for resource in value:
            # Attempt to look up project based on the provided name or ID and lookup data
            if key in resources:

Before your change, not everything was nested under if key in resources:, so its issues didn't stand out as much.

First impressions reading this block, the ordering of the conditions is wrong. key is an iteration variable of the outer look, in which case you should never check it inside of the inner loop. So that would dictate switching the 2nd for and the if.

But if you do that, then the more fundamental problem readily becomes apparent. key should never not be in resources, unless the dictionary is modified inside of the loop. I looked at the history, and I don't know if it ever was modified in the loop, and it certainly isn't anymore.

So it appears that if key in resources: should be deleted, because it is a condition which should always be true.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Went through and looked back on what this was based on.
https://github.com/ansible/awx/blob/22.0.0/awx_collection/plugins/modules/role.py#L245

it was a simplification at the time and you are right we don't need the if, or even another block, we just need the basic for as its a lookup of module parameters that were added in, and then need to be sorted to their respective set. actor_data and resource_data

I also discovered the instance group change was done incorrectly in #13784, where it was never added to a field to get the parameter, so I added it and tested it.

I also removed an option in one of the tests so the plurality is tested on its own instead of With a singular which led to the tests not detecting this issue.

@sean-m-sullivan sean-m-sullivan changed the title bugfix collection role module target teams option bugfix collection role module target_teams and instance_groups options Jun 14, 2023
Copy link
Member

@AlanCoding AlanCoding left a comment

Choose a reason for hiding this comment

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

This code looks much better than before, I want you to merge it. The role of the loop over resources makes sense now with a detailed reading.

@sean-m-sullivan
Copy link
Contributor Author

@AlanCoding I am not able to merge or run workflows, please advance this as you want to proceed.

@AlanCoding AlanCoding enabled auto-merge (squash) June 14, 2023 17:43
@AlanCoding AlanCoding merged commit a47bbb5 into ansible:devel Jun 14, 2023
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component:awx_collection issues related to the collection for controlling AWX
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants