-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Follow up on KEP-2885 using more precise language #2970
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: kevindelgado The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/assign @wojtek-t |
/lgtm Thanks! /assign @deads2k |
@deads2k can you slap an /approve on this when you are free |
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.
Thanks @kevindelgado 👍!
A couple of comments:
- Is
FieldValidation
case sensitive or not? Maybe add wording on that - YAML disallows duplicate keys; add some considerations on this (https://yaml.org/spec/1.2.1/). I'd personally probably shoot for being consistent and transitioning (eventually) to always disallow duplicate fields in JSON as well, as per https://datatracker.ietf.org/doc/html/rfc7493#section-2.3. It should be noted that such a change (always disallowing duplicate fields), is not necessarily considered exactly related to this KEP, but if people think it's close enough to include here, we probably should. Current duplicate field handling is pretty non-user-friendly: https://play.golang.org/p/CkCMdoGXc07. Once we've upgraded to yaml.v3 (Upgrade yaml.v2 to yaml.v3, improve testing and bugfixes kubernetes-sigs/yaml#61 (comment); kustomize is already at yaml.v3), duplicate fields will always be disallowed when decoding, no matter what.
- I don't see a clear conclusion here whether to go with the content type or query param approach, or both. I'd say we start with the query param approach for now.
- See WIP: Strict schema validation for POST and PUT kubernetes#104433 (comment) for updated wording of why the initial benchmarks look so expensive. Once https://github.com/kubernetes/kubernetes/pull/105030/files#diff-54b1d11ee7334f5492b87652fae8614bd77ce0fc0428f3ce986e9556fb1f9252R198-R222 is merged; we should see much smaller figures. Can you re-run with this patch and update the numbers?
- switch from json-iterator to forked stdlib json decoder kubernetes#105030 also points out that untagged fields are disallowed to decode into in the "strict" mode. Please update with information about this. We probably need to do some digging to see what the rationale for this code to be added in the first place switch from json-iterator to forked stdlib json decoder kubernetes#105030 (comment)
- Point out that a requirement for this feature to become beta/GA is that this functionality is exposed to the rest of the ecosystem that wants to do YAML/JSON decoding "the official k8s way", too, and not just buried in the API server without being importable. We need to let people reproduce our semantics, otherwise the benefits will be very few.
@@ -780,13 +780,7 @@ question. | |||
Pick one more of these and delete the rest. | |||
|
|||
- [x] Metrics | |||
- Metric name: CPU Usage |
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.
Why were these metrics removed?
@kevindelgado: PR needs rebase. 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/test-infra repository. |
These follow-ups were captured in the beta KEP #3081 |
Small follow up based on feedback from 2886 to use more precise language