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.

Sign up for GitHub

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

Jump to bottom

Document server side field validation #30532

Closed
wants to merge 4 commits into from
Closed

Document server side field validation #30532

wants to merge 4 commits into from

Conversation

kevindelgado
Copy link
Contributor

@kevindelgado kevindelgado commented Nov 17, 2021 •
edited
Loading

@k8s-ci-robot k8s-ci-robot added this to the 1.23 milestone Nov 17, 2021
@netlify
Copy link

netlify bot commented Nov 17, 2021 •
edited
Loading

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

🔨 Explore the source changes: d58468b

🔍 Inspect the deploy log: https://app.netlify.com/sites/kubernetes-io-vnext-staging/deploys/619c45b6dedfb5000763b6a2

@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. area/blog Issues or PRs related to the Kubernetes Blog subproject area/release-eng Issues or PRs related to the Release Engineering subproject language/de Issues or PRs related to German language language/en Issues or PRs related to English language language/es Issues or PRs related to Spanish language language/fr Issues or PRs related to French language language/id Issues or PRs related to Indonesian language language/ja Issues or PRs related to Japanese language language/ko Issues or PRs related to Korean language labels Nov 17, 2021
@k8s-ci-robot k8s-ci-robot added language/pl Issues or PRs related to Polish language language/pt Issues or PRs related to Portuguese language language/ru Issues or PRs related to Russian language language/zh Issues or PRs related to Chinese language sig/docs Categorizes an issue or PR as relevant to SIG Docs. sig/release Categorizes an issue or PR as relevant to SIG Release. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Nov 17, 2021
@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Nov 17, 2021
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign pi-victor after the PR has been reviewed.
You can assign the PR to them by writing /assign @pi-victor in a comment when ready.

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@kevindelgado kevindelgado mentioned this pull request Nov 17, 2021
Closed
26 tasks
@jihoon-seo
Copy link
Member

/remove-language de
/remove-language es
/remove-language fr
/remove-language id
/remove-language ja
/remove-language ko
/remove-language pl
/remove-language pt
/remove-language ru
/remove-language zh
/remove-area blog
/remove-area release-eng
/remove-sig release

@k8s-ci-robot k8s-ci-robot removed language/de Issues or PRs related to German language language/es Issues or PRs related to Spanish language labels Nov 18, 2021
@k8s-ci-robot k8s-ci-robot removed language/pt Issues or PRs related to Portuguese language language/ru Issues or PRs related to Russian language language/zh Issues or PRs related to Chinese language area/blog Issues or PRs related to the Kubernetes Blog subproject area/release-eng Issues or PRs related to the Release Engineering subproject sig/release Categorizes an issue or PR as relevant to SIG Release. labels Nov 18, 2021
@mehabhalodiya
Copy link
Contributor

/assign

@sftim
Copy link
Contributor

sftim commented Nov 19, 2021

/retitle [WIP] Document server side field validation

@k8s-ci-robot k8s-ci-robot changed the title Docs for Server Side Field Validation [WIP] Document server side field validation Nov 19, 2021
@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 Nov 19, 2021
@kevindelgado
Copy link
Contributor Author

docs added PTAL @apelisse

@kevindelgado kevindelgado changed the title [WIP] Document server side field validation Document server side field validation Nov 22, 2021
@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 Nov 22, 2021
liggitt
liggitt reviewed Nov 22, 2021
View reviewed changes
content/en/docs/reference/kubernetes-api/common-parameters/common-parameters.md
@@ -58,6 +58,30 @@ When present, indicates that modifications should not be persisted. An invalid o



Copy link
Member

Choose a reason for hiding this comment

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

huh... the header says this is auto-generated:
The file is auto-generated from the Go source code of the component using a generic...

not sure if that's true or where the source for this is

Copy link
Member

Choose a reason for hiding this comment

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

current content lgtm though

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmm, that's a good question. The other parameter descriptions do look like they are pulled from the comments of meta/v1/types.go.

I tried following the instructions for updating the generated docs and was successfully able to run ./update-imported-docs.py, but all it did was update kubectl-commands.html with an unrelated change. I think one issue may be that I can't use a branch that doesn't exist yet

@mehabhalodiya can you confirm that these docs are still being auto-generated and that the source of truth is meta/v1/types.go? If so, do you know how to best update these so that my new changes are included here?

Copy link
Contributor

Choose a reason for hiding this comment

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

@kevindelgado I too believe, The file is auto-generated as mentioned by @liggitt @sftim .
Also, may be you can set the branch of the previous release until 1.23 isn't formally released.

apelisse
apelisse reviewed Nov 23, 2021
View reviewed changes
content/en/docs/reference/kubernetes-api/common-parameters/common-parameters.md
behavior when the `ServerSideFieldValidation` feature gate is disabled.
- Warn: This will send a warning via the [standard warning response
header](https://datatracker.ietf.org/doc/html/rfc7234#section-5.5) for each
unknown that is dropped from the object, and for each duplicate field encountered.
Copy link
Member

Choose a reason for hiding this comment

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

each unknown fields, I'm assuming

apelisse
apelisse reviewed Nov 23, 2021
View reviewed changes
content/en/docs/reference/kubernetes-api/common-parameters/common-parameters.md
@@ -58,6 +58,32 @@ When present, indicates that modifications should not be persisted. An invalid o



## fieldValidation {#fieldValidation}

{{< feature-state for_k8s_version="v1.23" state="alpha" >}}
Copy link
Member

Choose a reason for hiding this comment

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

Should we mention that we're only ever returning strict errors when there is no non-strict errors?

apelisse
apelisse reviewed Nov 23, 2021
View reviewed changes
content/en/docs/reference/kubernetes-api/common-parameters/common-parameters.md
Comment on lines +82 to +86





Copy link
Member

Choose a reason for hiding this comment

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

That's a lot of empty lines :-)

@sftim
Copy link
Contributor

sftim commented Nov 26, 2021

It's typical to update generated documentation in a batch update, using the tooling from https://github.com/kubernetes-sigs/reference-docs/

We usually update the API reference for an upcoming release once, just before the release, as part of the overall release process. Because of that, you don't need to update individual generated files in the branch for that upcoming release.

@jlbutler
Copy link
Contributor

It's typical to update generated documentation in a batch update, using the tooling from https://github.com/kubernetes-sigs/reference-docs/

We usually update the API reference for an upcoming release once, just before the release, as part of the overall release process. Because of that, you don't need to update individual generated files in the branch for that upcoming release.

Is your advice to fold this into the overall API update PR @sftim?

/cc @kevindelgado

@k8s-ci-robot
Copy link
Contributor

@jlbutler: GitHub didn't allow me to request PR reviews from the following users: kevindelgado.

Note that only kubernetes members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

It's typical to update generated documentation in a batch update, using the tooling from https://github.com/kubernetes-sigs/reference-docs/

We usually update the API reference for an upcoming release once, just before the release, as part of the overall release process. Because of that, you don't need to update individual generated files in the branch for that upcoming release.

Is your advice to fold this into the overall API update PR @sftim?

/cc @kevindelgado

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 kevindelgado mentioned this pull request Nov 29, 2021
Merged
@kevindelgado
Copy link
Contributor Author

Closing this in favor of kubernetes/kubernetes#106722 that actually updates the source of truth for these generated docs. Since this feature is only alpha, the existing comment is fine for 1.23 and this updated comment will merge once code freeze is over and master reopens for 1.24

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@liggitt liggitt liggitt left review comments

@apelisse apelisse apelisse left review comments

@mehabhalodiya mehabhalodiya mehabhalodiya left review comments

@ahmetb ahmetb Awaiting requested review from ahmetb

Assignees

@mehabhalodiya mehabhalodiya

Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. language/en Issues or PRs related to English language sig/docs Categorizes an issue or PR as relevant to SIG Docs. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
1.23
Development

Successfully merging this pull request may close these issues.
8 participants
@kevindelgado @k8s-ci-robot @jihoon-seo @mehabhalodiya @sftim @jlbutler @liggitt @apelisse