Move controller revision to annotations #1234
Conversation
| ) | ||
|
|
||
| // ControllerVersionAnnotation is the annotation name that indicates the last controller version to update a resource | ||
| const ControllerVersionAnnotation = "k8s.elastic.co/controller-version" |
There was a problem hiding this comment.
I think we have common.k8s.elastic.co used in other places, maybe use it here as well?
There was a problem hiding this comment.
I'm good with that
| func UpdateControllerVersion(client k8s.Client, obj runtime.Object, version string) error { | ||
| metaObject, err := meta.Accessor(obj) | ||
| if err != nil { | ||
| log.Error(err, "error converting runtime object to metav1 object") |
There was a problem hiding this comment.
Maybe log obj.GetObjectKind().GroupVersionKind().Kind?
|
|
||
| annotations[ControllerVersionAnnotation] = version | ||
| accessor := meta.NewAccessor() | ||
| err = accessor.SetAnnotations(obj, annotations) |
There was a problem hiding this comment.
Can we not reuse metaObject.SetAnnotations here, instead of creating a new accessor?
There was a problem hiding this comment.
I don't think so because accessor's SetAnnotations(obj, annotations) returns a runtime.Object, which is what we need for the Update call, while metaObject.SetAnnotations() modifies the metav1.Object receiver.
But your question did make me realize that I didn't look closely enough at the funcs available to the metav1 Accessor so there's some small bit of refactoring I can do here
There was a problem hiding this comment.
metaObject.SetAnnotations() modifies the metav1.Object receiver
I think it also modifies the original object you passed to it, which you can the use for the update call.
But I'm also fine with the way you used it here :)
There was a problem hiding this comment.
Ah that was not my understanding just from scanning through the code but it's deeeeefinitely possible I am misunderstanding
| } | ||
|
|
||
| // setupScheme creates a scheme to use for our fake clients so they know about our custom resources | ||
| // TODO move this into one of the upper level common packages and make public, refactor out this code that's in a lot of our tests |
There was a problem hiding this comment.
+1, can you create an issue for that one?
There was a problem hiding this comment.
Technically you only need to register the Kibana types here?
There was a problem hiding this comment.
Yeah that's true @pebrc, wanted to make it easy to just copy it later
| } | ||
|
|
||
| // do not send extraneous update if the value would not change | ||
| if annotations[ControllerVersionAnnotation] == version { |
There was a problem hiding this comment.
Should this not also return if there is any version set?
There was a problem hiding this comment.
I thought we would want to use this func because the caller decided that we do want to proceed with an update -- for instance it might be okay with going from 0.9.0 to 0.9.1. We might need additional funcs before calling this
Agreed that's something we need to think about but think that might be out of the scope of this issue. |
|
Jenkins test this please |
|
Jenkins test this please |
Reverts most of #1224 and relates to #1137
Moves the controller revision to last update the CR to the metadata rather than the status. I'm open to having it wherever, this is just a first pass.
If we do want to use this field for control flow we will need to do it before we enter the inner reconcile flow (since this update occurs right before we do that), or change where this func is called.
Also looked like our dep lock file had fallen out of date so I ran
dep ensure. Might be worth adding a check for that to CI, but we can also just punt until go modules.