fix(rollout-restart): adding field manager vault-secrets-operator to the restartAt annotation #1016
Conversation
…the restartAt annotation
|
|
|
Thank you for your submission! We require that all contributors sign our Contributor License Agreement ("CLA") before we can accept the contribution. Read and sign the agreement Learn more about why HashiCorp requires a CLA and what the CLA includes Have you signed the CLA already but the status is still pending? Recheck it. |
…ignore argocd rollout
helpers/rollout_restart_test.go
Outdated
| case *appsv1.DaemonSet: | ||
| restartAt = o.Spec.Template.ObjectMeta.Annotations[AnnotationRestartedAt] | ||
| assertFieldManagerSet(t, obj, FieldManagerName) |
There was a problem hiding this comment.
For completeness, I wonder if we should assert that the field manager was not added in the ArgoRollout case below?
There was a problem hiding this comment.
Thanks for the initial pass @benashz. Actually i discovered the test itself is not testing anything regarding the field manager. Because its passed in as an option to the client.Patch function.
Im not super familiar with the kubernetes client; it seems to be a fake implementation of the actual Kubernetes client. Do you have an example of how to inspect the request potentially to make sure the options are really added to the request? I guess in other apps i would use a mock of the client and expect
As for the test for making sure its not applied to the argo rollout, makes complete sense. Will do it when i figure out how to actually inspect the options ahaha.
There was a problem hiding this comment.
I think i found an example using the interceptor functions. Will be able to look at implementing in the morning.
Is this kind of what you would expect?
https://github.com/kubernetes-sigs/controller-runtime/blob/main/pkg/client/fieldowner_test.go#L78
| default: | ||
| return fmt.Errorf("unsupported type %T for rollout-restart patching", t) | ||
| } | ||
|
|
||
| return client.Patch(ctx, obj, patch, patchOptions...) |
There was a problem hiding this comment.
Previously, we were returning from each case. I am not opposed to this refactoring but we should guard against a nil patch here.
helpers/rollout_restart_test.go
Outdated
| @@ -185,3 +188,14 @@ func assertPatchedRolloutRestartObj(t *testing.T, ctx context.Context, obj ctrlc | |||
| "restartAt should be after beforeRolloutRestart", | |||
| attr, restartAtTime, "beforeRolloutRestart", beforeRolloutRestart) | |||
| } | |||
|
|
|||
| func assertFieldManagerSet(t *testing.T, obj ctrlclient.Object, manager string) { | |||
| found := false | |||
There was a problem hiding this comment.
| found := false | |
| t.Helper() | |
| found := false |
|
After double-checking the installation of VSO in our development environment (0.7.1). It seems it's already adding the FieldManager for the kubectl get deployment -n deseb --show-managed-fields -o json my-test-app | jq '.metadata.managedFields[] | select(.manager == "vault-secrets-operator")'
{
"apiVersion": "apps/v1",
"fieldsType": "FieldsV1",
"fieldsV1": {
"f:spec": {
"f:template": {
"f:metadata": {
"f:annotations": {
".": {},
"f:vso.secrets.hashicorp.com/restartedAt": {}
}
}
}
}
},
"manager": "vault-secrets-operator",
"operation": "Update",
"time": "2025-02-21T11:47:17Z"
}Would you be able to double-check on your side @benashz if you already see the field, too? From my limited experience with the controller-runtime client, I thought the only way to add that was using the the So potentially, this PR this can just be closed alongside with #921. |
Should fix #921