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

Add documentation for Validation Rules feature to Custom Resource Definitions task docs #30494

Closed
wants to merge 4 commits into from
Closed

Add documentation for Validation Rules feature to Custom Resource Definitions task docs #30494

wants to merge 4 commits into from

Conversation

jpbetz
Copy link
Contributor

@jpbetz jpbetz commented Nov 15, 2021 •
edited
Loading

@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 15, 2021
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Nov 15, 2021 •
edited
Loading

CLA Signed

The committers are authorized under a signed CLA.

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. language/en Issues or PRs related to English language sig/docs Categorizes an issue or PR as relevant to SIG Docs. labels Nov 15, 2021
@netlify
Copy link

netlify bot commented Nov 15, 2021 •
edited
Loading

✔️ Deploy Preview for kubernetes-io-main-staging ready!

🔨 Explore the source changes: aadf90a

🔍 Inspect the deploy log: https://app.netlify.com/sites/kubernetes-io-main-staging/deploys/61981c1c2c9e1f0007bddbc7

😎 Browse the preview: https://deploy-preview-30494--kubernetes-io-main-staging.netlify.app

@cici37 cici37 force-pushed the validation-rules branch 2 times, most recently from 9b129c9 to dd93f10 Compare November 15, 2021 21:15
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 15, 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 divya-mohan0209 after the PR has been reviewed.
You can assign the PR to them by writing /assign @divya-mohan0209 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

@cici37 cici37 mentioned this pull request Nov 16, 2021
Closed
21 tasks
@sftim
Copy link
Contributor

sftim commented Nov 16, 2021

/wg api-expression
/sig api-machinery

@k8s-ci-robot k8s-ci-robot added wg/api-expression Categorizes an issue or PR as relevant to WG API Expression. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. labels Nov 16, 2021
sftim
sftim reviewed Nov 16, 2021
View reviewed changes
content/en/docs/tasks/extend-kubernetes/custom-resources/custom-resource-definitions.md Outdated Show resolved Hide resolved
content/en/docs/tasks/extend-kubernetes/custom-resources/custom-resource-definitions.md Outdated Show resolved Hide resolved
content/en/docs/tasks/extend-kubernetes/custom-resources/custom-resource-definitions.md Outdated
spec:
type: object
x-kubernetes-validation-rules:
- rule: "self.minReplicas <= self.replicas && self.replicas <= self.maxReplicas"
Copy link
Contributor

Choose a reason for hiding this comment

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

For teaching, would this work better as two separate rules in the array?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nifty, yes. Updated.

@mehabhalodiya
Copy link
Contributor

/assign

@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Nov 18, 2021
jpbetz
jpbetz commented Nov 18, 2021
View reviewed changes
content/en/docs/tasks/extend-kubernetes/custom-resources/custom-resource-definitions.md Outdated
The `rule` under `x-kubernetes-validations` represents the expression which will be evaluated by CEL.

The `message` represents the message displayed when validation fails. The message is required if the Rule contains
line breaks. The message must not contain line breaks. If unset, the message is "failed rule: {Rule}".
Copy link
Contributor Author

Choose a reason for hiding this comment

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

How about a multi-line example showing multiple rules with messages?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also, I don't think we need to be quite as detailed about the exact rules of the message field here where we first introduce it. I'd say only what it does here and then teach with an example. It's okay down at the end of the doc somewhere to spell out the exact rules if you want.

Copy link
Contributor

Choose a reason for hiding this comment

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

I have added message into example and add the response example with message unset.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This part of the above comment is still unaddressed right? "I don't think we need to be quite as detailed about the exact rules of the message field here where we first introduce it. I'd say only what it does here and then teach with an example. It's okay down at the end of the doc somewhere to spell out the exact rules if you want."

Copy link
Contributor

@cici37 cici37 Nov 19, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I'd say only what it does here and then teach with an example.

I thought we are ok with what it has now. Reduced the details on message.

jpbetz
jpbetz commented Nov 18, 2021
View reviewed changes
content/en/docs/tasks/extend-kubernetes/custom-resources/custom-resource-definitions.md Outdated
* spec: Invalid value: map[string]interface {}{"minReplicas": 0, "replicas":20, "maxReplicas": 10}: failed rule: self.minReplicas <= self.replicas && self.replicas <= self.maxReplicas
```

TODO: (using text from types_jsonprops.go and KEP were applicable, but using "full" multi-line examples that include both the schema and the custom resource data)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do we have any multi-line examples that have both the schema and the custom resource data being validated? I think that really helps explain the idea.

Copy link
Contributor

Choose a reason for hiding this comment

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

added

jpbetz
jpbetz commented Nov 19, 2021
View reviewed changes
content/en/docs/tasks/extend-kubernetes/custom-resources/custom-resource-definitions.md Outdated
type: object
x-kubernetes-validation-rules:
- rule: "self.minReplicas <= self.replicas"
message: "replicas should be greater than minReplicas."
Copy link
Contributor Author

Choose a reason for hiding this comment

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

nit: "should be greater than or equal to" or "should not be less than"?

Copy link
Contributor

Choose a reason for hiding this comment

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

Updated

@jpbetz jpbetz marked this pull request as ready for review November 19, 2021 17:42
@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 19, 2021
@jlbutler
Copy link
Contributor

@jpbetz Can you rebase this PR to the dev-1.23 branch please? Thanks!

@cici37
Copy link
Contributor

cici37 commented Nov 24, 2021

@jpbetz Can you rebase this PR to the dev-1.23 branch please? Thanks!

Hi @jlbutler , Sorry for the inconvenient. I have opened #30626 to replace the current PR for documentation of Validation Rules feature to Custom Resource Definitions. Thank you

@cici37
Copy link
Contributor

cici37 commented Nov 24, 2021

/close

All commits have been moved to #30626 which is against dev-1.23 branch.

@k8s-ci-robot
Copy link
Contributor

@cici37: Closed this PR.

In response to this:

/close

All commits have been moved to #30626 which is against dev-1.23 branch.

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.

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

@sftim sftim sftim left review comments

@cici37 cici37 cici37 left review comments

@celestehorgan celestehorgan Awaiting requested review from celestehorgan

@daminisatya daminisatya Awaiting requested review from daminisatya

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/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/docs Categorizes an issue or PR as relevant to SIG Docs. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. wg/api-expression Categorizes an issue or PR as relevant to WG API Expression.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@jpbetz @k8s-ci-robot @sftim @mehabhalodiya @jlbutler @cici37