OCPBUGS-6016: UpdateStrategy RegistryPoll with nil Interval #2920
Conversation
openshift-ci-robot
added
jira/severity-low
|
@dtfranz: This pull request references Jira Issue OCPBUGS-6016, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. 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. |
openshift-ci-robot
added
the
jira/invalid-bug
|
/jira refresh |
openshift-ci-robot
added
jira/valid-bug
|
@dtfranz: This pull request references Jira Issue OCPBUGS-6016, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
No GitHub users were found matching the public email listed for the QA contact in Jira ([email protected]), skipping review request. 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. |
dtfranz
force-pushed
the
update-interval-panic
branch
from
2f1101e to
ad7dec8
Compare
| return | ||
| } else if out.Spec.UpdateStrategy.RegistryPoll.ParsingError != "" && out.Status.Reason != v1alpha1.CatalogSourceIntervalInvalidError { |
There was a problem hiding this comment.
Nit. else not necessary as the if returns.
There was a problem hiding this comment.
I did this intentionally to avoid the newline and keep this as one error handling if/else block. I'm happy to separate them though if it doesn't look right.
There was a problem hiding this comment.
Other projects (e.g. OpenSSL) would recommend not having this, due to the prior return, regardless of lines. Mostly because it's clear that these if statements really stand apart from each other.
There was a problem hiding this comment.
👍 makes sense, I've updated it to separate them out.
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dtfranz, perdasilva 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
dtfranz
force-pushed
the
update-interval-panic
branch
from
ad7dec8 to
084bde3
Compare
Adds protection against a nil-pointer panic when an UpdateStrategy with nil value for RegistryPoll Interval is supplied. Also adds a unit test to ensure that the code does not panic but instead returns an error when this situation is encountered. Signed-off-by: Daniel Franz <[email protected]>
dtfranz
force-pushed
the
update-interval-panic
branch
from
084bde3 to
b71e6a3
Compare
|
/lgtm |
|
@dtfranz: All pull requests linked via external trackers have merged: Jira Issue OCPBUGS-6016 has been moved to the MODIFIED state. 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. |
Description of the change:
Adds protection against a nil-pointer panic when an UpdateStrategy with nil value for RegistryPoll Interval is supplied. Also adds a unit test to ensure that the code does not panic but instead returns an error when this situation is encountered.
Motivation for the change:
OCPBUGS-6016