-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
✨ Allow admission responses to send warnings #1157
✨ Allow admission responses to send warnings #1157
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.
/approve
/assign @alvaroaleman @DirectXMan12
/milestone v0.7.x
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: JoelSpeed, vincepri 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 |
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.
minor comment inline, generally looks good
pkg/webhook/admission/response.go
Outdated
|
||
// WithWarnings adds the given warnings to the Response. | ||
// If any warnings were already given, they will not be overwritten. | ||
func (r Response) WithWarnings(warnings []string) Response { |
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.
variadic would have better ergonomics here, I believe
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.
Oh yeah, that's a great suggestion! Will update
/hold until I can address Solly's suggestion |
51861ea
to
786a866
Compare
/hold cancel @DirectXMan12 I've updated to use variadic arguments based on your suggestion, PTAL |
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.
/lgtm
/retest |
Warnings have been introduced with K8s 1.19. This allows people who are running webhooks to send a warning message if there is something wrong with a resource (eg not following best practice) that is not a fatal error.
This PR adds a
WithWarnings
helper function that allows you to extend a response with warnings in an easy way. For example:Not sure if this is a majorly required helper since people could easily write one themselves, though I thought it could be a useful helper and it only took five minutes to prep, so thought I'd raise the code for discussion, let me know what y'all think.