Skip to content

✨ Bump CAPI to v1.3.0#1406

Merged
k8s-ci-robot merged 4 commits intokubernetes-sigs:mainfrom
mercedes-benz:tobiasgiese/bump-capi-v1.3.0
Dec 8, 2022
Merged

✨ Bump CAPI to v1.3.0#1406
k8s-ci-robot merged 4 commits intokubernetes-sigs:mainfrom
mercedes-benz:tobiasgiese/bump-capi-v1.3.0

Conversation

@tobiasgiese
Copy link
Member

@tobiasgiese tobiasgiese commented Dec 3, 2022

What this PR does / why we need 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 #1315

This PR bumps the CAPI version to v1.3.0 and respects the version migration guide.

Non-obvious ginkgo v2 changes:

Special notes for your reviewer:

  1. Please confirm that if this PR changes any image versions, then that's the sole change this PR makes.

TODOs:

  • squashed commits
  • if necessary:
    • includes documentation
    • adds unit tests

/hold

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Dec 3, 2022
@netlify
Copy link

netlify bot commented Dec 3, 2022

Deploy Preview for kubernetes-sigs-cluster-api-openstack ready!

Name Link
🔨 Latest commit 031966f
🔍 Latest deploy log https://app.netlify.com/sites/kubernetes-sigs-cluster-api-openstack/deploys/6390d20c243b9f0009046a49
😎 Deploy Preview https://deploy-preview-1406--kubernetes-sigs-cluster-api-openstack.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: tobiasgiese

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

The pull request process is described here

Details Needs approval from an approver in each of these files:

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

@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Dec 3, 2022
@tobiasgiese tobiasgiese force-pushed the tobiasgiese/bump-capi-v1.3.0 branch 6 times, most recently from 5039b0f to b4f18ad Compare December 3, 2022 12:28
@tobiasgiese tobiasgiese force-pushed the tobiasgiese/bump-capi-v1.3.0 branch 6 times, most recently from 356efa7 to e5b754c Compare December 4, 2022 16:02
@tobiasgiese
Copy link
Member Author

Waiting for #1408 to be merged

@tobiasgiese tobiasgiese force-pushed the tobiasgiese/bump-capi-v1.3.0 branch from e5b754c to 19a2546 Compare December 6, 2022 10:04
@tobiasgiese tobiasgiese changed the title [WIP] ✨ Bump CAPI to v1.3.0 ✨ Bump CAPI to v1.3.0 Dec 6, 2022
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 6, 2022
@tobiasgiese
Copy link
Member Author

Waiting for #1408 to be merged

e2e tests are green

@tobiasgiese
Copy link
Member Author

tobiasgiese commented Dec 6, 2022

Do we want to add CAPI v1.3.0 support to our v0.7 release? WDYT @mdbooth?

@tobiasgiese
Copy link
Member Author

/cherry-pick release-0.7

@k8s-infra-cherrypick-robot

@tobiasgiese: once the present PR merges, I will cherry-pick it on top of release-0.7 in a new PR and assign it to you.

Details

In response to this:

/cherry-pick release-0.7

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.

@jichenjc
Copy link
Contributor

jichenjc commented Dec 6, 2022

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 6, 2022
@tobiasgiese
Copy link
Member Author

Feel free to remove the hold @mdbooth

@seanschneeweiss
Copy link
Contributor

/lgtm
thanks a lot @tobiasgiese 👍

@tobiasgiese tobiasgiese force-pushed the tobiasgiese/bump-capi-v1.3.0 branch from 19a2546 to 642ae97 Compare December 6, 2022 16:31
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 6, 2022
@tobiasgiese tobiasgiese force-pushed the tobiasgiese/bump-capi-v1.3.0 branch 2 times, most recently from 9338897 to 76a5c22 Compare December 6, 2022 21:04
Copy link
Contributor

@mdbooth mdbooth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good.

I'm confused about the inclusion of E2E_IMAGE_URL, though. Was this broken before? If not, what is included in this PR which requires it?

CONFORMANCE_WORKER_MACHINE_COUNT: "5"
CONFORMANCE_CONTROL_PLANE_MACHINE_COUNT: "1"
INIT_WITH_KUBERNETES_VERSION: "v1.25.0"
E2E_IMAGE_URL: "http://10.0.3.15/capo-e2e-image.tar"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why was this needed?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was moved from

shared.SetEnvVar("E2E_IMAGE_URL", "http://10.0.3.15/capo-e2e-image.tar", false)

If we don't specify it in the e2e_conf.yaml the following error occurs

  [FAILED] Failed to run clusterctl config cluster
  Unexpected error:
      <*yamlprocessor.errMissingVariables | 0xc00121d740>: {
          Missing: ["E2E_IMAGE_URL"],
      }
      value for variables [E2E_IMAGE_URL] is not set. Please set the value using os environment variables or the clusterctl config file
  occurred
  In [It] at: /root/go/pkg/mod/sigs.k8s.io/cluster-api/test@v1.3.0/framework/clusterctl/client.go:302 @ 12/04/22 11:36:50.414

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member Author

@tobiasgiese tobiasgiese Dec 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As it was introduced in #1371 maybe @lentzi90 can say something about this?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But according to the comment

// Note: There isn't really anything fixing this to the v0.6 release. The test will
// simply pick the latest release with the correct contract from the E2EConfig, as seen here
// https://github.com/kubernetes-sigs/cluster-api/blob/3abb9089485f51d46054096c17ccb8b59f0f7331/test/e2e/clusterctl_upgrade.go#L265
// When new minor releases are added (with the same contract) we will need to work on this
// if we want to continue testing v0.6.

We need the e2e image to "pick the latest release with the correct contract"

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like it was a bug in the previous PR: we're including ci-hack-hc(p|t).yaml unconditionally in all the E2E tests, and they reference E2E_IMAGE_URL. However, we're only setting the environment variable in the cluster upgrade test. Presumably the undefined variable error is a change of behaviour in CAPI 1.3, or we'd have noticed this before.

@lentzi90 I wonder if we want to be consistent in the use of e2eCtx here:

shared.SetEnvVar("E2E_IMAGE_URL", "http://10.0.3.15/capo-e2e-image.tar", false)

Right now we're setting the same value in 2 places. It doesn't look like the clusterctl upgrade tests reads e2e_conf.yaml, so we likely can't remove the other one. I'd prefer to do this the same way in both places.

I don't think we need to hold up this PR, though. I'll open an issue to track it a tidy up.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, nm. It is initialised and you already fixed it 🤦

@tobiasgiese
Copy link
Member Author

I'm confused about the inclusion of E2E_IMAGE_URL, though. Was this broken before? If not, what is included in this PR which requires it?

This was just moved from the clusterctl_upgrade_test.go -- see #1406 (comment)

During this bump we must upgrade Golang to 1.19 as well.

Signed-off-by: Tobias Giese <tobias.giese@mercedes-benz.com>
Signed-off-by: Tobias Giese <tobias.giese@mercedes-benz.com>
This reverts commit 32a9fc9.

Signed-off-by: Tobias Giese <tobias.giese@mercedes-benz.com>
Signed-off-by: Tobias Giese <tobias.giese@mercedes-benz.com>
@tobiasgiese tobiasgiese force-pushed the tobiasgiese/bump-capi-v1.3.0 branch from 76a5c22 to 031966f Compare December 7, 2022 17:48
@mdbooth
Copy link
Contributor

mdbooth commented Dec 8, 2022

/lgtm
/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 8, 2022
@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 8, 2022
@k8s-ci-robot k8s-ci-robot merged commit ec27461 into kubernetes-sigs:main Dec 8, 2022
@k8s-infra-cherrypick-robot

@tobiasgiese: new pull request created: #1411

Details

In response to this:

/cherry-pick release-0.7

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.

@tobiasgiese tobiasgiese deleted the tobiasgiese/bump-capi-v1.3.0 branch December 8, 2022 17:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Migrate to Ginkgo v2 during cluster-api v1.3 bump

7 participants