-
Notifications
You must be signed in to change notification settings - Fork 14.7k
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
Update doc for KEP 2876 #42120
Update doc for KEP 2876 #42120
Conversation
👷 Deploy Preview for kubernetes-io-vnext-staging processing.
|
/sig api-machinery |
/assign @jpbetz |
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.
Some text suggestions
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
Show resolved
Hide resolved
@windsonsea All comments have been addressed. Thank you |
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
I have a few suggestions, and also I'd like to know why setting fieldPath
benefits people who consume the API that the CRD defines.
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
Show resolved
Hide resolved
#### The reason and fieldPath fields | ||
|
||
Besides the `message` and `messageExpression` fields, which defines the string reported for a validation rule failure, | ||
we have also added two more fields under `validation`: | ||
- `reason` field which allows user to specify a machine-readable validation failure reason when a request fails this validation rule. | ||
- `fieldPath` field which specify the field path returned when the validation fails. |
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.
We like timeless documentation: please avoid “we have also added”.
#### The reason and fieldPath fields | |
Besides the `message` and `messageExpression` fields, which defines the string reported for a validation rule failure, | |
we have also added two more fields under `validation`: | |
- `reason` field which allows user to specify a machine-readable validation failure reason when a request fails this validation rule. | |
- `fieldPath` field which specify the field path returned when the validation fails. | |
Setting `messageExpression` is optional. | |
#### The `message` field {#field-message} | |
If you want to set a static message, you can supply `message` rather than `messageExpression`. | |
The value of `message` is used as an opaque error string if validation fails. | |
Setting `message` is optional. | |
#### The `reason` field {#field-reason} | |
You can add a machine-readable validation failure reason within a `validation`, to be returned | |
whenever a request fails this validation rule. | |
<!-- an example here would be nice, but is hard to add using GitHub suggestions --> | |
Setting `reason` is optional. | |
#### The `fieldPath` field {#field-field-path} | |
You can specify the path to the data that have failed to validate. |
NB. Can we explain why it's useful to specify a fieldPath
?
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.
Thank you for the review! I have updated the doc to align with the suggested format.
For fieldPath
, we wanna user to be able to specify the accurate path when err returned instead of always using the path which self
scoped. It is very usefully especially when do cross fields validation and user wanna the returned err indicate the path the err occurred. For example, if user trying to say a field has to be a different value than another field, something like self.foo.test.a != self.b
. The validation has to be scoped high enough to have access to both field a
and b
but we wanna the returned err path to be on a
, in this case having a fieldPath .foo.test.a
will be great.
Hope that would make sense to you. Thank you^^
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.
Edit: I have also added explain in the doc with the example. Thank you
content/en/docs/tasks/extend-kubernetes/custom-resources/custom-resource-definitions.md
Show resolved
Hide resolved
/approve |
/lgtm |
LGTM label has been added. Git tree hash: b052a7422411ba0ecde37f95cb38283d837143ba
|
@sftim The PR has done with technical review and all comments have been addressed. It is ready for doc approval. Thank you! |
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.
/approve
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jpbetz, sftim 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 |
Update doc for KEP 2876
No description provided.