-
Notifications
You must be signed in to change notification settings - Fork 288
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
Add validation for Deployment queue-name to fail fast #3555
Add validation for Deployment queue-name to fail fast #3555
Conversation
✅ Deploy Preview for kubernetes-sigs-kueue canceled.
|
1826829
to
c74dad4
Compare
/cc @mimowo |
c74dad4
to
3b1b81e
Compare
Adding validation is tricky. Can you please summarize if the newly added validation prevents situations which would fail later anyway? Or it could work and we now start rejecting? |
Yes, it should prevent fail later on Pods. UPDATED: If we don’t have validation on the Deployment, it will try to create pods with an invalid queue-name, which will fail. |
/kind bug |
Thanks, yeah the validation is just a fail fast - it would not work before. In that case I'm ok to consider it a bugfix and include in 0.9.2, but it feels too late for 0.9.1. cc @tenzen-y |
Yeah, this is the change that either enhances or bugs since even if this commit does not exist, the scheduling mechanism will not be broken. But, since this would be helpful valuation, I'm ok with cherry-pick to v0.9. As @mimowo mentioned, the 0.9.1 patch commit has already closed. So, if users want to use this validation, they can use the latest release-0.9 branch image. |
/unhold |
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.
Thx
/lgtm
/approve
LGTM label has been added. Git tree hash: 5b467310476baa2cee3bd866ad181b081973cdce
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mbobrovskyi, tenzen-y 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 |
/cherry-pick release-0.9 |
@tenzen-y: once the present PR merges, I will cherry-pick it on top of 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. |
@tenzen-y: #3555 failed to apply on top of branch "release-0.9":
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. |
@mbobrovskyi I checked locally that #3556 produces a conflict. |
/cherry-pick release-0.9 |
@mbobrovskyi: new pull request created: #3580 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. |
…s#3555) * Add queue-name label validation on Deployment create. * Use clientObject on ValidateAnnotationAsCRDName and ValidateLabelAsCRDName.
What type of PR is this?
/kind bug
What this PR does / why we need it:
Add validation for Deployment queue-name to fail fast
Which issue(s) this PR fixes:
Prepare #3528
Special notes for your reviewer:
Does this PR introduce a user-facing change?