ConfigMapPropagation should keep other owner references when abandoning a copy configmap#2492
Conversation
knative-prow-robot
added
the
size/M
| if owner.UID == cmp.UID { | ||
| index = i | ||
| break | ||
| } |
There was a problem hiding this comment.
I only find this way to delete an owner reference (by iterating the owner reference array). Not sure if there is other better way to do this.
There was a problem hiding this comment.
I would move this to a method in the controller removeOwnerReference so it's easier to read.
Rather than modifying the array in place, it might be simpler to copy each element to a new array, skipping the ref to remove. Code readability is more important than perf here IMO.
There was a problem hiding this comment.
That make sense, I just changed it. Thank you.
| } | ||
|
|
||
| // removeOwnerReference removes the target ownerReference and returns a new slice of ownerReferences. | ||
| func (r *Reconciler) removeOwnerReference(ownerReferences []metav1.OwnerReference, uid types.UID) []metav1.OwnerReference { |
There was a problem hiding this comment.
No need for this to be a method. Since it doesn't use r it can just be a package-level function.
| var expected []metav1.OwnerReference | ||
| for _, owner := range ownerReferences { | ||
| if owner.UID != uid { | ||
| expected = append(expected, *owner.DeepCopy()) |
There was a problem hiding this comment.
Since the object itself isn't being modified, just the slice, I'd expect append(expected, owner) to work fine here. Why use DeepCopy?
|
The following is the coverage report on the affected files.
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: grac3gao, grantr The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
knative-prow-robot
added
the
approved
6 participants
Fixes #2437
Proposed Changes
Release Note