Create and apply patch when adding labels to API objects #687
Conversation
nkubala
requested review from
dgageot,
dlorenc,
r2d4 and
viglesiasce
as code owners
pkg/skaffold/runner/label/label.go
Outdated
| pods := client.CoreV1().Pods(retrieveNamespace(res.Namespace, p.ObjectMeta)) | ||
| _, err = pods.UpdateStatus(p) | ||
| apiObject := modifiedObj.(*corev1.Pod) | ||
| addSkaffoldLabels(labels, &apiObject.ObjectMeta) |
There was a problem hiding this comment.
can we just access the object meta directly? Even if its just assuming that its in the untyped yaml at root -> metadata
pkg/skaffold/runner/label/label.go
Outdated
| addSkaffoldLabels(labels, &apiObject.ObjectMeta) | ||
| modifiedJSON, _ := json.Marshal(modifiedObj) | ||
| p, _ := patch.CreateTwoWayMergePatch(originalJSON, modifiedJSON, apiObject) | ||
| _, err = client.AppsV1().ReplicaSets(retrieveNamespace(res.Namespace, apiObject.ObjectMeta)).Patch(apiObject.Name, types.StrategicMergePatchType, p) |
There was a problem hiding this comment.
What was the problem using the dynamic client here? I thought that was working?
There was a problem hiding this comment.
some of it works, but there are a few types that the version I'm using needs that are incompatible with other dependency versions we already have vendored. tl;dr it's a dep nightmare, I'll work through it eventually
pkg/skaffold/runner/label/label.go
Outdated
| addSkaffoldLabels(labels, &d.ObjectMeta) | ||
| deployments := client.AppsV1beta2().Deployments(retrieveNamespace(res.Namespace, d.ObjectMeta)) | ||
| _, err = deployments.Update(d) | ||
| apiObject := modifiedObj.(*appsv1beta2.Deployment) |
There was a problem hiding this comment.
At this point it might make sense to reduce this switch statement to lower the boilerplate. One idea is a map (or two maps) of type (appsv1beta2.Deployment) -> functions.
One function could be "apiObjectGetter", which does the type conversion, and the other could do the Patch itself.
Then this switch statement is a lookup of type to functions, then calls those functions. WDYT?
There was a problem hiding this comment.
yeah, I agree I should have addressed this before. just refactored it, I'll detail in the top-level comment
pkg/skaffold/runner/label/label.go
Outdated
| // objectMeta is responsible for returning a generic runtime.Object's metadata | ||
| type objectMeta func(runtime.Object) *metav1.ObjectMeta | ||
|
|
||
| var converters = map[objectType]converter{ |
There was a problem hiding this comment.
nit: could you collapse converter and objectmeta to one function that returns ok and objectmeta?
There was a problem hiding this comment.
yeah let me try that
This changes the labeling code to apply patches to kubernetes API objects instead of updating them. This should cause skaffold to correctly identify when an object has been modified, preventing it from recreating the objects on each deploy.
UPDATE: I went ahead and refactored out the unwieldy switch statement to remove most of the boilerplate. Instead, the main loop uses two maps of object types to functions that expose the necessary requirements to make the control flow generic. These maps are
objectMetas, used to retrieve a function that returns ametav1.ObjectMetapatchers, used to retrieve a function that uses the corresponding typed client to apply a patchStill fighting with the dynamic client, so I'll (hopefully) send that in another PR.
Fixes #681