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

Move injectors to ManagedCredentialType #57

Merged

Conversation

chrismeyersfsu
Copy link
Member

Injectors are a bail-out mechanism for when you need to do something that the templating engine does not support, for a particular credential type. They are strongly tied to one and only one credential type. Before this change they lived far away from the ManagedCredentialType they are associated for. The link was the credential kind string was the same as the injector function name. This was a very loose coupling. This change is a tighter coupling.

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.

Putting this in my own words...

The custom logic like awx_plugins.credentials.injectors.aws already lived here, but there was no link in the plugin definition to the custom logic. So somehow that got referenced by a literal name like "aws".

My only gripe would be that post_injectors doesn't feel like a great name. Like, sure, maybe it runs after the other stuff (templates)? But it's usually an alternative to the templates, making that detail kind of irrelevant practically. It's more important that it runs custom python logic like a caveman, as opposed to any structured injector definition.

@chrismeyersfsu
Copy link
Member Author

  1. injectors_override
  2. custom_injectors
  3. alt_injectors
  4. injectors_fn

@AlanCoding
Copy link
Member

I'd lean towards (2) myself.

@webknjaz
Copy link
Member

webknjaz commented Nov 15, 2024

I had an alternative idea with ansible/awx_plugins.interfaces#10 / #50 / ansible/awx#15595 FTR

@chrismeyersfsu chrismeyersfsu force-pushed the AAP-35749-move-inject-credential branch 3 times, most recently from ce238b7 to 888f5b4 Compare November 18, 2024 17:58
@AlanCoding
Copy link
Member

I slightly lean towards the sentiment @chrismeyersfsu expressed in ansible/awx#15595

So I lean more towards getting rid of this "problem" by bringing these custom injectors closer to the ManagedCredentialType they are related to.

But even if we did nothing, it wouldn't bother me that much, since they already live in the plugins repo.

@chrismeyersfsu chrismeyersfsu force-pushed the AAP-35749-move-inject-credential branch 2 times, most recently from 037a5c9 to e56d661 Compare November 19, 2024 10:32
@webknjaz
Copy link
Member

webknjaz commented Nov 19, 2024

@chrismeyersfsu typing change handling instructions:

  1. Merge Adopt the custom_injectors callback awx_plugins.interfaces#13
  2. Click Run workflow @ https://github.com/ansible/awx_plugins.interfaces/actions/workflows/ci-cd.yml, entering 0.0.1a2 precisely in the version field. Don't enter anything else and submit the form.
  3. Wait until the newly created workflow run completes tests and pauses, then approve releasing on the workflow run summary page.
  4. Wait until the workflow run completes fully.
  5. Click Run workflow @ https://github.com/ansible/awx-plugins/actions/workflows/pip-tools.yml, entering awx_plugins.interfaces in the only input field.
  6. Wait until that workflow completes and opens a PR with the version bump in the lock files.
  7. Go to that PR and undraft it. You can approve it and enable auto-merge at this point.
  8. Once that PR is merged, you can rebase this one.

@chrismeyersfsu chrismeyersfsu force-pushed the AAP-35749-move-inject-credential branch from 76512fe to 8b22499 Compare December 2, 2024 12:39
@chrismeyersfsu chrismeyersfsu force-pushed the AAP-35749-move-inject-credential branch from 8b22499 to e71e0bd Compare December 2, 2024 21:14
@chrismeyersfsu chrismeyersfsu force-pushed the AAP-35749-move-inject-credential branch 2 times, most recently from 5e3dea9 to 576d32d Compare December 2, 2024 21:46
.pre-commit-config.yaml Outdated Show resolved Hide resolved
docs/conf.py Show resolved Hide resolved
Injectors are a bail-out mechanism for when you need to do something
that the templating engine does not support, for a particular credential
type. They are strongly tied to one and only one credential type. Before
this change they lived far away from the ManagedCredentialType they are
associated for. The link was the credential kind string was the same as
the injector function name. This was a very loose coupling. This change
is a tighter coupling.
* Defining an injector as code is an override. The previous name made it
  sound like the template engine would run, then the custom injector.
  This is not the case. If the custom injector is defined then the
  templating engine doens't run.
* ManagedCredentialType custom callback functions now use a proper
  Credential class defined in awx_plugins.interfaces
* I am not sure why the docs can not find the Credential class. Feels
  like maybe I'm pointing at a stale awx_plugins.interfaces. Also kind
  of feels like sphinx is confused by this repo being named awx_plugins
  and there being another repo/package with a similarish name so it is
  searching ansible/awx_plugins instead of
  ansible/awx_plugins.interfaces
@chrismeyersfsu chrismeyersfsu force-pushed the AAP-35749-move-inject-credential branch 2 times, most recently from 66d9ed3 to 2663734 Compare December 3, 2024 18:26
@chrismeyersfsu chrismeyersfsu force-pushed the AAP-35749-move-inject-credential branch from 2663734 to 7f6ba7e Compare December 3, 2024 18:43
@chrismeyersfsu chrismeyersfsu added this pull request to the merge queue Dec 3, 2024
Merged via the queue into ansible:devel with commit b5350e2 Dec 3, 2024
27 checks passed
@chrismeyersfsu chrismeyersfsu deleted the AAP-35749-move-inject-credential branch December 3, 2024 19:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants