-
Notifications
You must be signed in to change notification settings - Fork 675
✨ Add v1beta2 conditions infrastructure to AWSCluster #5854
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
base: main
Are you sure you want to change the base?
Changes from all commits
0a79592
2dc7df7
4eb8d9e
48182c1
ad57c6a
44772a3
b97102d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -281,6 +281,23 @@ type AWSClusterStatus struct { | |
| FailureDomains clusterv1beta1.FailureDomains `json:"failureDomains,omitempty"` | ||
| Bastion *Instance `json:"bastion,omitempty"` | ||
| Conditions clusterv1beta1.Conditions `json:"conditions,omitempty"` | ||
|
|
||
| // v1beta2 groups all the fields that will be added or modified in AWSCluster's status with the V1Beta2 CAPI contract version. | ||
| // +optional | ||
| V1Beta2 *AWSClusterV1Beta2Status `json:"v1beta2,omitempty"` | ||
| } | ||
|
|
||
| // AWSClusterV1Beta2Status groups all the fields that will be added or modified in AWSCluster with the V1Beta2 CAPI contract version. | ||
| // See https://github.com/kubernetes-sigs/cluster-api/blob/main/docs/proposals/20240916-improve-status-in-CAPI-resources.md for more context. | ||
| type AWSClusterV1Beta2Status struct { | ||
| // Conditions represents the observations of an AWSCluster's current state. | ||
| // Known condition types are Ready, VpcReady, SubnetsReady, InternetGatewayReady, NatGatewaysReady, | ||
| // RouteTablesReady, ClusterSecurityGroupsReady, BastionHostReady, LoadBalancerReady, and Paused. | ||
| // +optional | ||
| // +listType=map | ||
| // +listMapKey=type | ||
| // +kubebuilder:validation:MaxItems=32 | ||
| Conditions []metav1.Condition `json:"conditions,omitempty"` | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are we only dealing with AWSCluster and Conditions here?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. my bad, i was initially planning to split these into separate prs to keep things smaller,. i agree it’s better to avoid creating too many prs for closely related changes.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. rethinking again, once we settle on the approach for v1beta1 conversions, #5854 (comment) i’ll add the same
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm happy either way. Smaller PRs always helps with review though. |
||
| } | ||
|
|
||
| // S3Bucket defines a supporting S3 bucket for the cluster, currently can be optionally used for Ignition. | ||
|
|
@@ -355,6 +372,24 @@ func (r *AWSCluster) SetConditions(conditions clusterv1beta1.Conditions) { | |
| r.Status.Conditions = conditions | ||
| } | ||
|
|
||
| // GetV1Beta2Conditions returns the set of conditions for this object. | ||
| // Note: GetV1Beta2Conditions will be renamed to GetConditions in a later stage of the transition to CAPI contract V1Beta2. | ||
| func (r *AWSCluster) GetV1Beta2Conditions() []metav1.Condition { | ||
| if r.Status.V1Beta2 == nil { | ||
| return nil | ||
| } | ||
| return r.Status.V1Beta2.Conditions | ||
| } | ||
|
|
||
| // SetV1Beta2Conditions sets conditions for an API object. | ||
| // Note: SetV1Beta2Conditions will be renamed to SetConditions in a later stage of the transition to CAPI contract V1Beta2. | ||
| func (r *AWSCluster) SetV1Beta2Conditions(conditions []metav1.Condition) { | ||
| if r.Status.V1Beta2 == nil { | ||
| r.Status.V1Beta2 = &AWSClusterV1Beta2Status{} | ||
| } | ||
| r.Status.V1Beta2.Conditions = conditions | ||
| } | ||
|
|
||
| func init() { | ||
| SchemeBuilder.Register(&AWSCluster{}, &AWSClusterList{}) | ||
| } | ||
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
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 don't think we should touch v1beta1 types at all.
Only v1beta2 ones
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.
@damdo just to make sure i’m understanding correctly about the
v1beta1conversion changes, should i remove theV1Beta3restoration logic fromawscluster_conversion.go? i followed the same pattern used for existing fields likeControlPlaneLoadBalancer, but i’m totally fine removing it if that’s not the direction we want to take.wanted to confirm this part before i move on to the rest of the feedback. thanks
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.
As we are adding new fields in v1beta2 then we need to preserve roundtrip compatibility. So it will require that you update the conversions in v1beta1 like you have done here to restore the value.
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.
A lot of people make the mistake of adding the new field to old api versions as well to make the conversions work. But restoring the value from the saved state is the correct way (like you are doing).
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.
Good point! Hadn't thought about that!