Extract GVK more robustly when building applyset parent #368
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: justinsb The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
approved
|
Thank you so much for the quick fix, @justinsb ! I made a local copy of your change and verified the issue in the CC reconciliation is fixed. /lgtm |
|
@maqiuyujoyce: changing LGTM is restricted to collaborators In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
| if err != nil { | ||
| return statusInfo, err | ||
| return nil, fmt.Errorf("getting GVK for %T: %w", instance, err) |
There was a problem hiding this comment.
So this appears to be us returning an error we were never returning (ignoring) before. I believe there is also a difference between object.GetObjectKind().GroupVersionKind() and apiutil.GVKForObject(instance, r.mgr.GetScheme()) such that we things which were not errors for the former (eg 0 length kind or versions) which are errors for the latter.
There was a problem hiding this comment.
client.Client uses apiutil.GVKForObject (IIRC), so we really should have been using that all along. We got away with it up until now because the cached client seems to set the GVK in the object. But when the KCC operator turned off the cache, the GVK was no longer set (possibly a bug in controller-runtime), and the difference mattered.
We also blew up immediately in the existing code if the GVK was empty. The first thing NewParentRef does is try to map the GVK to a resource, that gave an error which we weren't ignoring. By returning the error with the verbose message here, we hopefully make it easier to understand what's going wrong (and make it less likely that something is going wrong anyway, as we probably don't make it to this point if we can't make the object to a GVK!)
I did fix from returning nil, fmt.Errorf... to returning statusInfo, fmt.Errorf... ; mostly for consistency but it is more correct also.
There was a problem hiding this comment.
Just to add some data points: During the upgrade of KDP from v0.13.0-beta to v0.15.0-beta, KCC had to deal with 12+/20+ (depending on how they are counted) breaking changes. Not saying that it is a good thing to have breaking changes, though if it is a fix and is potentially breaking, I think it seems to be aligning with the philosophy of KDP's minor version bump.
There was a problem hiding this comment.
@maqiuyujoyce I think the majority of those breakages are due to controller-runtime; we are actually starting to build a compatibility layer into kdp to try to smooth these transitions (commonclient). This one though is a bug/regression, although we are also switching the apply method which is why we hit it (and that should be opt-in).
Include the case when we're using a typed object.
justinsb
force-pushed
the
extract_gvk_more_robustly
branch
from
92ca451
to
94acf17
Compare
|
/lgtm |
k8s-ci-robot
added
the
lgtm
…elease-0.15 Automated cherry pick of #368: Extract GVK more robustly when building applyset parent
We are aiming to pick up kubernetes-sigs/kubebuilder-declarative-pattern#368
We are aiming to pick up kubernetes-sigs/kubebuilder-declarative-pattern#368
Include the case when we're using a typed object.