-
Notifications
You must be signed in to change notification settings - Fork 78
Set up defaulting webhook #51
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
Conversation
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: danielvegamyhre 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 |
01c769f
to
8fdec93
Compare
8fdec93
to
4738f34
Compare
}{ | ||
{ | ||
name: "job completion mode is unset", | ||
js: &JobSet{ |
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 not able to use wrappers here because it would create a circular import dependency (testing package imports v1alpha1, so v1alpha1 cannot import testing).
We could consider a refactor to solve this (kueue uses a separate webhooks package and sets up a CustomerDefaulter, which allows for more flexibility, contextual logging, etc. but this would require creating a separate webhook struct rather than adding methods to the JobSet struct to implement the Defaulter interface, and it becomes more complicated and didn't seem like it was worth it for this very simple defaulting we are doing.
/label tide/merge-method-squash |
/lgtm |
Fixes #6