-
Notifications
You must be signed in to change notification settings - Fork 254
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
Fix DIC returned reconcile.Result for ImageStream #2823
Conversation
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.
Not insisting just a thought
res, err := r.pollImageStreamDigest(ctx, dataImportCron) | ||
// We use the poll returned reconcile.Result for RequeueAfter if needed | ||
// res is the func returned result, so it must not be block-scoped | ||
res, err = r.pollImageStreamDigest(ctx, dataImportCron) |
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.
My concern is that this is so delicate we may step on it again in the future,
maybe we should decide whether requeue happens more explicitly?
BTW the reasoning for this was to get a uniform way of assigning errors in this module (= to :=) IIRC
instead of mixing them up
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.
Rewrited the code a bit. I think it's enough.
regression introduced in kubevirt#2700 Signed-off-by: Arnon Gilboa <agilboa@redhat.com>
d8c4d5f
to
627b1f1
Compare
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: akalenyu 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 |
if isImageStreamSource(dataImportCron) && dataImportCron.Spec.Schedule != "" { | ||
res, err := r.pollImageStreamDigest(ctx, dataImportCron) | ||
// We use the poll returned reconcile.Result for RequeueAfter if needed | ||
pollRes, err := r.pollImageStreamDigest(ctx, dataImportCron) |
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.
Would it be simpler to do
res, err = r.pollImageStreamDigest(ctx, dataImportCron)
if err != nil {
return res, err
}
Note the subtle change from :=
to =
which essentially assigns res to the result of pollImageStreamDigest. Which is logically the same as all the code here.
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.
This was the original solution but my concern was that we'd trip over this again
#2823 (comment)
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.
Apparently this was the original solution, but not explicit enough with regards to how res and err are assigned. I have no issues with either approach honestly.
/cherrypick release-v1.57 |
@arnongilboa: once the present PR merges, I will cherry-pick it on top of release-v1.57 in a new PR and assign it to you. 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. |
/retest |
2 similar comments
/retest |
/retest |
@arnongilboa: new pull request created: #2826 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:
Regression introduced in #2700
Which issue(s) this PR fixes:
Fixes bz #2226982
Special notes for your reviewer:
Release note: