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 v1alpha5 API Version #1525

Merged
merged 1 commit into from
Mar 20, 2024
Merged

⚠️ Remove v1alpha5 API Version #1525

merged 1 commit into from
Mar 20, 2024

Conversation

kashifest
Copy link
Member

Remove the v1alpha5 version across the CAPM3 codebase.
Related to #971

@metal3-io-bot metal3-io-bot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Mar 13, 2024
@kashifest
Copy link
Member Author

/test-centos-e2e-integration-main
/test-ubuntu-integration-main
/test-ubuntu-feature-main

@kashifest
Copy link
Member Author

/test-centos-e2e-integration-main
/test-ubuntu-integration-main
/test-ubuntu-feature-main

@adilGhaffarDev @mboukhalfa which other tests should I run ?

@kashifest
Copy link
Member Author

/test-ubuntu-features-main

@kashifest
Copy link
Member Author

/test-ubuntu-e2e-feature-main

@mboukhalfa
Copy link
Member

/test-e2e-upgrade-main-from-release-1-6

@mboukhalfa
Copy link
Member

seems not triggered 🙅

@kashifest
Copy link
Member Author

/hold

There are more places in e2e where we need to drop alpha api

@metal3-io-bot metal3-io-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 13, 2024
@adilGhaffarDev
Copy link
Member

If we want to add the new clusterctl upgrade tests(v0.3=>v1.5=>current) that CAPI has added, we should keep these APIs in our code.
ref: kubernetes-sigs/cluster-api#10147

@kashifest
Copy link
Member Author

If we want to add the new clusterctl upgrade tests(v0.3=>v1.5=>current) that CAPI has added, we should keep these APIs in our code. ref: kubernetes-sigs/cluster-api#10147

ok interesting, I was following this, kubernetes-sigs/cluster-api#9939. Are you sure that we are going to test (v0.3=>v1.5=>current) we dont have 0.3 APIs either, perhaps you meant (v0.4=>v1.5=>current) for CAPI and (v0.5=>v1.5=>current) for us? But then, I am also thinking didnt we decide to drop upgrade from alpha versions ?

The reason I started to work on this now is when I bump to CAPI v1.7.0-beta.0 , I am getting the following error

sigs.k8s.io/cluster-api/api/v1alpha4: package sigs.k8s.io/cluster-api/api/v1alpha4 provided by sigs.k8s.io/cluster-api at latest version v1.6.3 but not at required version v1.7.0-beta.0

@adilGhaffarDev
Copy link
Member

ok interesting, I was following this, kubernetes-sigs/cluster-api#9939. Are you sure that we are going to test (v0.3=>v1.5=>current) we dont have 0.3 APIs either, perhaps you meant (v0.4=>v1.5=>current) for CAPI and (v0.5=>v1.5=>current) for us? But then, I am also thinking didnt we decide to drop upgrade from alpha versions ?

In CAPI they moved alpha APIs to an internal folder, they removed it from the API folder that is on root.
Also, CAPI didn't ask providers to add these tests, they only asked to uplift, so we are fine. These tests are running in CAPI already and the edge case where the upgrade fails is already covered there we don't need to test it on our side. We can remove alpha APIs for now. Sorry for the confusion.

@kashifest
Copy link
Member Author

/test-centos-e2e-integration-main
/test-ubuntu-integration-main
/test-ubuntu-feature-main

1 similar comment
@kashifest
Copy link
Member Author

/test-centos-e2e-integration-main
/test-ubuntu-integration-main
/test-ubuntu-feature-main

@kashifest
Copy link
Member Author

/test-ubuntu-e2e-feature-main

@kashifest
Copy link
Member Author

/test-centos-e2e-integration-main

@kashifest
Copy link
Member Author

/test-centos-e2e-integration-main

1 similar comment
@kashifest
Copy link
Member Author

/test-centos-e2e-integration-main

@kashifest
Copy link
Member Author

/test-ubuntu-e2e-integration-main

@kashifest
Copy link
Member Author

/test-ubuntu-integration-main
/test-ubuntu-e2e-integration-main
/test-ubuntu-e2e-feature-main

@kashifest
Copy link
Member Author

/test-ubuntu-e2e-feature-main

@kashifest
Copy link
Member Author

/test-ubuntu-integration-main

@kashifest
Copy link
Member Author

/test-e2e-upgrade-main-from-release-1-6

@kashifest
Copy link
Member Author

/test-e2e-upgrade-main-from-release-1-6
/test-e2e-upgrade-main-from-release-1-5
/test-e2e-upgrade-main-from-release-1-4
/test-e2e-upgrade-main-from-release-1-3
/test-e2e-upgrade-main-from-release-1-2

@kashifest
Copy link
Member Author

/test-ubuntu-integration-main

@kashifest
Copy link
Member Author

/test-e2e-upgrade-main-from-release-1-6

@kashifest
Copy link
Member Author

/override test-ubuntu-e2e-feature-main
ubuntu feature test is failing for unrrelated reason, centos has passed

@metal3-io-bot
Copy link
Contributor

@kashifest: Overrode contexts on behalf of kashifest: test-ubuntu-e2e-feature-main

In response to this:

/override test-ubuntu-e2e-feature-main
ubuntu feature test is failing for unrrelated reason, centos has passed

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.

@kashifest
Copy link
Member Author

/hold cancel

@metal3-io-bot metal3-io-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 18, 2024
@kashifest
Copy link
Member Author

/cc @furkatgofurov7

@kashifest kashifest mentioned this pull request Mar 18, 2024
Merged
@metal3-io-bot metal3-io-bot added the needs-rebase Indicates that a PR cannot be merged because it has merge conflicts with HEAD. label Mar 18, 2024
@kashifest
Copy link
Member Author

Passed tests:

@kashifest
Remove v1alpha5 API Version
79d1d93 
Signed-off-by: Kashif Khan <kashif.khan@est.tech>
@metal3-io-bot metal3-io-bot removed the needs-rebase Indicates that a PR cannot be merged because it has merge conflicts with HEAD. label Mar 19, 2024
@kashifest
Copy link
Member Author

/test-centos-e2e-integration-main
/test-ubuntu-integration-main

1 similar comment
@kashifest
Copy link
Member Author

/test-centos-e2e-integration-main
/test-ubuntu-integration-main

@kashifest
Copy link
Member Author

/cc @mboukhalfa @Sunnatillo @adilGhaffarDev @lentzi90 @tuminoid

mboukhalfa
mboukhalfa reviewed Mar 19, 2024
View reviewed changes
test/e2e/upgrade_clusterctl_test.go
Comment on lines +277 to +278
os.Setenv("CAPI_VERSION", "v1beta1")
os.Setenv("CAPM3_VERSION", "v1beta1")
Copy link
Member

Choose a reason for hiding this comment

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

Wondering if this needed I think in the previous we were overriding the v1beta1 with v1alpha5

Copy link
Member

Choose a reason for hiding this comment

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

also overriding the format is in line 283

Copy link
Member Author

Choose a reason for hiding this comment

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

I dont think I understood your 2nd comment , if these vars are not needed, we need to do cleanup in a followup.

Copy link
Member

Choose a reason for hiding this comment

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

ok

furkatgofurov7
furkatgofurov7 reviewed Mar 19, 2024
View reviewed changes
test/e2e/config/e2e_conf.yaml
@@ -39,7 +39,7 @@ providers:
- name: kubeadm
type: BootstrapProvider
versions:
- name: ${CAPI_FROM_RELEASE} # latest published release in the v1alpha4 series; this is used for v1alpha4 --> v1beta1 clusterctl upgrades test only.
- name: ${CAPI_FROM_RELEASE}
Copy link
Member

@furkatgofurov7 furkatgofurov7 Mar 19, 2024 •
edited

Choose a reason for hiding this comment

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

I can't directly comment on the line, but what I am not understanding is why this is so much complicated rather then just using hardcoded values for CAPI_FROM and CAP_TO releases. That is not my main question though, but it is this line:

files:
    - sourcePath: "../data/shared/${CAPI_FROM_RELEASE:0:4}/metadata.yaml"

how it used to work AND will work as is? you have v0.4, v1.2, v1.3, v1.4, v1.5, v1.6 folders under data/shared folders and script does sub string extraction to set CONTRACT_FROM, but how they are related to each other? I mean v0.4 is not the contract but CAPM3 minor version, contract should be v1alpha4/v1beta1, no?

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree this is hard to follow and confusing as well. We have another PR where we are hardcoding these values for simplifications, #1465, as for how it works, @mboukhalfa can clarify more, we are exporting CAPI_TO_RELEASE inherits its value from CAPIRELEASE which is set in dev-env. I think I have to also do a cleanup of the line you pointed out.

Copy link
Member

Choose a reason for hiding this comment

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

that line is extract the the minor release and removing the patch chars to set the correct path based on the release exported in dev-env

@mboukhalfa
Copy link
Member

/approve

@metal3-io-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mboukhalfa

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

@metal3-io-bot metal3-io-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 20, 2024
Sunnatillo
Sunnatillo reviewed Mar 20, 2024
View reviewed changes
Copy link
Member

@Sunnatillo Sunnatillo left a comment

Choose a reason for hiding this comment

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

/lgtm

@metal3-io-bot metal3-io-bot added the lgtm Indicates that a PR is ready to be merged. label Mar 20, 2024
@metal3-io-bot metal3-io-bot merged commit bec1704 into metal3-io:main Mar 20, 2024
19 checks passed
@metal3-io-bot metal3-io-bot deleted the remove/v1alpha5 branch March 20, 2024 12:27
@kashifest kashifest mentioned this pull request May 8, 2024
Closed
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mboukhalfa mboukhalfa mboukhalfa left review comments

@Sunnatillo Sunnatillo Sunnatillo left review comments

@furkatgofurov7 furkatgofurov7 furkatgofurov7 left review comments

@adilGhaffarDev adilGhaffarDev Awaiting requested review from adilGhaffarDev

@lentzi90 lentzi90 Awaiting requested review from lentzi90

@tuminoid tuminoid Awaiting requested review from tuminoid

Assignees
No one assigned
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm 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
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@kashifest @mboukhalfa @adilGhaffarDev @metal3-io-bot @Sunnatillo @furkatgofurov7