Skip to content
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

Field Validation API Concepts #32357

Merged

Conversation

kevindelgado
Copy link
Contributor

@k8s-ci-robot k8s-ci-robot added this to the 1.24 milestone Mar 18, 2022
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Mar 18, 2022
@netlify
Copy link

netlify bot commented Mar 18, 2022

👷 Deploy Preview for kubernetes-io-vnext-staging processing.

Name Link
🔨 Latest commit 2dc5f25
🔍 Latest deploy log https://app.netlify.com/sites/kubernetes-io-vnext-staging/deploys/62e61a099c8ac10008e9eca1

@k8s-ci-robot k8s-ci-robot added the sig/docs Categorizes an issue or PR as relevant to SIG Docs. label Mar 18, 2022
@mehabhalodiya
Copy link
Contributor

/assign

@sftim
Copy link
Contributor

sftim commented Mar 21, 2022

/retitle [WIP] placeholder for field validation beta docs

@k8s-ci-robot k8s-ci-robot changed the title placeholder for field validation beta docs [WIP] placeholder for field validation beta docs Mar 21, 2022
@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 21, 2022
@tengqm
Copy link
Contributor

tengqm commented Mar 30, 2022

@kevindelgado I have proposed something in #32592, for the same topic. Would you like review #32592 and see if that covers all changes needed?

@kevindelgado
Copy link
Contributor Author

kevindelgado commented Mar 30, 2022

@kevindelgado I have proposed something in #32592, for the same topic. Would you like review #32592 and see if that covers all changes needed?

Thanks for the PR. We can can go ahead and merge that. I think the one other piece of documentation we want, is something for the api-concepts page similar to what we have for --dry-run. I will use this PR to do that, and should have something up for review EOD today.

@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Mar 30, 2022
@kevindelgado
Copy link
Contributor Author

/assign @apelisse

@k8s-ci-robot k8s-ci-robot added the language/en Issues or PRs related to English language label Mar 30, 2022
@kevindelgado kevindelgado changed the title [WIP] placeholder for field validation beta docs Field Validation API Concepts Mar 30, 2022
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 30, 2022
content/en/docs/reference/using-api/api-concepts.md Outdated Show resolved Hide resolved
content/en/docs/reference/using-api/api-concepts.md Outdated Show resolved Hide resolved
content/en/docs/reference/using-api/api-concepts.md Outdated Show resolved Hide resolved
content/en/docs/reference/using-api/api-concepts.md Outdated Show resolved Hide resolved
content/en/docs/reference/using-api/api-concepts.md Outdated Show resolved Hide resolved
content/en/docs/reference/using-api/api-concepts.md Outdated Show resolved Hide resolved
content/en/docs/reference/using-api/api-concepts.md Outdated Show resolved Hide resolved
content/en/docs/reference/using-api/api-concepts.md Outdated Show resolved Hide resolved
content/en/docs/reference/using-api/api-concepts.md Outdated Show resolved Hide resolved
content/en/docs/reference/using-api/api-concepts.md Outdated Show resolved Hide resolved
@apelisse
Copy link
Member

apelisse commented Apr 1, 2022

Technical LGTM

sftim
sftim previously requested changes Apr 1, 2022
Copy link
Contributor

@sftim sftim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remember to remove placeholder.md from this PR.

@kevindelgado kevindelgado force-pushed the ss-field-validation-beta branch from 55d2b63 to a760f33 Compare April 1, 2022 18:40
@kevindelgado kevindelgado requested a review from apelisse April 1, 2022 18:41
@k8s-ci-robot k8s-ci-robot removed language/ja Issues or PRs related to Japanese language language/ko Issues or PRs related to Korean language language/ru Issues or PRs related to Russian language language/zh Issues or PRs related to Chinese language lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Jul 19, 2022
@kevindelgado kevindelgado force-pushed the ss-field-validation-beta branch from 2fec366 to 1d4accf Compare July 24, 2022 06:41
@kevindelgado
Copy link
Contributor Author

/lgtm

@k8s-ci-robot
Copy link
Contributor

@kevindelgado: you cannot LGTM your own PR.

In response to this:

/lgtm

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.

@kevindelgado
Copy link
Contributor Author

feedback/nits addressed, please tag it again @sftim

Copy link
Contributor

@sftim sftim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One more thought. Assume that some folk don't read the “Setting the field validation level” text because they know it's not enabled for their cluster.

When someone supplies duplicate values and these are eventually accepted, it would be nice to clearly explain which value wins. For JSON, it's the last duplicate - right?

@@ -661,6 +661,76 @@ of single-resource API requests, then aggregates the responses if needed.
By contrast, the Kubernetes API verbs **list** and **watch** allow getting multiple
resources, and **deletecollection** allows deleting multiple resources.

## Field validation

{{< feature-state for_k8s_version="v1.25" state="beta" >}}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On reflection, I think this should be for the heading “Setting the field validation level” and not for the 2nd-level field validation heading. Even with the feature gate off, there's some (actually quite a lot) of field validation going on.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

moved

{{< feature-state for_k8s_version="v1.25" state="beta" >}}

By default, the API server drops fields that it does not recognize
from an object it receives.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
from an object it receives.
from an input that it receives (for example, the JSON body of a `PUT` request).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

By default, the API server drops fields that it does not recognize
from an object it receives.

The two situations where this will occur are:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(nit)

Suggested change
The two situations where this will occur are:
There are two situations where the API server drops fields that you supplied in an HTTP request.
These situations are:

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@kevindelgado kevindelgado force-pushed the ss-field-validation-beta branch from 1d4accf to 4584fd5 Compare July 28, 2022 05:24
Copy link
Contributor Author

@kevindelgado kevindelgado left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

all feedback addressed, ptal @sftim

@@ -661,6 +661,76 @@ of single-resource API requests, then aggregates the responses if needed.
By contrast, the Kubernetes API verbs **list** and **watch** allow getting multiple
resources, and **deletecollection** allows deleting multiple resources.

## Field validation

{{< feature-state for_k8s_version="v1.25" state="beta" >}}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

moved

{{< feature-state for_k8s_version="v1.25" state="beta" >}}

By default, the API server drops fields that it does not recognize
from an object it receives.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

By default, the API server drops fields that it does not recognize
from an object it receives.

The two situations where this will occur are:
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@kevindelgado kevindelgado requested a review from sftim July 28, 2022 05:24
@kevindelgado kevindelgado force-pushed the ss-field-validation-beta branch from 4584fd5 to 2dc5f25 Compare July 31, 2022 05:58
@divya-mohan0209 divya-mohan0209 added this to the 1.25 milestone Jul 31, 2022
@kevindelgado
Copy link
Contributor Author

Build passes now, ptal @sftim

@reylejano
Copy link
Member

/assign @didicodes
/cc @kcmartin

@sftim
Copy link
Contributor

sftim commented Aug 8, 2022

@kevindelgado you set a hold on this PR in #32357 (comment)
Is that still needed?

@apelisse
Copy link
Member

apelisse commented Aug 8, 2022

/unhold

Not needed anymore. Kevin is out for a while.

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 8, 2022
@reylejano
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 16, 2022
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: b3400bdc27af9d47c37a3535e782c650249c0e63

@k8s-ci-robot k8s-ci-robot merged commit fc237ad into kubernetes:dev-1.25 Aug 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. language/en Issues or PRs related to English language lgtm "Looks good to me", indicates that a PR is ready to be merged. sig/docs Categorizes an issue or PR as relevant to SIG Docs. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants