UPSTREAM: 57214: Remove mutation from pvc validation#17876
UPSTREAM: 57214: Remove mutation from pvc validation#17876openshift-merge-robot merged 2 commits intoopenshift:masterfrom
Conversation
openshift-ci-robot
added
the
approved
openshift-ci-robot
added
the
size/XXL
deads2k
added
queue/critical-fix
and removed
approved
openshift-merge-robot
added
the
vendor-update
| func ValidatePersistentVolumeClaimUpdate(newPvc, oldPvc *core.PersistentVolumeClaim) field.ErrorList { | ||
| allErrs := ValidateObjectMetaUpdate(&newPvc.ObjectMeta, &oldPvc.ObjectMeta, field.NewPath("metadata")) | ||
| allErrs = append(allErrs, ValidatePersistentVolumeClaim(newPvc)...) | ||
| newPvcClone := newPvc.DeepCopy() |
There was a problem hiding this comment.
fix lgtm. Would even add a big red warning sign and a 😱 .
There was a problem hiding this comment.
nit: do not even call them *Clone, better shadow them. Avoid that somebody chooses the wrong PVC by accident.
There was a problem hiding this comment.
ack, I think that is sensible I wrote upstream code. :((
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: deads2k, sttts The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
openshift-ci-robot
added
the
approved
| } | ||
|
|
||
| oldSize := oldPvc.Spec.Resources.Requests["storage"] | ||
| newSize := newPvc.Spec.Resources.Requests["storage"] |
There was a problem hiding this comment.
this is still so much spaghetti here that newSize is the value without the mutation above.
There was a problem hiding this comment.
hrm, why would we pick newSize from mutated spec? because that will simply return old size...
There was a problem hiding this comment.
The logic in the code is correct. But it is hard to read when you have the original and the clone in the context.
There was a problem hiding this comment.
at least call it originalNewSize or something explicit like that.
This is a pick of an already merged pull upstream. I'm taking this to be good enough to tag. |
|
/retest |
1 similar comment
|
/retest |
|
Automatic merge from submit-queue. |
5 participants
fixes #17769
/assign gnufied
/assign sttts