validations: Add new property validation for format constraints
#17
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: everettraven The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
approved
| @@ -1,59 +1,62 @@ | |||
| { | |||
| "crdValidation": [ | |||
There was a problem hiding this comment.
It seems the order here is non-determinisitic which is leading to re-ordering of lots of the output? Can that be fixed?
There was a problem hiding this comment.
I would say yes if we were not using the serialized JSON output here. I'm sure it is possible to make it deterministic if we created our own JSON marshalling methods, but at the time that felt like overkill.
If we feel we must have deterministic output that may be a path worth considering, but my general understand is that YAML and JSON output formats are usually non-deterministic in ordering. I could be mistaken, but if this is the case I'd rather not fight the usual behaviors of particular output formats.
We certainly could consider using a different output format that we make more deterministic but we do have logic implemented that these should only ever be updated if there is a semantic difference. I think this semantic difference logic and a fix for #29 would result in less frequent updates of these files.
There was a problem hiding this comment.
I thought most serialization libraries in Go sorted lists on output, but I could be wrong. Being non deterministic in this case seems particularly problematic as it's creating unnecessary churn isn't it?
There was a problem hiding this comment.
I'm not sure. I could certainly be wrong about the list serialization being non-deterministic, but I think it may be because we pull the set of checks we need to perform from a map and execute them in the order we pulled from the map (which is non-deterministic).
I've got #38 up which we can certainly include adjusting that list to always be deterministic (maybe based on validation name?) before running them
k8s-ci-robot
added
the
needs-rebase
Signed-off-by: Bryce Palmer <bpalmer@redhat.com>
as it is flagging validations as duplicates because the code is very similar but the constraints they are validating are different. This may go a bit against DRY principles, but repetition in this case is more situational and keeping this repetition allows us to evolve validations independently as needed. Signed-off-by: Bryce Palmer <bpalmer@redhat.com>
Signed-off-by: Bryce Palmer <bpalmer@redhat.com>
everettraven
force-pushed
the
validation/format
branch
from
70a036f to
313c0fb
Compare
k8s-ci-robot
added
size/L
|
@JoelSpeed This PR should be up to date now and pulling in all the improvements made for deterministic and minimal output. PTAL when you've got some time - thanks! |
|
The Kubernetes project currently lacks enough contributors to adequately respond to all PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle stale |
k8s-ci-robot
added
the
lifecycle/stale
4 participants
Adds a new property validation for validating compatibility of changes to the
formatconstraint of a CRD schema property.Changing the
formatof a property is a breaking change as each format type outlined in https://github.com/kubernetes/apiextensions-apiserver/blob/5714b0b58b88ea1b939e40a3602ee349ee4b57e7/pkg/apis/apiextensions/v1/types_jsonschema.go#L47-L73 has a different validation that is applied to the property value.Making changes to this results in breaking of client/user expectations and may invalidate existing values.
Fixes #18