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.

Sign up for GitHub

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

Jump to bottom

⚠️ Remove v1alpha3 API Version #8997

Merged
merged 1 commit into from
Aug 31, 2023
Merged

⚠️ Remove v1alpha3 API Version #8997

merged 1 commit into from
Aug 31, 2023

Conversation

killianmuldoon
Copy link
Contributor

Remove v1alpha3 versions from across the Cluster API codebase.

/hold

The intention isn't to remove this until some time after the 1.5 release. Just opened this PR to get a sense of the size and the problems it might surface.

@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 Jul 12, 2023
@k8s-ci-robot k8s-ci-robot requested review from chrischdi and elmiko July 12, 2023 18:08
@k8s-ci-robot k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Jul 12, 2023
@kubernetes-sigs kubernetes-sigs deleted a comment from k8s-ci-robot Jul 12, 2023
@killianmuldoon
Copy link
Contributor Author

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

@killianmuldoon killianmuldoon mentioned this pull request Jul 13, 2023
Closed
18 tasks
@killianmuldoon
Copy link
Contributor Author

/retest

sbueringer
sbueringer reviewed Jul 19, 2023
View reviewed changes
Copy link
Member

@sbueringer sbueringer left a comment

Choose a reason for hiding this comment

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

Just a high-level review. Looks good so far!

test/e2e/clusterctl_upgrade.go Outdated Show resolved Hide resolved
metadata.yaml Show resolved Hide resolved
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 22, 2023
@killianmuldoon killianmuldoon changed the title [WIP] ⚠️ Remove v1alpha3 API Version ⚠️ Remove v1alpha3 API Version Aug 29, 2023
@killianmuldoon
Copy link
Contributor Author

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

@k8s-ci-robot k8s-ci-robot removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Aug 29, 2023
@killianmuldoon
Copy link
Contributor Author

/retest

@killianmuldoon
Copy link
Contributor Author

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

@cprivitere
Copy link
Member

cprivitere commented Aug 30, 2023 •
edited
Loading

In the meeting today, Vince suggested adding a check to clusterctl to tell users if they try to install a provider trying to use v1alpha3 (just wanted this to get written down somewhere, i'm aware this is probably a separate issue/PR).

@killianmuldoon
Copy link
Contributor Author

Today if we try to install versions on the old contract clusterctl init will fail with a message like: current version of clusterctl is only compatible with v1beta1 providers, detected v1alpha4 for provider cluster-api so this is already covered as long as providers are setting the contract versions correctly.

sbueringer
sbueringer reviewed Aug 30, 2023
View reviewed changes
Copy link
Member

@sbueringer sbueringer left a comment

Choose a reason for hiding this comment

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

Just some smaller findings

cmd/clusterctl/client/cluster/upgrader.go
@@ -352,6 +349,7 @@ func (u *providerUpgrader) getUpgradeComponents(ctx context.Context, provider Up

func (u *providerUpgrader) doUpgrade(ctx context.Context, upgradePlan *UpgradePlan, opts UpgradeOptions) error {
// Check for multiple instances of the same provider if current contract is v1alpha3.
// TODO(killianmuldoon) Assess if we can remove this piece of code.
Copy link
Member

@sbueringer sbueringer Aug 30, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Based on the link to the docs in CheckSingleProviderInstance I assume this check should be for >= v1alpha3. So basically for all versions that we support upgrading today (?)

(xref: https://cluster-api.sigs.k8s.io/developer/architecture/controllers/support-multiple-instances.html)

Let's circle back with @fabriziopandini next week before we change anything here though (and merge the PR independent of that)

controlplane/kubeadm/api/v1alpha4/conversion_test.go Show resolved Hide resolved
test/framework/clusterctl/client.go Show resolved Hide resolved
bootstrap/kubeadm/api/v1alpha3/condition_consts.go Show resolved Hide resolved
test/infrastructure/docker/main.go Show resolved Hide resolved
elmiko
elmiko reviewed Aug 30, 2023
View reviewed changes
Copy link
Contributor

@elmiko elmiko left a comment

Choose a reason for hiding this comment

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

generally looks good to me

@killianmuldoon
Remove v1alpha3 apiVersion
a64d0b1 
Signed-off-by: killianmuldoon <kmuldoon@vmware.com>
@k8s-ci-robot
Copy link
Contributor

@killianmuldoon: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
pull-cluster-api-apidiff-main a64d0b1 link false /test pull-cluster-api-apidiff-main

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

@killianmuldoon killianmuldoon mentioned this pull request Aug 31, 2023
Merged
@sbueringer
Copy link
Member

Thx!

/lgtm
/approve

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

LGTM label has been added.

Git tree hash: 454582da5fb54c731df06f57537c23b37240ee83

@k8s-ci-robot
Copy link
Contributor

[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 /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 Aug 31, 2023
@killianmuldoon
Copy link
Contributor Author

@sbueringer WDYT about removing hold on this one?

@sbueringer
Copy link
Member

I didn't notice that there is a hold on it :D

/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 Aug 31, 2023
@k8s-ci-robot k8s-ci-robot merged commit b3e0615 into kubernetes-sigs:main Aug 31, 2023
@k8s-ci-robot k8s-ci-robot added this to the v1.6 milestone Aug 31, 2023
@g-gaston
Copy link
Contributor

/area api

@k8s-ci-robot k8s-ci-robot added the area/api Issues or PRs related to the APIs label Oct 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sbueringer sbueringer sbueringer left review comments

@elmiko elmiko elmiko left review comments

@chrischdi chrischdi Awaiting requested review from chrischdi

Assignees

@sbueringer sbueringer

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/api Issues or PRs related to the APIs 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
Milestone
v1.6
Development

Successfully merging this pull request may close these issues.
6 participants
@killianmuldoon @cprivitere @k8s-ci-robot @sbueringer @g-gaston @elmiko