-
Notifications
You must be signed in to change notification settings - Fork 1.6k
KEP-5073: Declarative Validation: Explain and update document with cross-field field reference validation information #5363
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
Conversation
29293ad to
d635349
Compare
|
/assign @deads2k @jpbetz @thockin |
|
|
||
| ```go | ||
| type Config struct { | ||
| // +k8s:reference(name: minValue) |
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.
Can we switch examples?
How about we use the union example for virtual fields, and we use the min/max example for field paths?
I think this better highlights our best practices.
If we ever do need min/max to support references we can add that, but it feels more natural to use the simple sibling reference approach for the common case.
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.
I have changed this example to be as suggested. The field path section uses a min/max example currently so I believe we are good there. The initial idea was to show how to use all the tags in the virtual field but I agree it is better to show the best practice/practical cases.
keps/sig-api-machinery/5073-declarative-validation-with-validation-gen/README.md
Outdated
Show resolved
Hide resolved
keps/sig-api-machinery/5073-declarative-validation-with-validation-gen/README.md
Outdated
Show resolved
Hide resolved
keps/sig-api-machinery/5073-declarative-validation-with-validation-gen/README.md
Outdated
Show resolved
Hide resolved
|
|
||
| Limits LimitConfig | ||
|
|
||
| // +k8s:subfield(resources)=+k8s:subfield(cpu)=+k8s:maximum(constraint: maxCpu) |
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.
Oh interesting. Here, maxCpu references the MaxCpu field of Config using a field path? This implies that the field path is relative to the Config and not SettingsConfig, which was not immediately obvious to me because this tag is injected into the SettingsConfig type with subfield.
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.
This is based off the below section in the original DD:
Field Path Resolution & Chained Tags
Explicit Path Context: Field paths always resolve from the original tag location
// 'minValue' always refers to Config.MinValue, not Nested siblings
type Config struct {
MinValue int
// +k8s:subfield(nestedCurrent)=+k8s:minimum(limit: minValue)
Nested Nested
}
type Nested struct {
NestedCurrent int
}
In thinking through the implications of this though, it is not feasible with the current implementation of subfield. This would be a potential case to use +k8s:reference. I will remove this tag examples for now as I don't believe there are actually any subfield cases on shared objects that have cross-field validation with the parent object
6703f2f to
ee52f7e
Compare
deads2k
left a comment
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.
questions
keps/sig-api-machinery/5073-declarative-validation-with-validation-gen/README.md
Outdated
Show resolved
Hide resolved
keps/sig-api-machinery/5073-declarative-validation-with-validation-gen/README.md
Show resolved
Hide resolved
| type Config struct { | ||
| // +k8s:listType=map | ||
| // +k8s:listMapKey=type | ||
| // +k8s:union(union: terminalStatus) |
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.
Need to update the above table for +k8s:union(...).
Actually, I'm unclear about the union tag usage here. Is it only used for declaring the group?
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.
The KEP differs from the tags in the dev-branch which need to be updated to be in line with this KEP Update. In the dev branch there is +k8s:unionMember which has implicit virtual fields for the groups. For +k8s:unionMember that tag is essentially +k8s:union and +k8s:memberOf in a single tag. This KEP Update decouples these concepts to have a generic way of grouping fields via virtual fields and then having +k8s:union consume the virtual field grouping.
+k8s:union in this case is the only tag in this example that validates exclusivity for each member in the group terminalStatus. +k8s:memberOf generically groups the values
Yes, there will be a separate KEP update to address the tags/approach to immutability EDIT: This is submitted and out for review now: https://github.com/kubernetes/enhancements/pull/5373/files |
…oss-field field reference validation information
…e for virtual fields
a11b170 to
a8589d7
Compare
|
lgtm. I see Joe has a review on this too /assign @jpbetz |
|
/lgtm This sets up a solid set of core principals to build on. Thanks! |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: aaron-prindle, jpbetz 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 |
One-line PR description: This addresses how we intend to handle referencing fields for cross-field validations with Declarative Validation.
Issue link: #5073