🐛 Handle a nil mutator by returning an error, not panicking #10951
Conversation
k8s-ci-robot
added
kind/bug
k8s-ci-robot
added
the
size/M
dlipovetsky
force-pushed
the
fix-applymutators-nil-mutator
branch
from
d8a84bf to
b6573a2
Compare
dlipovetsky
changed the title
The call is incorrect, but the test does not fail, because it enables dry-run, and that does not exercise the mutators.
dlipovetsky
force-pushed
the
fix-applymutators-nil-mutator
branch
from
b6573a2 to
e143580
Compare
dlipovetsky
changed the title
| @@ -2408,3 +2408,48 @@ func TestWaitReadyForMove(t *testing.T) { | |||
| }) | |||
| } | |||
| } | |||
|
|
|||
| func Test_applyMutators(t *testing.T) { | |||
There was a problem hiding this comment.
Not a bad suggestion, but I'd rather not, because
(a) There's no measurable decrease in time for this specific test, and
(b) relatively few tests across the repo use it.
Address feedback
|
Thanks for reviewing, @sivchari! |
|
Thx! /lgtm Feel free to cherry-pick back further /cherry-pick release-1.8 |
|
@sbueringer: once the present PR merges, I will cherry-pick it on top of release-1.8 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-sigs/prow repository. |
k8s-ci-robot
added
the
lgtm
|
LGTM label has been added. Git tree hash: 432ec603f7bb8b278be9ba506c2b6c5b77b84ada
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: sbueringer 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 |
k8s-ci-robot
added
the
approved
|
@sbueringer: new pull request created: #10981 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-sigs/prow repository. |
What this PR does / why we need it:
I discovered that the
applyMutatorsfunction panics if one of the mutators is nil. I updated the function to return an error on any nil mutator.The first commit adds unit tests, one of which confirms that
applyMutatorspanics if a the mutators slice contains a nil mutator.The second commit fixes the issue.
The third commit fixes a test that passes a nil mutator. The call is incorrect, but the test has never failed, because it enables dry-run, and that does not exercise the mutators.
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 #
/kind bug
/area clusterctl