🌱 Add back flavor to Waiter interface #9166
Conversation
4b45420 to
c28181c
Compare
c28181c to
f9f44f1
Compare
|
LGTM label has been added. Git tree hash: 3ada2b0252d0874bcfa1750e37f85e6f87630086
|
| @@ -333,6 +334,7 @@ type ApplyCustomClusterTemplateAndWaitInput struct { | |||
| ClusterName string | |||
There was a problem hiding this comment.
@CecileRobertMichon I could be mixing up some thing. But why are you moving from ApplyClusterTemplateAndWaitInput to ApplyCustomClusterTemplateAndWaitInput in CAPZ?
/hold
There was a problem hiding this comment.
xref: kubernetes-sigs/cluster-api-provider-azure#3707 (comment)
(Not sure if you're referring to this)
There was a problem hiding this comment.
Sorry, now understood that you had to align because you use the interface. Should have seen it the first time
There was a problem hiding this comment.
I'll answer in CAPZ. The link to the CAPZ conversation didn't work the first time for me
There was a problem hiding this comment.
@sbueringer we're not moving from one to the other, we're implementing the Waiter interface https://github.com/kubernetes-sigs/cluster-api/blob/main/test/framework/clusterctl/clusterctl_helpers.go#L227 as part of ControlPlaneWaiters. I don't understand if you're saying there's another way around this?
There was a problem hiding this comment.
I'm basically wondering if you have to move to the ApplyCustomClusterTemplateAndWaitInput struct in all cases where you did (because not all of them are for the Waiters interface). And then if in the cases you had to move you require the flavor
I'm not very familiar with the CAPZ code, I'm just seeing that this input struct is passed around in a lot of places
There was a problem hiding this comment.
Okay took a closer look and saw that all starts with EnsureControlPlaneInitialized which takes the input struct and then calls InstallCalicoAndCloudProviderAzureHelmChart where the flavor is needed
There was a problem hiding this comment.
I think there are ways around it basically with closure, but looking at how you use the flavor it seems reasonable to require the flavor in this struct. So I would say let's add it
|
/lgtm /hold cancel /assign @fabriziopandini |
There was a problem hiding this comment.
/lgtm
@sbueringer happy to approve this one unless you specifically want @fabriziopandini to take a look.
|
I think we can go ahead /approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: sbueringer 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 |
|
/cherry-pick release-1.5 |
|
@sbueringer: once the present PR merges, I will cherry-pick it on top of release-1.5 in a new PR and assign it to you. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
@sbueringer: new pull request created: #9177 In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
What this PR does / why we need it: As part of caaa744#diff-7084fa27f2b10b8d2472295c14187d21ea186aae39efa12e051f340562a4cca4, the
Waiterinterface input type in the e2e test framework was changed fromApplyClusterTemplateAndWaitInputtoApplyCustomClusterTemplateAndWaitInput, however this is breaking for providers that use theWaiterinterface outside of scale.go.When bumping CAPI to v1.5.0 in CAPZ, we ran into an issue where
Flavoris no longer available from that input type (see here).This PR adds
Flavorto the new struct which should have no negative impacts for tests that don't use it but allows tests who need it to keep using it.Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when PR gets merged):Fixes #
/area e2e-testing