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

feat: remove collection support for oauth #15623

Conversation

PabloHiro
Copy link
Contributor

SUMMARY
ISSUE TYPE
  • Breaking Change
COMPONENT NAME
  • Collection
AWX VERSION
TBD
ADDITIONAL INFORMATION

@github-actions github-actions bot added component:awx_collection issues related to the collection for controlling AWX community labels Nov 8, 2024
@PabloHiro PabloHiro force-pushed the feature/remove-collection-oauth branch 3 times, most recently from 68ed9a5 to 634d445 Compare November 8, 2024 15:38
@AlanCoding
Copy link
Member

From output:

Run command with data: /root/.ansible/test/venv/sanity.runtime-metadata/3.12/c33cecd0/bin/python /root/ansible/test/lib/ansible_test/_util/controller/sanity/code-smell/runtime-metadata.py
ERROR: Found 1 runtime-metadata issue(s) which need to be resolved:
ERROR: meta/runtime.yml:0:0: Removal version must be a semantic version (https://semver.org/) for dictionary value @ data['plugin_routing']['inventory']['token']['tombstone']['removal_version']. Got 'X.Y.Z'

Our collection often can not comply with the Ansible sanity rules because of the dual-versioning system where it vendors as both ansible.controller and awx.awx which have different versions.

@PabloHiro PabloHiro force-pushed the feature/remove-collection-oauth branch 2 times, most recently from 5fa8667 to 310cb6f Compare November 11, 2024 14:10
@PabloHiro PabloHiro force-pushed the feature/remove-collection-oauth branch from 310cb6f to b490ef2 Compare November 11, 2024 16:55
@PabloHiro PabloHiro marked this pull request as ready for review November 11, 2024 17:01
@PabloHiro PabloHiro force-pushed the feature/remove-collection-oauth branch 3 times, most recently from 14e0988 to e052876 Compare November 12, 2024 17:57
@PabloHiro PabloHiro force-pushed the feature/remove-collection-oauth branch 2 times, most recently from 788938f to 327d784 Compare November 12, 2024 18:24
@PabloHiro
Copy link
Contributor Author

@AlanCoding I think I need the CLI PR merged first. The CI fails when testing the export module, which still considers application as an exportable asset.

Copy link
Collaborator

@hakbailey hakbailey left a comment

Choose a reason for hiding this comment

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

Just a couple of small comments, otherwise this looks good.

awx_collection/README.md Outdated Show resolved Hide resolved
awx_collection/plugins/module_utils/controller_api.py Outdated Show resolved Hide resolved
@PabloHiro PabloHiro force-pushed the feature/remove-collection-oauth branch from 327d784 to 299e0cc Compare November 13, 2024 16:01
@AlanCoding
Copy link
Member

The CI fails when testing the export module, which still considers application as an exportable asset.

That's just a single failure, and a legitimate point of overlap between the two PRs. I don't see any problem with us both making the same obvious change (deleting applications from options and use), and it likely won't require manual intervention in the rebase.

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.

One collection test needs "application" removed in one place and then this is otherwise in good shape.

@PabloHiro
Copy link
Contributor Author

One collection test needs "application" removed in one place and then this is otherwise in good shape.

@AlanCoding The parameters are retrieved automatically from an API/and-or whatever is available in AWXKIT so that is why we would need to merge #15613 (or maybe #15588 too) first before the CI passes

@AlanCoding
Copy link
Member

#15613 has another blocker that I'd like to not block this with. Maybe you can skip around that test or something temporarily since it'll be resolved by the other merge anyway.

Otherwise I was expecting this to merge first, but we can get together and coordinate order if needed. I'll go ahead and approve because my comment was self-evident from the checks anyway.

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.

Remaining issues are related to merge-order coordination. This should be otherwise ready to go and I don't see any reason to delay.

@AlanCoding
Copy link
Member

Request to merge into feature branch

#15647

@PabloHiro PabloHiro changed the base branch from devel to feature_token_removal November 19, 2024 16:49
self.connection.login(username=self.username, password=self.password)
self.authenticated = True
self.connection.login(username=self.username, password=self.password)
self.authenticated = True
Copy link
Member

Choose a reason for hiding this comment

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

This is the merge conflict and my branch actually has:

MERGE: fix conflicts with removal of OAuth2 token from collection branch

So there's no lack of clarity there (take this diff). We just need to formally have that happen.

@AlanCoding AlanCoding merged commit a9ef11c into ansible:feature_token_removal Nov 19, 2024
16 of 17 checks passed
@PabloHiro PabloHiro deleted the feature/remove-collection-oauth branch November 20, 2024 08:56
AlanCoding added a commit that referenced this pull request Nov 20, 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.

4 participants