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

don't pass the 'organization' or other fields to the search of the instance group or execution environments #14223

Merged
merged 11 commits into from
Sep 14, 2023
Merged

Conversation

ivarmu
Copy link
Contributor

@ivarmu ivarmu commented Jul 11, 2023

Bug, Docs Fix or other nominal change

SUMMARY

Fixes a bug detected here: redhat-cop/infra.aap_configuration#624

  • If instance_groups are specified in the input schedule, the Schedule role fails to get the specified instance group
  • If execution_environment is specified in the input schedule, the Schedule role fails to get the specified execution_environment
ISSUE TYPE
  • Bug
COMPONENT NAME
  • Collection
AWX VERSION

redhat-cop/infra.aap_configuration#624

ADDITIONAL INFORMATION

redhat-cop/infra.aap_configuration#624

@github-actions github-actions bot added component:awx_collection issues related to the collection for controlling AWX community labels Jul 11, 2023
@ivarmu ivarmu changed the title don't pass the 'organization' or other fields to the search of the instance group don't pass the 'organization' or other fields to the search of the instance group or execution environments Jul 11, 2023
@ivarmu
Copy link
Contributor Author

ivarmu commented Jul 27, 2023

@gamuniz, @john-westcott-iv: any feedback related to this bugfix?

@john-westcott-iv
Copy link
Member

@ivarmu can you add/modify the tests in https://github.com/ansible/awx/blob/devel/awx_collection/tests/integration/targets/schedule/tasks/main.yml and/or https://github.com/ansible/awx/blob/devel/awx_collection/test/awx/test_schedule.py to catch this issue so we can test it before and after your change to make sure we don't get a regression in the future?

Also, is this "fully" broken or does it work now under some circumstances? I'm asking to find out if we need a release/doc note if the behavior is changing.

@ivarmu
Copy link
Contributor Author

ivarmu commented Jul 28, 2023

@john-westcott-iv , I'm adding the tests and pushing the changes.

I think this is fully broken as the detailed fields are never returned through the schedules api.

@ivarmu
Copy link
Contributor Author

ivarmu commented Jul 28, 2023

@john-westcott-iv , I'm adding the tests and pushing the changes.

I think this is fully broken as the detailed fields are never returned through the schedules api.

The tests were already implemmented, the only parameter that was not tested at all was passing the organization field to the schedule module (see

,
organization=dict(),
and
organization = module.params.get('organization')
).

I've changed the direction of the patch, letting other parameters to be passed to the API calls if needed. I was thinking on removing the field organization like the name one is removed here: https://github.com/ansible/awx/blob/ab5cc2e69c68a1fa763d181ec15b986af78e531d/awx_collection/plugins/modules/schedule.py#L283C1-L285C34 ... This would be a clearer solution, but I'm not sure that the organization field could be used after these API calls (by the execution_environment API call, for example)... @john-westcott-iv what do you think about that? Maybe a change ordering the tasks would allow that?

@ivarmu
Copy link
Contributor Author

ivarmu commented Jul 28, 2023

Nah!, I've done it, it seems more elegant than before :-)

@ivarmu
Copy link
Contributor Author

ivarmu commented Aug 21, 2023

@gamuniz, @john-westcott-iv: I know these may be holiday days... when you become ready again... any advance on this PR?

@ivarmu
Copy link
Contributor Author

ivarmu commented Sep 8, 2023

@gamuniz, @john-westcott-iv: putting this bugfix in your radar again :-)

@john-westcott-iv john-westcott-iv merged commit 49832d6 into ansible:devel Sep 14, 2023
18 checks passed
djyasin pushed a commit to djyasin/awx that referenced this pull request Sep 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community component:awx_collection issues related to the collection for controlling AWX
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants