-
Notifications
You must be signed in to change notification settings - Fork 450
add support for maxlength linter command for kubernetes-api-linter #7283
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
✅ Deploy Preview for kubernetes-sigs-kueue canceled.
|
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.
Almost LGTM
Thanks 💯
a76da24 to
c9b0694
Compare
c9b0694 to
4cfa79c
Compare
|
I checked the failure: https://prow.k8s.io/view/gs/kubernetes-ci-logs/pr-logs/pull/kubernetes-sigs_kueue/7283/pull-kueue-test-e2e-tas-main/1980645093949837312 from kube-scheduler logs Kueue was running on worker5 and worker2: then I checked the logs on kind-worker2: show panic: |
|
/retest |
4cfa79c to
931c9c6
Compare
f80d2fc to
58cc165
Compare
apis/kueue/v1beta2/workload_types.go
Outdated
| // | ||
| // +required | ||
| // +listType=atomic | ||
| // +kubebuilder:validation:MaxItems=64 |
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.
| // +kubebuilder:validation:MaxItems=64 | |
| // +kubebuilder:validation:MaxItems=100000 |
This number can be large, this is the number of nodes essentially
| // clusterQueuePendingWorkload contains the list of top pending workloads. | ||
| // +listType=atomic | ||
| // +optional | ||
| // +kubebuilder:validation:MaxItems=64 |
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.
This type is going to be removed anyway, so maybe just mark it no lint?
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.
Another pass, I think when these are applied I'm happy to merge the PR.
cc @tenzen-y if you could also give it another pass
eb195a2 to
54d209a
Compare
54d209a to
1dd2a62
Compare
|
/lgtm |
|
LGTM label has been added. Git tree hash: 6f8a482c0827cdf2c6b6b81483398bca7e3c1686
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: kannon92, mimowo 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.
LGTM as well.
@kannon92 Thank you!
/approve
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
Enable MaxLength.
Which issue(s) this PR fixes:
Partially address #7119
Special notes for your reviewer:
Does this PR introduce a user-facing change?