Skip to content
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

feat: bump up CRD version to v1beta1 #664

Merged
merged 10 commits into from
Mar 9, 2023

Conversation

binbin-li
Copy link
Collaborator

@binbin-li binbin-li commented Feb 21, 2023

Description

What this PR does / why we need it:

Update CRD version from v1alpha1 to v1beta1.

Basically I followed this example, API conversion guide and this example to make the upgrade. For the hub-spoke conversion code, I use conversion-gen to auto-generate it.

Added a doc listing the steps of the upgrade.

Steps for the upgrade:

  1. Create new APIs version by: kubebuilder create api --group config.ratify.deislabs.io --version v1beta1 --kind <kind>
  2. Copy over existing types.
  3. Create an unversioned API which will become the hub between v1alpha1 and v1beta1 to faciliate conversion. When we add a new V1 version, it would be convenient to generate conversion functions between v1 and unversioned.
  4. Copy over existing types to unversioned API.
  5. In each "spoke" version package(v1alpha1, v1beta1), add marker +k8s:conversion-gen directive pointing to the “hub” version package. It must be in doc.go.
  6. In “spoke” version package, add a localSchemeBuilder = runtime.NewSchemeBuilder(SchemeBuilder.AddToScheme) in groupversion_info.go so the auto-generated code would compile.
  7. In unversioned package, create doc.go file and add marker: +kubebuilder:object:generate=true
  8. Add +kubebuilder:skip to unversioned api.
  9. Mark v1beta1 as the storage version, add marker +kubebuilder:storageversion to v1beta1.
  10. run make manifests generate to generate CRD objects, DeepCopy and conversion methods.

Notes:
in this pr, we didn't make any changes to the CRD properties. In the future, if we have incompatible changes, we would have to Add {kind}_conversion.go in each version to implement Convertible for each type.

Which issue(s) this PR fixes (optional, using fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when the PR gets merged):

Fixes #602

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

Test passed. Tested locally with v1alpha1 CR as well.

Checklist:

  • Does the affected code have corresponding tests?
  • Are the changes documented, not just with inline documentation, but also with conceptual documentation such as an overview of a new feature, or task-based documentation like a tutorial? Consider if this change should be announced on your project blog.
  • Does this introduce breaking changes that would require an announcement or bumping the major version?
  • Do all new files have appropriate license header?

@binbin-li binbin-li changed the title chore: bump up CRD version to v1beta1 feat: bump up CRD version to v1beta1 Feb 21, 2023
@codecov
Copy link

codecov bot commented Feb 21, 2023

Codecov Report

Patch coverage: 41.66% and no project coverage change

Comparison is base (7cf6951) 44.51% compared to head (c3e6fad) 44.51%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #664   +/-   ##
=======================================
  Coverage   44.51%   44.51%           
=======================================
  Files          56       56           
  Lines        3266     3266           
=======================================
  Hits         1454     1454           
  Misses       1664     1664           
  Partials      148      148           
Impacted Files Coverage Δ
pkg/controllers/certificatestore_controller.go 12.50% <25.00%> (ø)
pkg/controllers/store_controller.go 35.59% <50.00%> (ø)
pkg/controllers/verifier_controller.go 37.09% <50.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@sozercan
Copy link
Contributor

instead of this being a breaking change, you should consider creating a seperate v1beta1 api and converting between different versions https://book.kubebuilder.io/multiversion-tutorial/tutorial.html
example: eraser-dev/eraser#544

@binbin-li binbin-li marked this pull request as draft February 27, 2023 03:39
@binbin-li binbin-li changed the title feat: bump up CRD version to v1beta1 [WIP] feat: bump up CRD version to v1beta1 Feb 27, 2023
@binbin-li binbin-li marked this pull request as ready for review March 1, 2023 06:34
@binbin-li binbin-li changed the title [WIP] feat: bump up CRD version to v1beta1 feat: bump up CRD version to v1beta1 Mar 1, 2023
Copy link
Collaborator

@susanshi susanshi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for the PR binbin. Left some questions

@binbin-li binbin-li force-pushed the update-crd-version branch 2 times, most recently from b396290 to 7db663c Compare March 2, 2023 06:30
@binbin-li binbin-li force-pushed the update-crd-version branch 3 times, most recently from f656593 to a54990d Compare March 2, 2023 07:34
Complete(r)
}

// Creates a store reference from CRD spec and add store to map
func storeAddOrReplace(spec configv1alpha1.StoreSpec, fullname string) error {
func storeAddOrReplace(spec configv1beta1.StoreSpec, fullname string) error {
Copy link
Contributor

@sozercan sozercan Mar 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i don't see usage of unversioned. if it's not a specific Kubernetes API reference, you should use unversioned API

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually in controller, the Store object is retrieved from K8s Cluster which should be a valid version. https://github.com/deislabs/ratify/blob/main/pkg/controllers/store_controller.go#L64
So I use the v1beta1 version across the controller file, for other files we should definitely use unversioned API but there is no usage yet.
And for this specific function, we could probably do a manual conversion from v1beta1 to unversioned once we retrieved object from the cluster.

@binbin-li binbin-li merged commit fd211bf into ratify-project:main Mar 9, 2023
@susanshi susanshi mentioned this pull request Apr 7, 2023
10 tasks
@kevin85421 kevin85421 mentioned this pull request Jun 1, 2023
4 tasks
bspaans pushed a commit to bspaans/ratify that referenced this pull request Oct 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Upgrade CRD version from alpha to beta
4 participants