Closed tomasaschan closed 1 year ago
Hi @tomasaschan. Thanks for your PR.
I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test
on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.
Once the patch is verified, the new status will be reflected by the ok-to-test
label.
I understand the commands that are listed here.
For full context, here's the simplification this PR would enable.
Without this PR, I have to do this:
return func(ctx context.Context, do declarative.DeclarativeObject, os *manifest.Objects) error {
errs := &multierror.Error{}
for _, o := range os.Items {
merged := make(map[string]string)
for k, v := range o.UnstructuredObject().GetAnnotations() {
merged[k] = v
}
for k, v := range annotationMaker(ctx, do) {
merged[k] = v
}
for k, v := range merged {
if err := o.SetNestedField(v, "metadata", "annotations", k); err != nil {
errs = multierror.Append(errs, err)
}
}
}
return errs.ErrorOrNil()
}
With this PR, the following is equivalent:
return func(ctx context.Context, do declarative.DeclarativeObject, os *manifest.Objects) error {
for _, o := range os.Items {
annotations := annotationMaker(ctx, do)
o.AddAnnotations(annotations)
}
return nil
}
Note that not only is the code simpler - by avoiding to use SetNestedField
, it's also safer!
/ok-to-test
And yes, we really shouldn't be exposing UnstructuredObject, because it doesn't clear the cached json.
Yeah, that's why I used SetNestedField
instead of just modifying the returned Unstructured
with e.g. AddAnnotations()
. Maybe it could be nice to instead offer some general function that can be used to modify the Unstructured
, but with proper cache behavior?
type Mutator func(*unstructured.Unstructured) error
func (o *objects.Object) Mutate(f Mutator) error {
if err := f(o.object); err != nil {
return err
}
o.json = nil
return nil
}
(which has the implicit contract that if an error is returned from the mutator, the object is not modified, but maybe that's OK...)
With something like that, anyone could implement arbitrary transforms like this without worrying about the json cache.
Maybe it could be nice to instead offer some general function...
Yes, I think that's an excellent idea. We could also just drop the cache, it's a relatively minor optimization. If we keep the cache, we should at least comment that mutating with the UnstructuredObject accessor is unsafe.
A nice thing about the mutator with the callback is that we could also support it on type Objects
(it would presumably visit each object in Items)
Thanks for the contribution @tomasaschan
/approve /lgtm
[APPROVALNOTIFIER] This PR is APPROVED
This pull-request has been approved by: justinsb, tomasaschan
The full list of commands accepted by this bot can be found here.
The pull request process is described here
Thanks for taking the time to review despite me submitting this on a Saturday!
I had a use case for setting annotations on all objects in an object transform, and found that while we do have
AddLabels
that does this, there's no correspondingAddAnnotations
.There is a way to do this without this PR, since the
manifest.Object
exposesUnstructuredObject()
to get at the underlyingunstructured.Unstructured
, but its docstring statesso I figured it's better to not ask users to take a dependency on that.