Permission claim fixes / cleanups #1745
Conversation
openshift-ci
bot
added
the
kind/api-change
| for k, v := range expectedLabels { | ||
| if labels == nil { |
There was a problem hiding this comment.
Why do this inside the loop instead of at line 96?
There was a problem hiding this comment.
because only here it is needed
| u.SetLabels(labels) | ||
|
|
||
| return nil | ||
|
|
||
| } | ||
|
|
||
| func (m *mutatingPermissionClaims) Validate(ctx context.Context, a admission.Attributes, o admission.ObjectInterfaces) error { | ||
| if a.GetSubresource() != "" { |
There was a problem hiding this comment.
Is it always the case that we don't want to validate the labels the subresource?
I am unaware of a subresource that will let you change metadata, but I thought I would ask.
There was a problem hiding this comment.
as admission is best effort in our case this should be safe, even if some day some other subresource is added that allows it (I doubt it).
| labels[k] = v | ||
| } | ||
|
|
||
| u.SetLabels(labels) | ||
|
|
||
| return nil | ||
|
|
||
| } | ||
|
|
||
| func (m *mutatingPermissionClaims) Validate(ctx context.Context, a admission.Attributes, o admission.ObjectInterfaces) error { |
There was a problem hiding this comment.
Pretty sure we can remove this function entirely, given that Admit sets everything we're validating here. Right?
| labels := obj.GetLabels() | ||
| if labels == nil { | ||
| labels = make(map[string]string) | ||
| } |
There was a problem hiding this comment.
background for this commit: it is not good to depend on nil-vs-empty behaviour of LabelsFor.
|
/lgtm |
|
/approve |
|
[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 |
openshift-ci
bot
added
the
approved
Extracted from https://github.com/kcp-dev/kcp/pull/1563/commits.