Skip to content
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

Use newer v1beta1 version in API version upgrade test #4433

Merged
merged 1 commit into from Jan 9, 2024

Conversation

CecileRobertMichon
Copy link
Contributor

What type of PR is this?

/kind failing-test

What this PR does / why we need it: The k8s 1.22 images have been deprecated and removed from the Azure marketplace. Since CAPI v1.0.x only supports old k8s versions, we are no longer able to run the test as-is. This PR changes the upgrade test to run upgrade from the earliest supported minor release of CAPI and CAPZ (n-2).

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 #4422

Special notes for your reviewer:

  • cherry-pick candidate

TODOs:

  • squashed commits
  • includes documentation
  • adds unit tests

Release note:

NONE

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. kind/failing-test Categorizes issue or PR as related to a consistently or frequently failing test. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jan 4, 2024
Copy link

codecov bot commented Jan 4, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (285a75b) 61.96% compared to head (25b53a8) 61.96%.
Report is 8 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #4433   +/-   ##
=======================================
  Coverage   61.96%   61.96%           
=======================================
  Files         188      188           
  Lines       18768    18768           
=======================================
  Hits        11630    11630           
  Misses       6500     6500           
  Partials      638      638           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@nojnhuh nojnhuh left a comment

Choose a reason for hiding this comment

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

Overall LGTM.

Should we also document in the release guide to make these same changes whenever we cut a new minor release?

@nojnhuh
Copy link
Contributor

nojnhuh commented Jan 4, 2024

Should we also document in the release guide to make these same changes whenever we cut a new minor release?

I think even a link to this PR would be good enough.

@CecileRobertMichon
Copy link
Contributor Author

/hold

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 5, 2024
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jan 5, 2024
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 6, 2024
Copy link
Contributor

@nojnhuh nojnhuh left a comment

Choose a reason for hiding this comment

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

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 8, 2024
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 900689e6eb81f7cad939369e688cd9b68ed8f355

@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 8, 2024
@nojnhuh
Copy link
Contributor

nojnhuh commented Jan 8, 2024

/assign @willie-yao

Copy link
Contributor

@willie-yao willie-yao 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 pending one question!

test/e2e/config/azure-dev.yaml Show resolved Hide resolved
@CecileRobertMichon
Copy link
Contributor Author

/retest

Copy link
Contributor

@willie-yao willie-yao left a comment

Choose a reason for hiding this comment

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

/lgtm

@nojnhuh
Copy link
Contributor

nojnhuh commented Jan 8, 2024

/approve

@CecileRobertMichon please remove the hold whenever you're ready.

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: nojnhuh

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 8, 2024
Copy link
Contributor

@mboersma mboersma left a comment

Choose a reason for hiding this comment

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

/lgtm++

Thanks @CecileRobertMichon!

@nojnhuh
Copy link
Contributor

nojnhuh commented Jan 9, 2024

@CecileRobertMichon Do we need to cherry pick this to fix the test on the release branch?

@CecileRobertMichon
Copy link
Contributor Author

Do we need to cherry pick this to fix the test on the release branch?

Yes. How do we want to handle the upgrade from version on previous release branches? Should it always be n-2? In which case we might need a manual cherry-pick.

@CecileRobertMichon
Copy link
Contributor Author

/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 Jan 9, 2024
Copy link
Member

@nawazkh nawazkh left a comment

Choose a reason for hiding this comment

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

/lgtm

Comment on lines +262 to +267
InitWithKubernetesVersion: "v1.26.12",
InitWithBinary: "https://github.com/kubernetes-sigs/cluster-api/releases/download/v1.5.4/clusterctl-{OS}-{ARCH}",
InitWithCoreProvider: "cluster-api:v1.5.4",
InitWithBootstrapProviders: []string{"kubeadm:v1.5.4"},
InitWithControlPlaneProviders: []string{"kubeadm:v1.5.4"},
InitWithInfrastructureProviders: []string{"azure:v1.11.7"},
Copy link
Member

Choose a reason for hiding this comment

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

Is there a way we could fetch these values from azure-dev.yaml so that we do not maintain it going ahead? Not a blocking comment per se.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

since we're adding a second test with a different set of versions we need to configure this per test #4446

We could have two separate sets of env variables 🤔 Maybe something to follow up on in #4446 @willie-yao? Let's merge this one as-is for now to fix the broken test

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree that having two sets of env variables would be the better way to go about this. Will work on implementing it for my PR!

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. kind/failing-test Categorizes issue or PR as related to a consistently or frequently failing test. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
Status: Done
6 participants