Fix updating invalid Instance parameters #1441
Conversation
Summary: in the past we've allowed updating Instance with parameters that are missing in the corresponding OV (since there were no way to forbid this in the absence of an admission controller). Now that we have instance admission controller in place, we can fix this behavior. Fixes #1411 Signed-off-by: Aleksey Dukhovniy <alex.dukhovniy@googlemail.com>
zen-dog
requested review from
alenkacz,
gerred,
kensipe and
nfnt
as code owners
Signed-off-by: Aleksey Dukhovniy <alex.dukhovniy@googlemail.com>
zen-dog
force-pushed
the
ad/updating-missing-params
branch
from
3cb6f37
to
7506140
Compare
Signed-off-by: Aleksey Dukhovniy <alex.dukhovniy@googlemail.com>
| @@ -299,15 +299,15 @@ func TestValidateUpdate(t *testing.T) { | |||
| wantErr: true, | |||
| }, | |||
| { | |||
| name: "parameter update triggering a non-existing OV plan IS allowed but will NOT trigger a plan", | |||
| name: "parameter update triggering a non-existing OV plan IS NOT allowed", | |||
There was a problem hiding this comment.
I think this change of behaviour deserves at least a mention in the PR description, if not title.
There was a problem hiding this comment.
It is mentioned in the PR description and somewhat vaguely in the PR title. Do you have a better title suggestion that doesn't exceed 60 characters?
There was a problem hiding this comment.
This test claims to be about "non-existent plans" while the PR title/description only seems to mention non-existent parameters. To me these are two different things.
See also #1268 w.r.t the former.
There was a problem hiding this comment.
You're right, plan description is misleading, it's the parameter existence that is being checked. Fixed the description.
There was a problem hiding this comment.
...and also added a test for the missing plan 👍
Signed-off-by: Aleksey Dukhovniy <alex.dukhovniy@googlemail.com>
Signed-off-by: Aleksey Dukhovniy <alex.dukhovniy@googlemail.com>
Summary:
in the past, we've allowed updating Instance with parameters that are missing in the corresponding OV (since there was no way to forbid this in the absence of an admission controller). Now that we have the instance admission controller in place, we can fix this behavior.
Fixes #1411
Signed-off-by: Aleksey Dukhovniy alex.dukhovniy@googlemail.com