Provide a way to customize mutation "success" annotation

mjnagel commented 8 months ago

Is your feature request related to a problem? Please describe.

Currently pepr will add an annotation upon successful mutation of a k8s object. This happens via updateStatus and is hardcoded to ${config.uuid}.pepr.dev/${name}: succeeded when something is successful. We would like a way to customize the key and/or value of this annotation to our unique mutations, and ideally have the key/value be unique per mutation in our capability.

Describe the solution you'd like

Unsure of what would make the most sense but I would like options to either:

opt out of the pepr mutation annotation (in favor of my own)
some way to pass in configuration to the annotation (ex: pass in a string about the mutation instead of succeeded)

I'm not very familiar with the pepr code surrounding this, but wonder if it would be possible to set something in the request or elsewhere to tell pepr what to add.

Describe alternatives you've considered

We could add an additional mutation of our own, which is very viable, but the default mutation would still be added so it could be duplicative.

cmwylie19 commented 8 months ago

@mjnagel We currently are using this key for tests and documentation to ensure a given object has been seen and modified by Pepr as a trouble shooting breadcrumb.

Curious as to what pain point would occur if you added your own annotation and the succeeded annotation?

On hold for now due to the usage in our e2e tests. we will revisit if this becomes a priority for ya

mjnagel commented 8 months ago

There's nothing wrong with that path for sure. We are currently doing that over on uds-core and end up with pods looking like this:

uds-core.pepr.dev/mutated: drop-all-capabilities
uds-core.pepr.dev/uds-core-policies: succeeded

There's definitely nothing wrong with this - basically we just want to add more details on where/why the mutation originated. If we should just stay the course and add our own annotation, happy to close this out. I think from my perspective it was a bit unclear where/why the succeeded annotation got added and we wanted something with more info for the end user to see what happened (i.e. this was mutated by a specific policy, then they can check what that policy does).

cmwylie19 commented 8 months ago

There's nothing wrong with that path for sure. We are currently doing that over on uds-core and end up with pods looking like this:
uds-core.pepr.dev/mutated: drop-all-capabilities
uds-core.pepr.dev/uds-core-policies: succeeded
There's definitely nothing wrong with this - basically we just want to add more details on where/why the mutation originated. If we should just stay the course and add our own annotation, happy to close this out. I think from my perspective it was a bit unclear where/why the succeeded annotation got added and we wanted something with more info for the end user to see what happened (i.e. this was mutated by a specific policy, then they can check what that policy does).

Makes a lot of sense what you are saying. For specific cases where the goal is to understand which callback generates said mutation it makes a lot of sense to bake your own with [request].SetAnnotation("this_callback","succeeded"). Currently we would recommend this approach instead of baking it into the core of Pepr since:

this feels like "module" responsibility
we still want a standard way to know where a certain module has annotated for helping debug and testing

Do you think we should make the docs more clear on where that annotation has come from? We can keep the conversation open around this if you would like.

defenseunicorns / pepr