feat(crd): Prevent breaking changes in RGD schemas #352
Conversation
michaelhtm
left a comment
michaelhtm
left a comment
There was a problem hiding this comment.
Great job on this @a-hilaly!! 🚀🚀🚀
I left a few inline comments
| // Breaking change types | ||
| PropertyRemoved ChangeType = "PROPERTY_REMOVED" | ||
| TypeChanged ChangeType = "TYPE_CHANGED" | ||
| RequiredAdded ChangeType = "REQUIRED_ADDED" | ||
| EnumRestricted ChangeType = "ENUM_RESTRICTED" | ||
| PatternChanged ChangeType = "PATTERN_CHANGED" | ||
|
|
||
| // Non-breaking change types | ||
| PropertyAdded ChangeType = "PROPERTY_ADDED" | ||
| DescriptionChanged ChangeType = "DESCRIPTION_CHANGED" | ||
| DefaultChanged ChangeType = "DEFAULT_CHANGED" | ||
| RequiredRemoved ChangeType = "REQUIRED_REMOVED" | ||
| EnumExpanded ChangeType = "ENUM_EXPANDED" |
There was a problem hiding this comment.
Maybe we can add number changes as well..I saw a PR approved for adding maximum and minimum to the schema. We should also track those changes..
There was a problem hiding this comment.
Are those considered breaking changes? should we really inspect the defaults/max/min and basically every attribute in https://kro.run/docs/concepts/simple-schema#supported-markers ? cc @jakobmoellerdev
|
Unknown CLA label state. Rechecking for CLA labels. Send feedback to sig-contributor-experience at kubernetes/community. /check-cla |
k8s-ci-robot
added
cncf-cla: yes
|
Apologies for a very drive-by suggestion, but crdify might be useful to externalize some of the breaking change detection: https://github.com/kubernetes-sigs/crdify |
|
@crenshaw-dev I believe crdify is a CLI tool first. does it plan to support a library mode long term? If so lets take a look there! |
|
@jakobmoellerdev good question... I used it as a go lib for a small project, and it seemed to work fine: https://github.com/crenshaw-dev/miaka/blob/8d568300f998f1028942ceee66d65cace12b8f09/pkg/build/validation/breaking_changes.go#L45 I'm not sure whether they plan to maintain stable APIs for use as a go library, but I asked the question here: kubernetes-sigs/crdify#52 |
|
Thanks for the suggestion @crenshaw-dev! I actaully looked at crdify before writing this PR. At that time, it didn't strike me as something designed for library usage and looked primarily a CLI tool [global registries, That said, thank you for opening the github issue - happy to reconsider this in favor of using crdify. And since I was here, I just rebased this PR! |
k8s-ci-robot
added
the
do-not-merge/invalid-commit-message
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: a-hilaly, n3wscott 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 |
k8s-ci-robot
added
approved
ResourceGraphDefinitions generate CRDs dynamically. Schema changes like removing properties, changing types, or adding required fields can break existing resources. This adds a compatibility layer that compares schemas before CRD updates and blocks breaking changes. Introduces a `pkg/graph/crd/compat` package with `Compare()` and `CompareVersions()` functions that return a `Report` identifying breaking vs non-breaking changes. The CRD wrapper now validates compatibility before applying updates. Resolves: 186
k8s-ci-robot
removed
the
do-not-merge/invalid-commit-message
|
Looks like crdify is usable as a library! |
8 participants
ResourceGraphDefinitionsgenerate CRDs dynamically. Schema changes likeremoving properties, changing types, or adding required fields can break
existing resources. This adds a compatibility layer that compares schemas
before CRD updates and blocks breaking changes.
Introduces a
pkg/graph/crd/compatpackage withCompare()andCompareVersions()functions that return aReportidentifying breakingvs non-breaking changes. The CRD wrapper now validates compatibility before
applying updates.
Resolves #186