-
Notifications
You must be signed in to change notification settings - Fork 456
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
Enhance error msg when trying to change .spec.seedName
by patching a shoot
#9294
Enhance error msg when trying to change .spec.seedName
by patching a shoot
#9294
Conversation
@@ -761,7 +761,7 @@ var _ = Describe("validator", func() { | |||
err := admissionHandler.Admit(ctx, attrs, nil) | |||
|
|||
Expect(err).To(BeForbiddenError()) | |||
Expect(err).To(MatchError(ContainSubstring("spec.seedName cannot be changed by patching the shoot, please use the shoots/binding subresource"))) | |||
Expect(err).To(MatchError(ContainSubstring("spec.seedName 'seed' cannot be changed to 'nil' by patching the shoot, please use the shoots/binding subresource"))) |
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.
seedName cannot be set to nil once set, right? So maybe we print that it cannot be nil at all, even with the binding resource.
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.
I was wondering about that as well, but decided to leave that error for the binding
subresource validation so as to keep the current "behaviour".
Otherwise I could potentially print two errors depending on whether the new .spec.seedName
is nil or not:
When it is not nil:
spec.seedName 'seed-name' cannot be changed to 'new-seed' by patching the shoot, please use the shoots/binding subresource
When it is nil:
spec.seedName is already set to `seed-name` and cannot be changed to 'nil'
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.
I would prefer the two error approach.
In binding, we print similar error for the nil case but a different message, you could use this error there too.
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.
Done
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.
/approve
71a4a83
to
d073970
Compare
d073970
to
941fdb9
Compare
941fdb9
to
a71b6bb
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.
Thanks!
/lgtm
LGTM label has been added. Git tree hash: 1327bb09392b5417993cb02a578923ec9c5e648a
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ScheererJ, shafeeqes 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 |
/retest |
/test pull-gardener-e2e-kind-ha-single-zone |
How to categorize this PR?
/area usability
/kind enhancement
What this PR does / why we need it:
This PR enhances the validation error which is shown when trying to change a shoot's
.spec.seedName
without using theshoots/binding
subresource. The new error message will also display the old and new.spec.seedName
s to show that a change attempt was actually made. If one of the.spec.seedName
fields arenil
it will print 'nil' in the error message. You can check the following examples:Which issue(s) this PR fixes:
Fixes #9186
Special notes for your reviewer:
Release note: