✨ Use different k8s version with each ClusterctlUpgrade test #6388

SubhasmitaSw · 2022-04-07T06:04:42Z

k8s-ci-robot · 2022-04-07T06:04:51Z

Hi @SubhasmitaSw. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

k8s-ci-robot · 2022-04-07T06:05:27Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign justinsb after the PR has been reviewed.
You can assign the PR to them by writing /assign @justinsb in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

test/e2e/clusterctl_upgrade.go

killianmuldoon

Thanks for this @SubhasmitaSw! I didn't get a chance to run this locally yet - have you managed to get it working?

test/e2e/clusterctl_upgrade.go

killianmuldoon

/ok-to-test

fabriziopandini

overall lgtm, just a couple of nits

fabriziopandini · 2022-04-19T13:44:32Z

test/e2e/clusterctl_upgrade.go

@@ -63,6 +63,7 @@ type ClusterctlUpgradeSpecInput struct {
 	InitWithBinary string
 	// InitWithProvidersContract can be used to override the INIT_WITH_PROVIDERS_CONTRACT e2e config variable with a specific
 	// provider contract to use to initialise the secondary management cluster, e.g. `v1alpha3`
+	InitWithKubernetesVersion string


It seems that the comment above the field does not match the field. Also being this a new public field I think we should add a comment for it

fabriziopandini · 2022-04-19T13:46:27Z

test/e2e/clusterctl_upgrade.go

 		clusterctlBinaryURLReplacer := strings.NewReplacer("{OS}", runtime.GOOS, "{ARCH}", runtime.GOARCH)
 		clusterctlBinaryURL = clusterctlBinaryURLReplacer.Replace(clusterctlBinaryURLTemplate)
-		Expect(input.E2EConfig.Variables).To(HaveKey(initWithKubernetesVersion))
-		Expect(input.E2EConfig.Variables).To(HaveKey(KubernetesVersion))


Q: Why are we dropping the check on KubernetesVersion?

fabriziopandini · 2022-04-19T13:47:49Z

test/e2e/clusterctl_upgrade.go

+		// create InitWithKubernetesVersion field to set the init kubernetes version via struct
+		if input.InitWithKubernetesVersion == "" {
+			Expect(input.E2EConfig.Variables).To(HaveKey(initWithKubernetesVersion), "Invalid argument. %s variable must be defined when calling %s spec", initWithKubernetesVersion, specName)
+			Expect(input.E2EConfig.Variables[initWithKubernetesVersion]).ToNot(BeEmpty(), "Invalid argument. %s variable can't be empty when calling %s spec", initWithKubernetesVersion, specName)


Wondering if we should move this check after the if so we can validate input.InitWithKubernetesVersion no matter if it comes from a env variable or not

I'm a little confused here, move 'after if', is it the initWithBinary ?

I think the idea is to move l.142 to after the if/else so we just validate what input. InitWithKubernetesVersion and the validation also is applied to the result we set in the else branch

fabriziopandini · 2022-04-19T13:49:14Z

/test pull-cluster-api-e2e-full-main

sbueringer · 2022-04-26T14:43:22Z

test/e2e/clusterctl_upgrade.go

@@ -133,6 +135,15 @@ func ClusterctlUpgradeSpec(ctx context.Context, inputGetter func() ClusterctlUpg
 		} else {
 			clusterctlBinaryURLTemplate = input.InitWithBinary
 		}
+


Can we please move the entire new code block to before l.149 so it's not in the middle of the clusterctlBinaryURL calculation

sbueringer · 2022-04-26T14:49:08Z

test/e2e/clusterctl_upgrade.go

+			input.InitWithKubernetesVersion = input.E2EConfig.GetVariable(initWithKubernetesVersion)
+		} else {
+			input.InitWithKubernetesVersion = os.Getenv(initWithKubernetesVersion)
+		}
 		clusterctlBinaryURLReplacer := strings.NewReplacer("{OS}", runtime.GOOS, "{ARCH}", runtime.GOARCH)
 		clusterctlBinaryURL = clusterctlBinaryURLReplacer.Replace(clusterctlBinaryURLTemplate)
 		Expect(input.E2EConfig.Variables).To(HaveKey(initWithKubernetesVersion))


Suggested change

Expect(input.E2EConfig.Variables).To(HaveKey(initWithKubernetesVersion))

We have to drop this line here as it would fail if the init Kubernetes Version is not set via variables

sbueringer · 2022-04-26T14:50:35Z

test/e2e/clusterctl_upgrade.go

+			Expect(input.E2EConfig.Variables[initWithKubernetesVersion]).ToNot(BeEmpty(), "Invalid argument. %s variable can't be empty when calling %s spec", initWithKubernetesVersion, specName)
+			input.InitWithKubernetesVersion = input.E2EConfig.GetVariable(initWithKubernetesVersion)
+		} else {
+			input.InitWithKubernetesVersion = os.Getenv(initWithKubernetesVersion)


I don't think we need the else branch. input.E2EConfig.GetVariable does this internally

Overall the logic should be

if input.InitWithKubernetesVersion is set, don't modify it

if input.InitWithKubernetesVersion is not set get it from variables (which looks for env variables and entries in the e2e config)

sbueringer · 2022-06-14T08:06:57Z

@SubhasmitaSw Do you have time to address the findings?

SubhasmitaSw · 2022-06-15T04:11:19Z

@sbueringer I'll make this up, thanks for reminding it slipped out of my mind.

k8s-triage-robot · 2022-09-13T05:08:38Z

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

Mark this issue or PR as fresh with /remove-lifecycle stale
Mark this issue or PR as rotten with /lifecycle rotten
Close this issue or PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

sbueringer · 2022-09-19T17:24:35Z

I'll implement it as part of #7244 as I need it there

/close

k8s-ci-robot · 2022-09-19T17:24:39Z

@sbueringer: Closed this PR.

In response to this:

I'll implement it as part of #7244 as I need it there

/close

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.

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Apr 7, 2022

k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Apr 7, 2022

k8s-ci-robot requested a review from JoelSpeed April 7, 2022 06:05

k8s-ci-robot requested a review from stmcginnis April 7, 2022 06:05

killianmuldoon reviewed Apr 7, 2022

View reviewed changes

test/e2e/clusterctl_upgrade.go Outdated Show resolved Hide resolved

killianmuldoon reviewed Apr 7, 2022

View reviewed changes

test/e2e/clusterctl_upgrade.go Outdated Show resolved Hide resolved

test/e2e/clusterctl_upgrade.go Show resolved Hide resolved

killianmuldoon reviewed Apr 14, 2022

View reviewed changes

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Apr 14, 2022

fabriziopandini reviewed Apr 19, 2022

View reviewed changes

fabriziopandini changed the title ~~✨ use different k8s version with each ClusterctlUpgrade test~~ ✨ Use different k8s version with each ClusterctlUpgrade test Apr 19, 2022

use different k8s version with each ClusterctlUpgrade test

6742363

SubhasmitaSw force-pushed the issue#5704 branch from 7549563 to 6742363 Compare April 22, 2022 05:53

sbueringer reviewed Apr 26, 2022

View reviewed changes

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Sep 13, 2022

k8s-ci-robot closed this Sep 19, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ Use different k8s version with each ClusterctlUpgrade test #6388

✨ Use different k8s version with each ClusterctlUpgrade test #6388

SubhasmitaSw commented Apr 7, 2022

k8s-ci-robot commented Apr 7, 2022

k8s-ci-robot commented Apr 7, 2022

killianmuldoon left a comment

killianmuldoon left a comment

fabriziopandini left a comment

fabriziopandini Apr 19, 2022

fabriziopandini Apr 19, 2022

fabriziopandini Apr 19, 2022

SubhasmitaSw Apr 22, 2022

sbueringer Apr 26, 2022

fabriziopandini commented Apr 19, 2022

sbueringer Apr 26, 2022

sbueringer Apr 26, 2022

sbueringer Apr 26, 2022

sbueringer Apr 26, 2022

sbueringer commented Jun 14, 2022

SubhasmitaSw commented Jun 15, 2022

k8s-triage-robot commented Sep 13, 2022

sbueringer commented Sep 19, 2022

k8s-ci-robot commented Sep 19, 2022

✨ Use different k8s version with each ClusterctlUpgrade test #6388

✨ Use different k8s version with each ClusterctlUpgrade test #6388

Conversation

SubhasmitaSw commented Apr 7, 2022

k8s-ci-robot commented Apr 7, 2022

k8s-ci-robot commented Apr 7, 2022

killianmuldoon left a comment

Choose a reason for hiding this comment

killianmuldoon left a comment

Choose a reason for hiding this comment

fabriziopandini left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabriziopandini commented Apr 19, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbueringer commented Jun 14, 2022

SubhasmitaSw commented Jun 15, 2022

k8s-triage-robot commented Sep 13, 2022

sbueringer commented Sep 19, 2022

k8s-ci-robot commented Sep 19, 2022