-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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 MaxItems markers to API fields #11934
⚠️ Add MaxItems markers to API fields #11934
Conversation
Signed-off-by: Stefan Büringer [email protected]
/assign @fabriziopandini @chrischdi @JoelSpeed PTAL and let me know if you have concerns if some of these values are too low and I'm happy to increase them. |
- path: "api/v1beta1/*" | ||
text: "must have a maximum length, add (kubebuilder:validation:MaxLength|kubebuilder:validation:items:MaxLength) marker" | ||
linters: | ||
- kal |
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.
Will be removed via the next PRs
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.
It may be interesting to try and inspect the maximum estimated size of each of these types based on the maximum request size, if you wanted to try, we do something similar in this snippet. The manual way to do it is to work out the minimum serialized size of the object and divide that into 3MiB.
@JoelSpeed @fabriziopandini @chrischdi Thx for the reviews. 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.
One last nit, otherwise lgtm for me
9b51b88
to
4bc5347
Compare
/lgtm |
LGTM label has been added. Git tree hash: 8482e2aff2e20034db0dcdddeffd8778499123f5
|
/approve for @JoelSpeed and @chrischdi to take another look (@sbueringer feel free to unhold if we can get additional finding fixed in a follow up) |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: fabriziopandini 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 |
Alrighty. Thx. I'll merge this one already. If there any further findings please let me know and I'll follow-up. Or feel free to comment on #11949 I added markers to mostly the same fields as I did on this PR here |
/hold cancel |
Signed-off-by: Stefan Büringer [email protected]
What this PR does / why we need it:
This PR only enables the KAL linter for
kubebuilder:validation:MaxItems
. There will be other PRs for similar markers soon.I tried to find a good trade-off between setting some reasonable upper bounds and restricting us too much, e.g. the API fields should not limit the number of MDs or Machines we can support per Cluster.
I also tried to pick similar values for similar fields and also to use only a small set of different values to make this all a bit easier.
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close the issue(s) when PR gets merged):Part of #11834