-
Notifications
You must be signed in to change notification settings - Fork 208
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: align GCPManagedControlPlane with CAPI >=v1.9 #1434
base: main
Are you sure you want to change the base?
feat: align GCPManagedControlPlane with CAPI >=v1.9 #1434
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: salasberryfin The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
✅ Deploy Preview for kubernetes-sigs-cluster-api-gcp ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Signed-off-by: Carlos Salas <carlos.salas@suse.com>
bf2c4da
to
153f13f
Compare
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.
Hey @salasberryfin
Thanks a lot for working on implementing this!
Mostly LGTM, left a couple of things we need to change though.
// +optional | ||
// | ||
// Deprecated: This field will soon be removed and you are expected to use Version instead. |
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 think the optional stylistically (and maybe even from an implementation standpoint) goes next to the field, whereas the Deprecated can also go above in the godoc comment.
Better be safe, example here:
https://github.com/kubernetes-sigs/cluster-api/blob/7a71f4b0ed71cb92fa751e48057df6bb672319bb/api/v1beta1/cluster_types.go#L533-L539
// CurrentVersion shows the current version of the GKE control plane. | ||
// +optional | ||
// | ||
// Deprecated: This field will soon be removed and you are expected to use Version instead. |
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.
Same here
|
||
// Version represents the version of the GKE control plane. | ||
// +optional | ||
Version *string `json:"version,omitempty"` | ||
} |
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.
We want to populate this status.Version
with whatever we currently populate the status.CurrentVersion
with.
Then When in the future we remove the status.CurrentVersion
, we will only have the status.Version
to set.
One instance I saw of this is here:
s.scope.GCPManagedControlPlane.Status.CurrentVersion = convertToSdkMasterVersion(cluster.GetCurrentMasterVersion()) |
But there may be others.
@salasberryfin Also we will likely need to add an cc. @richardcase |
@salasberryfin also we might need to silence the linter for errors like : |
/kind bug
What this PR does / why we need it:
To align
GCPManagedControlPlane
specification and status with CAPI >=v1.9 we need to introduce new control plane version fields and set existing ones for deprecation in coming releases. The list of changes, as agreed on Slack (thanks @damdo @richardcase @cpanato):Version
fields inGCPManagedControlPlaneSpec
andGCPManagedControlPlaneStatus
as per the updated CAPI contract for control planes.ControlPlaneVersion
andCurrentVersion
as deprecated with a message saying to useVersion
instead.Version
andControlPlaneVersion
.Version
.Which issue(s) this PR fixes:
Fixes #1433
Special notes for your reviewer:
In the future, as the deprecated field is ultimately removed, we can add a conversion from
ControlPlaneVersion
toVersion
.TODOs:
Release note: