-
Notifications
You must be signed in to change notification settings - Fork 676
✨ 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 5 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"` | ||
|
|
||
| // v1beta3 groups all the fields that will be added or modified in AWSCluster's status with the V1Beta3 version. | ||
| // +optional | ||
| V1Beta3 *AWSClusterV1Beta3Status `json:"v1beta3,omitempty"` | ||
| } | ||
|
|
||
| // AWSClusterV1Beta3Status groups all the fields that will be added or modified in AWSClusterStatus with the V1Beta3 version. | ||
| // See https://github.com/kubernetes-sigs/cluster-api/blob/main/docs/proposals/20240916-improve-status-in-CAPI-resources.md for more context. | ||
| type AWSClusterV1Beta3Status 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,22 @@ func (r *AWSCluster) SetConditions(conditions clusterv1beta1.Conditions) { | |
| r.Status.Conditions = conditions | ||
| } | ||
|
|
||
| // GetV1Beta3Conditions returns the set of conditions for this object. | ||
| func (r *AWSCluster) GetV1Beta3Conditions() []metav1.Condition { | ||
|
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. This is where the naming of things is going to get weird 😉 We need to adopt // SetV1Beta2Conditions sets conditions for an API object.
// Note: SetV1Beta2Conditions will be renamed to SetConditions in a later stage of the transition to V1Beta2.
SetV1Beta2Conditions([]metav1.Condition)
// GetV1Beta2Conditions returns the list of conditions for a cluster API object.
// Note: GetV1Beta2Conditions will be renamed to GetConditions in a later stage of the transition to V1Beta2.
GetV1Beta2Conditions() []metav1.ConditionOr we need additional functions. It will be very messy/weird until we drop the old fields.
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. thanks for clarifying 😄 it does feel a bit awkward during the transition, but i agree it’s better to align exactly with capi’s interface expectations. |
||
| if r.Status.V1Beta3 == nil { | ||
| return nil | ||
| } | ||
| return r.Status.V1Beta3.Conditions | ||
| } | ||
|
|
||
| // SetV1Beta3Conditions sets conditions for an API object. | ||
| func (r *AWSCluster) SetV1Beta3Conditions(conditions []metav1.Condition) { | ||
| if r.Status.V1Beta3 == nil { | ||
| r.Status.V1Beta3 = &AWSClusterV1Beta3Status{} | ||
| } | ||
| r.Status.V1Beta3.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!