apiserver: clarify watcher fanout deepcopy and self-link code #105700
Conversation
k8s-ci-robot
added
release-note-none
Originally added in https://github.com/kubernetes/kubernetes/pull/94397/files#diff-4519926a750520e1d319f4931197f61b4b9c277740f46e8f4b514bec746e2647R331, it does not look to be at the right place.
sttts
force-pushed
the
sttts-cacher-cleanups
branch
from
d8894ee
to
9107aac
Compare
k8s-ci-robot
added
size/M
k8s-ci-robot
removed
the
needs-rebase
|
/assign @wojtek-t |
k8s-ci-robot
removed
the
do-not-merge/contains-merge-commits
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: sttts 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 |
sttts
added
sig/api-machinery
k8s-ci-robot
added
the
approved
sttts
added
the
priority/important-soon
k8s-ci-robot
added
area/apiserver
and removed
needs-priority
| if err := setObjectSelfLink(ctx, result, req, scope.Namer); err != nil { | ||
| scope.err(err, w, req) | ||
| return | ||
| if !utilfeature.DefaultFeatureGate.Enabled(features.RemoveSelfLink) { |
There was a problem hiding this comment.
Actually - I'm fairly sure you can just remove "setObjectSelfLink" from here completely.
before (currently in line 241) you are calling transformResponseObject, which underneaths is calling transformObject which is setting selflink.
So I think it's not needed at all.
There was a problem hiding this comment.
only exception is metav1.Status which skips transformObject for whatever reason. Do we care?
There was a problem hiding this comment.
Hmm - I can't see how this can matter if we call it only in patch and not in created/update/get/...
If it matters - I think we should set it in other operations too, right?
So I think that we should set it elsewhere too or get rid from here for consistency...
There was a problem hiding this comment.
I'd rather not spend a lot of time trying to fix existing inconsistencies in self-link-adding, since that is disabled by default and is planned to be removed. Just honoring the feature gate here is easier to reason about.
There was a problem hiding this comment.
OK - I would like to propose it for 1.24 (we moved to Beta in 1.16, so I think we're either ready to do that now or will never be). I'm going to start a conversation for it on slack.
But with that - I agree this is probably a moot discussion.
sttts
force-pushed
the
sttts-cacher-cleanups
branch
from
9107aac
to
506e54c
Compare
k8s-ci-robot
added
the
cncf-cla: yes
|
/triage accepted |
k8s-ci-robot
added
triage/accepted
|
/assign @liggitt |
| // Ensure that for empty lists we don't return <nil> items. | ||
| if meta.IsListType(obj) && meta.LenList(obj) == 0 { | ||
| if err := meta.SetList(obj, []runtime.Object{}); err != nil { | ||
| return nil, err | ||
| } | ||
| } |
There was a problem hiding this comment.
off the top of my head, I can't think of a reason it would matter, but since SetList was conditionally done at the top of setObjectSelfLink, should we do that before calling setObjectSelfLink to be perfectly equivalent?
|
@sttts: PR needs rebase. 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. |
k8s-ci-robot
added
the
needs-rebase
|
Closing as it is mostly obsolete after #107527 got merged. |
Some pre-factoring for #104802 or for removal of the self-link code.
/kind cleanup