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

🌱 Make KCP using embedded kubeadm types while manipulating the kubeadm-config ConfigMap #4443

Merged
merged 1 commit into from
May 11, 2021
Merged

🌱 Make KCP using embedded kubeadm types while manipulating the kubeadm-config ConfigMap #4443

merged 1 commit into from
May 11, 2021
+973 −2,002

Conversation

fabriziopandini
Copy link
Member

What this PR does / why we need it:
This PR is a follow up of #4255 and the last step for implementing the Insulate users from kubeadm API version changes KEP

More specifically, this PR changes KCP in order to use the embedded kubeadm types and the underlying conversion mechanism while manipulating the Kubeadm-config ConfigMap.

Given the existing nested interfaces in KCP, this PR touch many methods, however during the change:

  • all the ConfigMap transformations are now managed by a single method and executed only if required (before most code was duplicated, in many case with slight variations)
  • version management was consistently moved to use semver.Version (before it was a mix of string, semver.Version, and version.Version)

Which issue(s) this PR fixes:
Fixes #

@fabriziopandini fabriziopandini requested a review from vincepri April 7, 2021 15:31
@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Apr 7, 2021
@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Apr 7, 2021
@fabriziopandini fabriziopandini force-pushed the use-cluster-API-owned-kubeadm-types-in-KCP branch 2 times, most recently from 5e31866 to f60c48e Compare April 26, 2021 12:50
@fabriziopandini
Copy link
Member Author

/retest

@fabriziopandini fabriziopandini removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 26, 2021
@fabriziopandini
Copy link
Member Author

It seems there are lint and test errors happening in CI and not locally 🤔, I have to investigate

@sbueringer
Copy link
Member

@fabriziopandini I think it's using a newer linter (v1.38.0) https://github.com/kubernetes-sigs/cluster-api/pull/4443/checks?check_run_id=2438374848
vs the one we have in go.mod (v1.32.0)

vincepri
vincepri requested changes Apr 27, 2021
View reviewed changes
bootstrap/kubeadm/types/utils.go Outdated Show resolved Hide resolved
bootstrap/kubeadm/types/utils.go Outdated Show resolved Hide resolved
controlplane/kubeadm/internal/workload_cluster.go Outdated Show resolved Hide resolved
controlplane/kubeadm/internal/workload_cluster.go Outdated Show resolved Hide resolved
controlplane/kubeadm/internal/workload_cluster.go Outdated Show resolved Hide resolved
controlplane/kubeadm/internal/workload_cluster.go Outdated Show resolved Hide resolved
controlplane/kubeadm/internal/workload_cluster.go Outdated Show resolved Hide resolved
controlplane/kubeadm/internal/workload_cluster.go Outdated Show resolved Hide resolved
controlplane/kubeadm/internal/workload_cluster.go Outdated Show resolved Hide resolved
controlplane/kubeadm/internal/workload_cluster.go Outdated Show resolved Hide resolved
@fabriziopandini fabriziopandini force-pushed the use-cluster-API-owned-kubeadm-types-in-KCP branch from 66c214a to a88b1d0 Compare April 29, 2021 10:04
@fabriziopandini
Copy link
Member Author

@vincepri thanks for feedback
I took over from your suggestion and completely dropped the KubeadmConfig structure, thus reducing the number of internal abstraction in KCP. Unfortunately this lead to refactoring some test, but I think this is worth the effort....

@fabriziopandini
Copy link
Member Author

/retest

@fabriziopandini fabriziopandini mentioned this pull request Apr 29, 2021
Merged
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 29, 2021
@fabriziopandini fabriziopandini force-pushed the use-cluster-API-owned-kubeadm-types-in-KCP branch from a88b1d0 to 7f5f409 Compare April 30, 2021 18:09
@fabriziopandini
Copy link
Member Author

/retest

vincepri
vincepri approved these changes Apr 30, 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.

/lgtm
/assign @CecileRobertMichon

@k8s-ci-robot k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Apr 30, 2021
CecileRobertMichon
CecileRobertMichon reviewed Apr 30, 2021
View reviewed changes
Copy link
Contributor

@CecileRobertMichon CecileRobertMichon left a comment

Choose a reason for hiding this comment

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

lgtm

@fabriziopandini fabriziopandini force-pushed the use-cluster-API-owned-kubeadm-types-in-KCP branch from 7f5f409 to b33c910 Compare May 4, 2021 08:58
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 4, 2021
@fabriziopandini
Copy link
Member Author

@CecileRobertMichon added comments to updateClusterConfiguration/updateClusterStatus, PTAL

@fabriziopandini fabriziopandini mentioned this pull request May 5, 2021
Closed
@vincepri
Copy link
Member

/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 May 10, 2021
@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 10, 2021
@fabriziopandini fabriziopandini force-pushed the use-cluster-API-owned-kubeadm-types-in-KCP branch from b33c910 to 5c19ea1 Compare May 11, 2021 12:29
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 11, 2021
@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented May 11, 2021 •
edited
Loading

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

Test name Commit Details Rerun command
pull-cluster-api-apidiff-main 5c19ea1 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.

@fabriziopandini
Copy link
Member Author

@vincepri rebased fixing recently added remediation tests to get Kubernetes version as a input

vincepri
vincepri approved these changes May 11, 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 May 11, 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 merged commit 321ec39 into kubernetes-sigs:master May 11, 2021
@k8s-ci-robot k8s-ci-robot added this to the v0.4 milestone May 11, 2021
@fabriziopandini fabriziopandini deleted the use-cluster-API-owned-kubeadm-types-in-KCP branch May 12, 2021 11:43
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

@vincepri vincepri vincepri approved these changes

@JoelSpeed JoelSpeed Awaiting requested review from JoelSpeed

Assignees

@vincepri vincepri

@CecileRobertMichon CecileRobertMichon

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/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
v0.4
Development

Successfully merging this pull request may close these issues.
5 participants
@fabriziopandini @sbueringer @vincepri @k8s-ci-robot @CecileRobertMichon