-
Notifications
You must be signed in to change notification settings - Fork 23
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 crddiff: A CRD breaking schema changes detection & reporting tool #48
Conversation
d78ec15
to
0fa3c7e
Compare
Signed-off-by: Alper Rifat Ulucinar <[email protected]>
528a283
to
6d9fcda
Compare
Signed-off-by: Alper Rifat Ulucinar <[email protected]>
cmd/crddiff/main.go
Outdated
baseCRDPath = app.Arg("base", "The manifest file path of the CRD to be used as the base").Required().ExistingFile() | ||
revisionCRDPath = app.Arg("revision", "The manifest file path of the CRD to be used as a revision to the base").Required().ExistingFile() | ||
) | ||
kingpin.MustParse(app.Parse(os.Args[1:])) |
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.
Why did we introduce this as a new command instead of adding a subcommand to the uptest binary?
We have uptest e2e
for end to end testing and this could be introduced as uptest crddiff
.
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.
I preferred a different command because I think they are conceptually different tasks, one is e2e testing using kuttl and the other is detecting & reporting breaking API changes. Instead of producing a single binary capable of doing conceptually different tasks, I preferred splitting the functionality into multiple binaries (which I believe is more inline with the Unix philosophy). But I don't have a strong opinion, we can also merge them into a single binary.
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.
The main advantage of merging into a single binary is packaging and distribution, i.e., we won't have to make separate releases and introduce another tool into the build submodule.
I believe they could be combined conceptually as tooling to test xp resources :)
So, I would vote for combining them due to practical reasons.
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.
Done. We now have crddiff revision
and crddiff self
as subcommands to a single uptest binary.
crdschema/crd.go
Outdated
} | ||
|
||
func getOpenAPIv3Document(crd *v1.CustomResourceDefinition) (*openapi3.T, error) { | ||
if len(crd.Spec.Versions) != 1 { |
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.
@ulucinar while bumping API version of a CRD, we need to keep the previous version by setting storage
field to false
which will break the condition here. @ezgidemirel recently did that while bumping compositionsrevisions to v1beta1 here.
AFAIR from this work, not keeping previous version in CRD spec will make the package manager fail to upgrade with an error like below:
cannot establish control of object: CustomResourceDefinition.apiextensions.k8s.io "nodepools.container.gcp.crossplane.io" is invalid: status.storedVersions[0]: Invalid value: "v1alpha1": must appear in spec.versions
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.
Thanks @turkenh, I think this is a good point. I believe we can consider extending the functionality if we consider using this tool in other contexts (like moving it to crossplane-tools so that it's more of a generic tool). Currently, we are only planning to use it with Upjet-based providers, which are currently capable of producing a single API version of a CRD. And we would like to use this tool to detect changes in APIs of the same Terraform resource generated for different versions of the underlying Terraform provider.
If, at some point, we would like to generalize the functionality, we can add a new mode for the tool, where we accept a single CRD and detect changes between the different versions declared for that CRD. Do you think we had better introduce a similar functionality now?
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.
Added this functionality as a new subcommand. Now, we have:
crddiff revision <base path> <revision path>
to detect changes between two revisions of the same generated CRD (CI usecase), and
crddiff self <crd path>
to detect changes between versions declared in a CRD.
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.
Thank you @ulucinar!
Indeed, I forgot about the primary use case we are focusing on now but great that you've introduced that functionality now.
Signed-off-by: Alper Rifat Ulucinar <[email protected]>
Signed-off-by: Alper Rifat Ulucinar <[email protected]>
…s in the declared versions of a single CRD Signed-off-by: Alper Rifat Ulucinar <[email protected]>
5f36810
to
0c8f87e
Compare
Signed-off-by: Alper Rifat Ulucinar <[email protected]>
Signed-off-by: Alper Rifat Ulucinar <[email protected]>
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.
Looking great, thanks @ulucinar!
Description of your changes
This PR proposes the
crddiff
tool, which is a CRD breaking API changes detection & reporting tool. Example usage is as follows:I have:
make reviewable
to ensure this PR is ready for review.backport release-x.y
labels to auto-backport this PR, as appropriate.How has this code been tested
Manually tested on example CRD base and and various revisions with the following changes: