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

📖 CAEP: Clusterctl redesign #1558

Merged
merged 1 commit into from
Nov 6, 2019
Merged

📖 CAEP: Clusterctl redesign #1558

merged 1 commit into from
Nov 6, 2019

Conversation

fabriziopandini
Copy link
Member

What this PR does / why we need it:
This PR adds the clusterctl redesign proposal

/assign @timothysc @frapposelli
/cc @ncdc @vincepri @detiber

@k8s-ci-robot
Copy link
Contributor

Welcome @fabriziopandini!

It looks like this is your first PR to kubernetes-sigs/cluster-api 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/cluster-api has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 16, 2019
@fabriziopandini fabriziopandini changed the title 📖 clusterctl redesign proposal 📖 clusterctl redesign proposal Oct 16, 2019
@vincepri vincepri added this to the v0.3.0 milestone Oct 16, 2019
detiber
detiber reviewed Oct 16, 2019
View reviewed changes
Copy link
Member

@detiber detiber left a comment

Choose a reason for hiding this comment

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

A couple of minor comments, otherwise lgtm

docs/proposals/20191016-clusterctl-redesign.md Outdated Show resolved Hide resolved
docs/proposals/20191016-clusterctl-redesign.md Outdated Show resolved Hide resolved
@vincepri vincepri self-assigned this Oct 16, 2019
frapposelli
frapposelli reviewed Oct 16, 2019
View reviewed changes
Copy link
Member

@frapposelli frapposelli left a comment

Choose a reason for hiding this comment

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

Besides one minor nit it LGTM

vincepri
vincepri approved these changes Oct 17, 2019
View reviewed changes
Copy link
Member

@vincepri vincepri left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Gave a pretty good read at this proposal, it goes in the right direction and it's a great start.

/approve

docs/proposals/20191016-clusterctl-redesign.md Outdated
1. Is based on a list of pre-configured provider repositories; it is possible to add new provider configurations to the list of known providers by using the clusterctl config file (see day 2 operations for more details about this).
2. Assumes init will replace env variables defined in the components yaml read from provider repositories, or error out if such variables are missing.
3. Assumes providers will be installed in the namespace defined in the yaml for installing components; at the time of this writing, this leads to having each provider in a dedicated namespace. This could be customized by specifying the --target-namespace flag.
4. Assumes to not change the namespace each provider is watching on objects (settings defined in the yaml for installing components will be used); at the time of this writing, this leads to having each provider watching objects in all namespaces. Pending [1490](https://github.com/kubernetes-sigs/cluster-api/issues/1490) issue addressed, this could be customized by specifying the --watching-namespace flag.
Copy link
Member

Choose a reason for hiding this comment

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

If I understood this correctly, this is effectively saying that we assume that each manager (from CAPI, CAPA, CABPK) can only watch all namespaces for now? If so, we should make it more clear.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done. please let me know if the new version is more clear.

docs/proposals/20191016-clusterctl-redesign.md Show resolved Hide resolved
docs/proposals/20191016-clusterctl-redesign.md Outdated Show resolved Hide resolved
docs/proposals/20191016-clusterctl-redesign.md Outdated Show resolved Hide resolved
docs/proposals/20191016-clusterctl-redesign.md Outdated Show resolved Hide resolved
@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 17, 2019
@ncdc ncdc mentioned this pull request Oct 17, 2019
Merged
@andrewsykim
Copy link
Member

/assign

@vincepri vincepri changed the title 📖 clusterctl redesign proposal 📖 CAEP: Clusterctl redesign Oct 18, 2019
@vincepri
Copy link
Member

/kind proposal

@k8s-ci-robot k8s-ci-robot added the kind/proposal Issues or PRs related to proposals. label Oct 21, 2019
@ncdc
Copy link
Contributor

ncdc commented Oct 22, 2019

Removing auto-approve so reviewers can manually indicate approval when it's ready.

/approve cancel

@ncdc
Copy link
Contributor

ncdc commented Oct 22, 2019

/approve cancel

@ncdc ncdc removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 22, 2019
@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 23, 2019
@fabriziopandini
Copy link
Member Author

/approve cancel

as per @ncdc comment

pablochacin
pablochacin reviewed Oct 24, 2019
View reviewed changes
docs/proposals/20191016-clusterctl-redesign.md

Clusterctl consists of a single binary artifact implementing a CLI; the same features exposed by the CLI will be made available to other management tools via a client library.

During provider installation, clusterctl internal pkg/lib is expected to access provider repositories and read the yaml file specifying all the provider components to be installed in the management cluster.
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe the internal library should be agnostic with respect of the source of this yaml and the cli or external tool should be the responsible of retrieving it. This would allow external tools to use other sources.

Copy link
Contributor

Choose a reason for hiding this comment

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

clusterctl will let you specify alternative sources. Pulling from the provider repositories will be a default but not the only option. Does that address your comment?

Copy link
Contributor

@pablochacin pablochacin Oct 24, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Yes I saw it below and commented more on this topic.

Copy link
Contributor

Choose a reason for hiding this comment

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

@pablochacin just checking in - how are you feeling about this question?

@ncdc ncdc removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 24, 2019
@ncdc
Copy link
Contributor

ncdc commented Oct 24, 2019

/hold
pending approvals

@detiber when you have time, please review & comment or GitHub-approve. Thanks!

@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 Oct 24, 2019
@frapposelli
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 24, 2019
@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Oct 25, 2019
@fabriziopandini
Copy link
Member Author

@ncdc fixed latest nits and squashed commits
thanks for the feedback!

@vincepri
Copy link
Member

/lgtm

Feel free to cancel the hold once ready to be merged.

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 25, 2019
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 30, 2019
@fabriziopandini
Copy link
Member Author

Approver list in the CAEP updated to match PR approvals
PTAL @pablochacin @voor @jzhoucliqr @andrewsykim @detiber @ncdc @vincepri

frapposelli
frapposelli approved these changes Oct 31, 2019
View reviewed changes
Copy link
Member

@frapposelli frapposelli 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 Oct 31, 2019
voor
voor approved these changes Oct 31, 2019
View reviewed changes
Copy link
Member

@voor voor 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
Copy link
Contributor

@voor: changing LGTM is restricted to collaborators

In response to this:

/lgtm

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.

@ncdc
Copy link
Contributor

ncdc commented Oct 31, 2019

FYI, as discussed in yesterday's community meeting, we'll be following lazy consensus and merging on Wednesday, November 6, or once all reviewers have approved the PR, whichever comes sooner.

@timothysc
Copy link
Member

/lgtm
/approve

Thanks for pushing this over the line @fabriziopandini !!!

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: timothysc, vincepri

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

@vincepri
Copy link
Member

vincepri commented Nov 6, 2019

/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 Nov 6, 2019
@k8s-ci-robot k8s-ci-robot merged commit e9162ab into kubernetes-sigs:master Nov 6, 2019
@frapposelli
Copy link
Member

Thanks to all the reviewers, this is a great milestone for the future of clusterctl 🎉

@fabriziopandini fabriziopandini deleted the clusterctl-redesign-proposal branch November 12, 2019 14:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ncdc ncdc ncdc approved these changes

@pablochacin pablochacin pablochacin left review comments

@jzhoucliqr jzhoucliqr jzhoucliqr left review comments

@frapposelli frapposelli frapposelli approved these changes

@detiber detiber detiber approved these changes

@voor voor voor approved these changes

@vincepri vincepri vincepri approved these changes

@andrewsykim andrewsykim Awaiting requested review from andrewsykim

Assignees

@timothysc timothysc

@frapposelli frapposelli

@vincepri vincepri

@andrewsykim andrewsykim

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/proposal Issues or PRs related to proposals. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
v0.3.0
Development

Successfully merging this pull request may close these issues.