-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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: Update collection arguments #15627
base: devel
Are you sure you want to change the base?
Conversation
Consistency is the most important thing for this. What other content is using these connection parameters? This? https://galaxy.ansible.com/ui/repo/published/infra/aap_configuration/docs/ |
This is the primary thing using it at the moment, and the installer using it as well, it was more Some Variables should be exposed to playbooks for use, lets set a standard. |
Looking good 👍 I got some more background on this initiative. It looks like you've already taken the lead in proposing these changes elsewhere. I fully support this, but want to wait a few days to assure we've solidified consensus. |
I'm actually not convinced this will work anymore. I expect that if the collection receives I could test this manually to clarify specifically how it's not working, but there's no automated tests we can rely on right now. |
The PR you created here is for the collection. There may be a needed change to eda-server as well. This is probably more consequential, since this is a credential type used in-app, most similar to the awx_plugins change you are making. At some point we hope to essentially merge the 2 CredentialType models, but we are not there yet, so the controller type is more-or-less duplicated in both places. |
It may be needed, didn't know it was there will look and add that to the list
|
- If value not set, will try environment variable C(CONTROLLER_OAUTH_TOKEN) and then config files | ||
type: raw | ||
version_added: "3.7.0" | ||
aliases: [ tower_oauthtoken, aap_token ] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer for the main name to be aap_token
, otherwise looks alright.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated
ea83b58
to
83713ac
Compare
Quality Gate passedIssues Measures |
- If value not set, will try environment variable C(CONTROLLER_OAUTH_TOKEN) and then config files | ||
type: raw | ||
version_added: "3.7.0" | ||
aliases: [ tower_oauthtoken, controller_oauthtoken] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this still make sense?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agree, I think this is okay if it has no aliases, due to other recent changes
collection_name: 'awx.awx' | ||
version: '4.0.0' | ||
why: Collection name change | ||
alternatives: 'TOWER_OAUTH_TOKEN, AAP_TOKEN' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same with this. Not sure how to handle this so it is clear that OAUTH is coming from the Resource Server but is not available in Controller
SUMMARY
Update the injectors for the AAP credential type to work across collections
ISSUE TYPE
COMPONENT NAME
AWX VERSION
ADDITIONAL INFORMATION
Related PR's: