-
Notifications
You must be signed in to change notification settings - Fork 259
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Enable DV status subresource #2611
Conversation
2c8ce43
to
b6b2774
Compare
b6b2774
to
f2b9883
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good to me, just one minor comment.
@@ -825,7 +823,7 @@ func (r *ReconcilerBase) emitEvent(dataVolume *cdiv1.DataVolume, dataVolumeCopy | |||
} | |||
// Only update the object if something actually changed in the status. | |||
if !reflect.DeepEqual(dataVolume.Status, dataVolumeCopy.Status) { | |||
if err := r.updateDataVolume(dataVolumeCopy); err != nil { | |||
if err := r.updateDataVolumeStatus(dataVolumeCopy); err != nil { | |||
r.log.Error(err, "Unable to update datavolume", "name", dataVolumeCopy.Name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe change the error to unable to update datavolume status
?
e63d11e
to
17d44f0
Compare
DV status can now be updated only by UpdateStatus() and no more by Update(), so regular users cannot update status without special permission. In addition, updating just the status would not update the generation of the object, so we’ll have less “object has been modified” errors in the reconcile loops. Signed-off-by: Arnon Gilboa <agilboa@redhat.com>
17d44f0
to
3317780
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice!
/lgtm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
/approve Now let's do this with our other CRDs! |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mhenriks 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 |
/cherrypick release-v1.56 |
@arnongilboa: new pull request created: #2626 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. |
What this PR does / why we need it:
DataVolume.Status
can now be updated only byUpdateStatus()
and no more byUpdate()
, so regular users cannot updateStatus
without special permission. In addition, updating just theStatus
would not update theGeneration
of the object, so we’ll have lessobject has been modified
errors in the reconcile loops.Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close the issue(s) when PR gets merged):Fixes #
Special notes for your reviewer:
Release note: