feat: Update collection arguments

[sean-m-sullivan]

SUMMARY

Update the injectors for the AAP credential type to work across collections

ISSUE TYPE

• New or Enhanced Feature

COMPONENT NAME

• Collection

AWX VERSION

24.6.1

ADDITIONAL INFORMATION

Related PR's:

• awx-plugins
• ansible.platform
• ansible.hub
• ansible.controller
• ansible.eda
• eda-server
• infra.aap_configuration - Done.

[AlanCoding]

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/

[sean-m-sullivan]

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.

[AlanCoding]

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.

[AlanCoding]

I'm actually not convinced this will work anymore. I expect that if the collection receives AAP_HOSTNAME then in that scenario specifically, it needs to append to use /api/controller/ as opposed to /api/. I don't see that.

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.

[AlanCoding]

ansible/event-driven-ansible#347

The PR you created here is for the collection. There may be a needed change to eda-server as well.

https://github.com/ansible/eda-server/blob/ac4be90e413196e922bd255a04589711b25b818e/src/aap_eda/core/management/commands/create_initial_data.py#L302-L315

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.

[sean-m-sullivan]

It may be needed, didn't know it was there will look and add that to the list

ansible/event-driven-ansible#347

The PR you created here is for the collection. There may be a needed change to eda-server as well.

https://github.com/ansible/eda-server/blob/ac4be90e413196e922bd255a04589711b25b818e/src/aap_eda/core/management/commands/create_initial_data.py#L302-L315

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.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[PabloHiro]

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

[AlanCoding]

Here is a patch to hopefully fix the sanity failures.

sean-m-sullivan#60

Checks do have trouble running on forks, so we may not get meaningful output from that until merged here. I guess I can test separately... #15729

[AlanCoding]

I need to formally document the situation with tokens here. These lines of code were removed in recent changes:

3ba6e2e#diff-87795928c0b98b89601c8cc0bb837dd70788e1a0c7c5803a6bf7a695bc02984dL505-L507

        if self.oauth_token:
            # If we have a oauth token, we just use a bearer header
            headers['Authorization'] = 'Bearer {0}'.format(self.oauth_token)

Changing how we parse the token argument is ignoring the problem. With no code that constructs the headers to use a token, you can't make requests using a token.

For now, I would say kick the token out of scope of this, and an issue can be filed for that. Current reading is that all params related to OAuth2 tokens here are non-functional and that's the blocker. So the simple change I'm asking for is to remove the token params.

[AlanCoding]

For sanity test failures, I've now realized (after banging my head on my desk for a bit) that the following failure is introduced by this PR.

ERROR: Found 2 pep8 issue(s) which need to be resolved:
ERROR: plugins/module_utils/controller_api.py:80:14: E131: continuation line unaligned for hanging indent
ERROR: plugins/module_utils/controller_api.py:81:14: E131: continuation line unaligned for hanging indent

The patch I linked (my PR in your fork) is still valid. This is yet another fix that's needed for sanity to pass.

[AlanCoding]

For yet another request here, it was agreed that we need some measure of testing of the new params. This is my proposed solution. It can be expanded to some other params, but cannot be expanded realistically to test env vars (maybe? you could try)

sean-m-sullivan#61

I think that will pass, so this (along with my other PR 60) should hopefully be easy inclusions without breaking checks.

To recap the full work list here:

• fork PR 60 - 1 of 2 sanity test fixes
• 2 of 2 sanity test fixes (from pep8)
• fork PR 61 - add some very basic coverage for the updated params (that values given take effect)
• remove all the params about using tokens, because they don't work

And I hope all that should cover it to get this ready to merged and moved along.