Skip to content

Updating ClusterProperty API to v1betav1 #27

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
May 13, 2025
Merged

Updating ClusterProperty API to v1betav1 #27

merged 1 commit into from
May 13, 2025
+273 −0

Conversation

holgerson97
Copy link
Contributor

@holgerson97 holgerson97 commented Oct 21, 2024 •
edited
Loading

closes: #18

I upgrade the API files and adapted Makefile to support multiple CRD versions.
Is there more required to do to close issue #18?

NOTE Since I updated controller-gen in a different PR this one should probably merged afterwards and manifest freshly generated with new version.

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 21, 2024
@ryanzhang-oss
Copy link

I am not sure if the About API is ready to move to v1beta1 yet. There are a bunch of properties that we would like to add.

@lauralorenz
Copy link
Contributor

Thank you @holgerson97!!!! This is awesome!! I will catch up on these after Kubecon.

@ryanzhang-oss About API graduated to beta last year actually, but the repo hasn't caught up. Adding properties (if by that you mean, more records) is always possible; and well known properties can also be defined if that's what you mean.

@olivercodes olivercodes mentioned this pull request Dec 13, 2024
Merged
@lauralorenz
Copy link
Contributor

Let's do a rebase and regen soon since #26 is in and once #25 is in too

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 31, 2025
@cainelli cainelli mentioned this pull request Mar 18, 2025
Open
@JeremyOT
Copy link
Member

@holgerson97 - now that #25 is merged, if you rebase I'll push this one through.

Thanks!

@holgerson97 holgerson97 force-pushed the chore/bump_crd_v1beta1 branch from 5be1dc8 to cfc8e62 Compare March 21, 2025 07:10
@k8s-ci-robot k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 21, 2025
@holgerson97
Added CRD Variable to enable multiple version for ClusterProperty CRD
2ccf420 
Added API for v1beta1

Added StorageVersion and VersionName

Generated new CRD Manifest

Fixed Setting StorageVersion on false API Version
@holgerson97 holgerson97 reopened this Mar 21, 2025
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Mar 21, 2025
@holgerson97
Copy link
Contributor Author

holgerson97 commented Mar 21, 2025 •
edited
Loading

Squashed and rebased.
I always get the above notification saying I approved the changes, what do I need to do in order to not "approve" my PRs?

ryanzhang-oss
ryanzhang-oss reviewed Apr 21, 2025
View reviewed changes
clusterproperty/api/v1beta1/clusterproperty_types.go
Comment on lines +28 to +29
// INSERT ADDITIONAL SPEC FIELDS - desired state of cluster
// Important: Run "make" to regenerate code after modifying this file
Copy link

Choose a reason for hiding this comment

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

why leave those behind if we don't plan to add more field?

clusterproperty/api/v1beta1/clusterproperty_types.go
Comment on lines +23 to +24
// EDIT THIS FILE! THIS IS SCAFFOLDING FOR YOU TO OWN!
// NOTE: json tags are required. Any new fields you add must have json tags for the fields to be serialized.
Copy link

Choose a reason for hiding this comment

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

Those seem to be reminiscent of kubebuilder scaffolding, any reason to keep them?

@lauralorenz
Copy link
Contributor

Triage notes: API users and lauralorenz@ for lgtm, @jermeyot for approve. Interested in merging and then opening new PR if there are specific things to clean up.

@JeremyOT
Copy link
Member

Merging to keep things moving. @ryanzhang-oss I recommend a follow up PR for any scaffolding cleanup

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 13, 2025
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: holgerson97, JeremyOT

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 13, 2025
@k8s-ci-robot k8s-ci-robot merged commit da5947e into kubernetes-sigs:master May 13, 2025
2 checks passed
@qiujian16
Copy link
Contributor

cc @RainbowMango
thanks for upgrading to v1beta1, but I thought we will keep the api in pkg/apis and generate client from there?
For our use case, we will need the typed client.

@RainbowMango
Copy link
Member

Yes, seems we need another PR for the client thing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ryanzhang-oss ryanzhang-oss ryanzhang-oss left review comments

@JeremyOT JeremyOT Awaiting requested review from JeremyOT

@lauralorenz lauralorenz Awaiting requested review from lauralorenz

Assignees

@JeremyOT JeremyOT

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Update CRD to beta maturity
7 participants
@holgerson97 @ryanzhang-oss @lauralorenz @JeremyOT @k8s-ci-robot @qiujian16 @RainbowMango