feat: add unique items validator#48
feat: add unique items validator#48Sammyjroberts wants to merge 1 commit intokubernetes-sigs:mainfrom
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Sammyjroberts The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
cncf-cla: yes
k8s-ci-robot
added
the
size/L
There was a problem hiding this comment.
@everettraven learning from KAL, is there anything here that reminds us we need to registry these? Do we want to copy over a similar script as we just added for KAL?
There was a problem hiding this comment.
I don't think that there is anything that reminds us to register these no.
We can certainly copy a similar script over.
| // incompatible changes to the uniqueItems constraints of CRD properties. | ||
| // The uniqueItems constraint enforces that lists should only contain unique items. | ||
| // Going from non-unique to unique is more restrictive and breaking. | ||
| // Going from unique to non-unique is less restrictive and OK. |
There was a problem hiding this comment.
This depends on who you are aiming your compatibility at. Going to non-unique may surprise readers. I think we need to enforce in both directions, but have an option for folks to choose whether they are ok moving to the less restrictive option
There was a problem hiding this comment.
+1 on enforcing in both directions.
Some existing checks don't do this, but this is something I want to start moving towards. For new checks, we should restrict both ways by default but allow configuration based on individual user needs for which direction is best for their compatibility requirements.
| var ( | ||
| // ErrUniqueItemsConstraintAdded represents an error state where a uniqueItems constraint was added to a property. | ||
| ErrUniqueItemsConstraintAdded = errors.New("uniqueItems constraint added when there was none previously") | ||
| ) No newline at end of file |
There was a problem hiding this comment.
Nit, missing final new char line here
| } | ||
|
|
||
| internaltesting.RunTestcases(t, testcases...) | ||
| } No newline at end of file |
There was a problem hiding this comment.
Nit, missing final new line char
everettraven
left a comment
everettraven
left a comment
There was a problem hiding this comment.
@Sammyjroberts Thanks for the contribution - apologies on the log delay for a review.
Aside from existing comments, would you mind also adding e2e tests for this new validation?
k8s-ci-robot
added
the
needs-rebase
|
PR needs rebase. DetailsInstructions 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-sigs/prow repository. |
4 participants
#22