Validation of system labels only #4879
Conversation
knative-prow-robot
added
the
size/XL
There was a problem hiding this comment.
@shashwathi: 0 warnings.
In response to this:
Addresses partially #4700
Proposed Changes
- validation of system labels
- Breaking down Add validation for system labels and annotations #4822 PR
- Will follow up with separate PR for annotation validation after Do not allow modification for creator/lastModifer annotations #4763 is merged
Release Note
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.
|
/assign @dgerd |
| @@ -19,6 +19,8 @@ package serving | |||
| const ( | |||
| GroupName = "serving.knative.dev" | |||
There was a problem hiding this comment.
While you are here do you mind dropping a comment on this public constant?
| if ref.Kind != resource || val != ref.Name { | ||
| errs = errs.Also(apis.ErrInvalidArrayValue(ref.Name, label, i)) | ||
| } | ||
| found = true |
There was a problem hiding this comment.
I am not sure this loop is doing what you expect... With this we always return found true on the first item??
There was a problem hiding this comment.
found variable is used for tracking there is at least one owner reference with expected key.
There was a problem hiding this comment.
but you set it to true regardless of whether you found proper one or not.
There was a problem hiding this comment.
I fixed the logic for this function. Please take another look.
| errs = errs.Also(apis.ErrInvalidValue(val, label)) | ||
| } | ||
| for i, ref := range ownerRefs { | ||
| if ref.Kind == resource && val == ref.Name { |
There was a problem hiding this comment.
well, just if !(...) { errs = errs.Also(...)
TODO: annotations
- Refactor to use switch instead of if condition - Use serving constants in tests
- Use viaField for uniform field values - use return variable names to avoid multiple declarations
- Address edge case of multiple owner references
- Logic was broken for case when parent label is present but the owner ref is absent or wrong owner references are present. - Fixed the logic and added testcase to guard for future - Update serving.GroupNamePrefix to local const
- If there is at least one owner ref present for matching label then ignore the rest of references.
|
/approve I will let @vagababov set the lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dgerd, shashwathi 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 |
knative-prow-robot
added
the
approved
|
/test pull-knative-serving-integration-tests |
Addresses partially #4700
Proposed Changes
Release Note
cc @dgerd @vagababov @savitaashture