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

⚠️ Adapt clusterctl to webhook deployed with managers #4297

Merged
merged 1 commit into from
Jun 3, 2021
Merged

⚠️ Adapt clusterctl to webhook deployed with managers #4297

merged 1 commit into from
Jun 3, 2021

Conversation

fabriziopandini
Copy link
Member

@fabriziopandini fabriziopandini commented Mar 11, 2021 •
edited
Loading

What this PR does / why we need it:
After #3985 Cluster API and all the providers are going to deploy web hook with managers.

This makes it possible to cleanup some code in clusterctl and to simplify a few interfaces in the clusterctl library

Which issue(s) this PR fixes:
Rif #4056

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Mar 11, 2021
@fabriziopandini
Copy link
Member Author

/milestone v0.4.0

@k8s-ci-robot k8s-ci-robot added this to the v0.4.0 milestone Mar 11, 2021
@k8s-ci-robot k8s-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Mar 11, 2021
@fabriziopandini
Copy link
Member Author

/test pull-cluster-api-verify

@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 24, 2021
@fabriziopandini fabriziopandini force-pushed the adapt-clusterctl-to-new-manifests branch from ebae0df to a629df2 Compare March 26, 2021 11:37
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 26, 2021
@fabriziopandini
Copy link
Member Author

/retest

@fabriziopandini
Copy link
Member Author

/test pull-cluster-api-e2e-k8s-latest-main

@fabriziopandini
Copy link
Member Author

/test pull-cluster-api-e2e-main

@fabriziopandini fabriziopandini force-pushed the adapt-clusterctl-to-new-manifests branch from a629df2 to 9d12847 Compare April 6, 2021 20:00
@fabriziopandini
Copy link
Member Author

/test pull-cluster-api-e2e-k8s-latest-main

@stmcginnis
Copy link
Contributor

Changes look good to me, but are the failures in pull-cluster-api-apidiff-main and issue?

Very nice! >> "+75 −660"

@vincepri
Copy link
Member

vincepri commented May 10, 2021 •
edited
Loading 

sigs.k8s.io/cluster-api/cmd/clusterctl/client
  Incompatible changes:
  - sigs.k8s.io/cluster-api/cmd/clusterctl/client.Components.InstanceObjs: removed
  - sigs.k8s.io/cluster-api/cmd/clusterctl/client.Components.Objs: added
  - sigs.k8s.io/cluster-api/cmd/clusterctl/client.Components.SharedObjs: removed
sigs.k8s.io/cluster-api/cmd/clusterctl/api/v1alpha3
  Incompatible changes:
  - ClusterctlResourceLifecyleLabelName: removed
  - ResourceLifecycle: removed
  - ResourceLifecycleShared: removed
sigs.k8s.io/cluster-api/cmd/clusterctl/client/repository
  Incompatible changes:
  - (*components).InstanceObjs: removed
  - (*components).SharedObjs: removed
  - Components.InstanceObjs: removed
  - Components.Objs: added
  - Components.SharedObjs: removed
  - WebhookNamespaceName: removed
  Compatible changes:
  - (*components).Objs: added

Should we mark this as breaking?

@fabriziopandini fabriziopandini changed the title 🌱Adapt clusterctl to webhook deployed with managers ⚠️ Adapt clusterctl to webhook deployed with managers May 10, 2021
@cpanato
Copy link
Member

cpanato commented May 10, 2021

What is the impact on the providers? what we should do when updating the dependencies that will have this change here?

@vincepri
Copy link
Member

@cpanato See https://github.com/kubernetes-sigs/cluster-api/blob/master/docs/book/src/developer/providers/v1alpha3-to-v1alpha4.md#required-kustomize-changes-to-have-a-single-manager-watching-all-namespaces-and-answer-to-webhook-calls

@fabriziopandini
Copy link
Member Author

@cpanato impacts on the provider are already documented in https://github.com/kubernetes-sigs/cluster-api/blob/master/docs/book/src/developer/providers/v1alpha3-to-v1alpha4.md#multi-tenancy; this PR cleanups clusterctl accordingly

@cpanato
Copy link
Member

cpanato commented May 10, 2021

thanks for the clarification @fabriziopandini @vincepri

LGTM

sbueringer
sbueringer reviewed May 11, 2021
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.

code looks plausible and good to me, but I'm definitely missing too much context

cmd/clusterctl/client/cluster/components.go Outdated Show resolved Hide resolved
@fabriziopandini fabriziopandini force-pushed the adapt-clusterctl-to-new-manifests branch from 9d12847 to e500eb5 Compare May 12, 2021 10:46
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 12, 2021
@fabriziopandini fabriziopandini force-pushed the adapt-clusterctl-to-new-manifests branch from e500eb5 to d6a597b Compare May 17, 2021 09:59
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 17, 2021
@fabriziopandini fabriziopandini force-pushed the adapt-clusterctl-to-new-manifests branch from d6a597b to 8d2c668 Compare May 25, 2021 20:16
@vincepri
Copy link
Member

/retest

@fabriziopandini fabriziopandini linked an issue May 26, 2021 that may be closed by this pull request
Adapt clusterctl to the managers watching all namespaces for each provider #4056
Closed
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 29, 2021
@fabriziopandini fabriziopandini force-pushed the adapt-clusterctl-to-new-manifests branch from 8d2c668 to a4f18e7 Compare May 31, 2021 09:05
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 31, 2021
@fabriziopandini
Copy link
Member Author

/retest

@fabriziopandini fabriziopandini force-pushed the adapt-clusterctl-to-new-manifests branch from a4f18e7 to 9fe5c49 Compare June 3, 2021 19:57
@k8s-ci-robot
Copy link
Contributor

@fabriziopandini: The following test failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
pull-cluster-api-apidiff-main 9fe5c49 link /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.

vincepri
vincepri approved these changes Jun 3, 2021
View reviewed changes
Copy link
Member

@vincepri vincepri left a comment

Choose a reason for hiding this comment

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

/approve
/lgtm

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 3, 2021
@k8s-ci-robot k8s-ci-robot merged commit 61e63e0 into kubernetes-sigs:master Jun 3, 2021
@fabriziopandini fabriziopandini deleted the adapt-clusterctl-to-new-manifests branch June 4, 2021 09:57
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

@enxebre enxebre enxebre left review comments

@vincepri vincepri vincepri approved these changes

@CecileRobertMichon CecileRobertMichon Awaiting requested review from CecileRobertMichon

@moshloop moshloop Awaiting requested review from moshloop

Assignees

@vincepri vincepri

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. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Milestone
v0.4
Development

Successfully merging this pull request may close these issues.

Adapt clusterctl to the managers watching all namespaces for each provider
7 participants
@fabriziopandini @stmcginnis @vincepri @cpanato @k8s-ci-robot @enxebre @sbueringer