-
Notifications
You must be signed in to change notification settings - Fork 38.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
add validation functions for metav1.Conditions #92519
Conversation
/cc @liggitt |
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.
a couple nits, looks reasonable otherwise
I'm unsure about adding kube-builder annotations in types defined in k8s.io/kubernetes
staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/validation/validation.go
Outdated
Show resolved
Hide resolved
staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/validation/validation.go
Outdated
Show resolved
Hide resolved
68731b7
to
6182b62
Compare
6182b62
to
e624140
Compare
I think we can draw a reasonable line about adding these tags based on the generator being in kubernetes-sigs. I would allow additional tags on these types to added as long as they were for generators in kubernetes or kubernetes-sigs. I would decline PRs for tags for generators that are not sponsored by kubernetes sigs. |
@deads2k regarding to condition API, do we believe that the message in condition provide enough information for users to understand its context or give users clue about possible mitigations? |
staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/validation/validation.go
Outdated
Show resolved
Hide resolved
Yes. I think that's the purpose of the message. Its clarity will depend on the author |
e624140
to
04a5a44
Compare
/hold cancel |
staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/validation/validation.go
Outdated
Show resolved
Hide resolved
staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/validation/validation.go
Outdated
Show resolved
Hide resolved
staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/validation/validation.go
Outdated
Show resolved
Hide resolved
/approve nit on message, then lgtm |
1324fa5
to
26a0fd1
Compare
allErrs = append(allErrs, field.Invalid(fldPath.Child("observedGeneration"), condition.ObservedGeneration, "must be greater than or equal to zero")) | ||
} | ||
|
||
if condition.LastTransitionTime.IsZero() { |
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.
does this match https://github.com/kubernetes/kubernetes/pull/92660/files#diff-eca3b8d856fa2e661f6da91b61de5e76R1391 ? I think the OpenAPI schema will allow a zero date (not null).
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.
does this match https://github.com/kubernetes/kubernetes/pull/92660/files#diff-eca3b8d856fa2e661f6da91b61de5e76R1391 ? I think the OpenAPI schema will allow a zero date (not null).
Ok. We'll control the ones we can then. Not allowing zero is correct.
} | ||
|
||
const conditionReasonFmt string = "[A-Za-z]([A-Za-z0-9_,:]*[A-Za-z0-9_])?" | ||
const conditionReasonErrMsg string = "a condition reason must start with alphabetic character, followed by a string of alphanumeric characters or '_,:', and must end with an alphanumeric character or '_'" |
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 doesn't match the regexp. Some "optionally" are missing.
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 doesn't match the regexp. Some "optionally" are missing.
I need help seeing it. Which optionally do I need to add?
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.
const conditionReasonErrMsg string = "a condition reason must start with alphabetic character, followed by a string of alphanumeric characters or '_,:', and must end with an alphanumeric character or '_'" | |
const conditionReasonErrMsg string = "a condition reason must start with alphabetic character, optionally followed by a string of alphanumeric characters or '_,:', and must end with an alphanumeric character or '_'" |
Lgtm, with exception of https://github.com/kubernetes/kubernetes/pull/92519/files#r449081321. |
26a0fd1
to
a88b8db
Compare
/retest |
tagging per comments. |
/retest |
/retest Review the full test history for this PR. Silence the bot with an |
2 similar comments
/retest Review the full test history for this PR. Silence the bot with an |
/retest Review the full test history for this PR. Silence the bot with an |
a88b8db
to
e5fdc77
Compare
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: deads2k, liggitt 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 |
/lgtm |
/retest |
@deads2k: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. 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. I understand the commands that are listed here. |
/retest Review the full test history for this PR. Silence the bot with an |
The last commit adds validation rules. It's best to be as constraining as we think is reasonable, but we should try to make it possible to be consistent with generated schemas.
@liggitt as promised. See if you agree with the restrictions.
/kind feature
/priority important-soon
@kubernetes/sig-api-machinery-feature-requests
/hold this still needs tests
@damemi @sttts I tried to make these validation rules easy to port into kubebuilder markers in the types themselves so CRDs will generate cleanly. Can you confirm these look good.
@mfojtik do these rules correspond to how you've seen them used too?
related to kubernetes/enhancements#1623