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

[WIP] 🌱 remove separate eks providers and update quick start #5073

Closed
wants to merge 1 commit into from
Closed

[WIP] 🌱 remove separate eks providers and update quick start #5073

wants to merge 1 commit into from

Conversation

richardcase
Copy link
Member

What this PR does / why we need it:

With the graduation of the EKS functionality of CAPA, the controllers have been combined into the main AWS infrastructure provider. This change removed the separate aws-eks controlplane and bootstrap providers from clusterctl.

Also the quick start docs have been updated with a note mentioning how to disable eks support.

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 kubernetes-sigs/cluster-api-provider-aws#2656

@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. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Aug 10, 2021
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign cecilerobertmichon after the PR has been reviewed.
You can assign the PR to them by writing /assign @cecilerobertmichon 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:

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 size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Aug 10, 2021
@richardcase
Copy link
Member Author

Until CAPA 0.7.0 is available:

/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 Aug 10, 2021
@richardcase richardcase changed the title WIP: 🌱 chore: remove separate eks providers and update quick start WIP 🌱 remove separate eks providers and update quick start Aug 10, 2021
@richardcase richardcase force-pushed the remove_eks_providers branch from 5b6ca8b to a7f23b8 Compare August 10, 2021 15:09
@richardcase richardcase changed the title WIP 🌱 remove separate eks providers and update quick start [WIP] 🌱 remove separate eks providers and update quick start Aug 10, 2021
@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented Aug 10, 2021 •
edited
Loading

@richardcase: The following tests 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 a7f23b8 link /test pull-cluster-api-apidiff-main
pull-cluster-api-test-main a7f23b8 link /test pull-cluster-api-test-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.

CecileRobertMichon
CecileRobertMichon reviewed Aug 25, 2021
View reviewed changes
docs/book/src/user/quick-start.md
@@ -175,6 +175,8 @@ Download the latest binary of `clusterawsadm` from the [AWS provider releases] a

The [clusterawsadm] command line utility assists with identity and access management (IAM) for [Cluster API Provider AWS][capa].

> The AWS provider additionaly allows you to create EKS based clusters. If you don't plan to use EKS then please see the [disabling EKS documentation](https://cluster-api-aws.sigs.k8s.io/topics/eks/disabling.html) before proceeding.
Copy link
Contributor

Choose a reason for hiding this comment

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

this link returns 404

@richardcase
Copy link
Member Author

I don't think removing the providers is sufficient. Users may use clusterctl to install versions prior to 0.7.0 which still use the separate bootstrap and control plane providers.

It feels like we need the ability to specify a provider is only available until a specific version and then its deprecated. If a user tries to install a provider that is deprecated then it should be a no-op with a warning?

@sbueringer
Copy link
Member

sbueringer commented Aug 25, 2021 •
edited
Loading

I don't think removing the providers is sufficient. Users may use clusterctl to install versions prior to 0.7.0 which still use the separate bootstrap and control plane providers.

It feels like we need the ability to specify a provider is only available until a specific version and then its deprecated. If a user tries to install a provider that is deprecated then it should be a no-op with a warning?

Not sure if it helps, but clusterctl v0.4.0+ is unable to install v1alpha3 providers (you'll get an error). It can only upgrade v1alpha3 providers which have been previously installed with clusterctl v0.3.x

@richardcase
Copy link
Member Author

Created #5152 to cover provider deprecation

@randomvariable
Copy link
Member

Does this need to be updated?

@fabriziopandini
Copy link
Member

@richardcase is this PR still required?

@richardcase
Copy link
Member Author

@richardcase is this PR still required?

@fabriziopandini - it is still required. I completely forgot about it. I will get it updated in the next few days.

@sbueringer
Copy link
Member

@richardcase friendly reminder :)

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 29, 2022
@k8s-ci-robot
Copy link
Contributor

@richardcase: PR needs rebase.

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.

@richardcase
Copy link
Member Author

I'll get this updated today

@richardcase richardcase mentioned this pull request Mar 29, 2022
Merged
@richardcase
Copy link
Member Author

Closing in favour of #6344

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@CecileRobertMichon CecileRobertMichon CecileRobertMichon left review comments

@enxebre enxebre Awaiting requested review from enxebre

@sbueringer sbueringer Awaiting requested review from sbueringer

Assignees
No one assigned
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Remove eks controlplane and bootstrap providers from clusterctl
6 participants
@richardcase @k8s-ci-robot @sbueringer @randomvariable @fabriziopandini @CecileRobertMichon