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

✨ add MHC generation from ClusterClass #5873

Merged
merged 1 commit into from
Jan 14, 2022
Merged

✨ add MHC generation from ClusterClass #5873

merged 1 commit into from
Jan 14, 2022

Conversation

killianmuldoon
Copy link
Contributor

@killianmuldoon killianmuldoon commented Dec 16, 2021 •
edited

Signed-off-by: killianmuldoon kmuldoon@vmware.com

Allow the definition of MachineHealthChecks in a ClusterClass at either the MachineDeployment or ControlPlane Object level. Reconcile the definitions and handle the lifecycle of the MachineHealthCheck in relation to its referenced object.

Fixes #5125

@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. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Dec 16, 2021
@killianmuldoon killianmuldoon force-pushed the topology/introduce-mhc-from-clusterclass branch from 14ac730 to 7f9b99c Compare December 17, 2021 17:44
@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Dec 17, 2021
@killianmuldoon killianmuldoon force-pushed the topology/introduce-mhc-from-clusterclass branch from 7f9b99c to 62551ac Compare December 17, 2021 17:59
@killianmuldoon killianmuldoon force-pushed the topology/introduce-mhc-from-clusterclass branch from 62551ac to 2947865 Compare December 20, 2021 14:13
@killianmuldoon killianmuldoon changed the title [WIP] 🌱 add MHC generation from ClusterClass ✨ add MHC generation from ClusterClass Dec 20, 2021
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 20, 2021
@killianmuldoon killianmuldoon force-pushed the topology/introduce-mhc-from-clusterclass branch 4 times, most recently from dfbbe12 to 8e41069 Compare December 20, 2021 15:43
fabriziopandini
fabriziopandini reviewed Dec 21, 2021
View reviewed changes
Copy link
Member

@fabriziopandini fabriziopandini left a comment

Choose a reason for hiding this comment

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

First pass after WIP removal; most of the comments are nitpicking
What is missing though are testing on reconcileMHC

controllers/topology/blueprint_test.go Outdated Show resolved Hide resolved
controllers/topology/current_state.go Outdated Show resolved Hide resolved
controllers/topology/current_state.go Outdated Show resolved Hide resolved
controllers/topology/current_state_test.go Outdated Show resolved Hide resolved
internal/webhooks/clusterclass_test.go Outdated Show resolved Hide resolved
controllers/topology/machinedeployment_controller.go Outdated Show resolved Hide resolved
controllers/topology/machinedeployment_controller.go Outdated Show resolved Hide resolved
controllers/topology/reconcile_state.go Outdated Show resolved Hide resolved
controllers/topology/reconcile_state.go Outdated Show resolved Hide resolved
controllers/topology/reconcile_state.go Outdated Show resolved Hide resolved
@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Dec 22, 2021
@killianmuldoon killianmuldoon force-pushed the topology/introduce-mhc-from-clusterclass branch 2 times, most recently from dd4042f to c43f231 Compare December 22, 2021 17:21
fabriziopandini
fabriziopandini reviewed Jan 3, 2022
View reviewed changes
controllers/topology/desired_state.go Outdated Show resolved Hide resolved
controllers/topology/desired_state.go Outdated Show resolved Hide resolved
controllers/topology/desired_state.go Outdated Show resolved Hide resolved
controllers/topology/desired_state.go Outdated Show resolved Hide resolved
controllers/topology/desired_state.go Outdated Show resolved Hide resolved
sbueringer
sbueringer reviewed Jan 7, 2022
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.

Looks great, first round of review

api/v1alpha4/conversion.go Outdated Show resolved Hide resolved
api/v1beta1/clusterclass_types.go Outdated Show resolved Hide resolved
api/v1beta1/clusterclass_types.go Show resolved Hide resolved
api/v1beta1/common_types.go Outdated Show resolved Hide resolved
api/v1beta1/common_types.go Outdated Show resolved Hide resolved
controllers/topology/machinedeployment_controller.go Outdated Show resolved Hide resolved
controllers/topology/reconcile_state.go Outdated Show resolved Hide resolved
controllers/topology/reconcile_state.go Outdated Show resolved Hide resolved
controllers/topology/reconcile_state.go Outdated Show resolved Hide resolved
controllers/topology/reconcile_state.go Outdated Show resolved Hide resolved
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 7, 2022
@killianmuldoon killianmuldoon force-pushed the topology/introduce-mhc-from-clusterclass branch from c43f231 to 41e85db Compare January 11, 2022 14:12
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 11, 2022
@killianmuldoon killianmuldoon force-pushed the topology/introduce-mhc-from-clusterclass branch from 41e85db to 32a73f2 Compare January 11, 2022 19:33
@killianmuldoon killianmuldoon force-pushed the topology/introduce-mhc-from-clusterclass branch from b23dd19 to 272aa71 Compare January 12, 2022 16:31
@killianmuldoon killianmuldoon force-pushed the topology/introduce-mhc-from-clusterclass branch from 272aa71 to 27d775c Compare January 12, 2022 19:03
@sbueringer
Copy link
Member

@killianmuldoon I'm reviewing now. Based on the verify job I think you have to re-generate the core manifests and conversions.

@killianmuldoon killianmuldoon force-pushed the topology/introduce-mhc-from-clusterclass branch 2 times, most recently from 45bc9c9 to 27d775c Compare January 13, 2022 14:53
sbueringer
sbueringer reviewed Jan 13, 2022
View reviewed changes
Copy link
Member

@sbueringer sbueringer left a comment •
edited

Choose a reason for hiding this comment

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

Nice work! :)
(almost only nits)

internal/builder/builders.go Outdated Show resolved Hide resolved
internal/builder/builders.go Outdated Show resolved Hide resolved
internal/controllers/topology/cluster/scope/blueprint.go Outdated Show resolved Hide resolved
internal/webhooks/clusterclass.go Outdated Show resolved Hide resolved
internal/webhooks/clusterclass.go Outdated Show resolved Hide resolved
internal/controllers/topology/cluster/reconcile_state.go Outdated Show resolved Hide resolved
internal/controllers/topology/cluster/reconcile_state.go Outdated Show resolved Hide resolved
internal/controllers/topology/cluster/reconcile_state.go Outdated Show resolved Hide resolved
internal/controllers/topology/cluster/reconcile_state.go Show resolved Hide resolved
internal/controllers/topology/machinedeployment/machinedeployment_controller.go Outdated Show resolved Hide resolved
@sbueringer
Copy link
Member

If you want to pair on the conversion issue, just let me know.

@killianmuldoon killianmuldoon force-pushed the topology/introduce-mhc-from-clusterclass branch from 925a4aa to 29ba4c9 Compare January 13, 2022 18:11
fabriziopandini
fabriziopandini reviewed Jan 13, 2022
View reviewed changes
Copy link
Member

@fabriziopandini fabriziopandini left a comment

Choose a reason for hiding this comment

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

two small nits, otherwise lgtm

internal/controllers/topology/cluster/reconcile_state.go Outdated Show resolved Hide resolved
internal/controllers/topology/cluster/reconcile_state.go Outdated Show resolved Hide resolved
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 14, 2022
@killianmuldoon killianmuldoon force-pushed the topology/introduce-mhc-from-clusterclass branch from 30af67a to 96bc970 Compare January 14, 2022 10: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 Jan 14, 2022
@killianmuldoon killianmuldoon force-pushed the topology/introduce-mhc-from-clusterclass branch 2 times, most recently from 7f652c0 to 8906cef Compare January 14, 2022 11:18
sbueringer
sbueringer reviewed Jan 14, 2022
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.

looks great, one last nit

api/v1alpha4/conversion.go Show resolved Hide resolved
@killianmuldoon killianmuldoon force-pushed the topology/introduce-mhc-from-clusterclass branch from 8906cef to 4de172a Compare January 14, 2022 11:36
@killianmuldoon
add MHC generation from ClusterClass
1b760e0 
Signed-off-by: killianmuldoon <kmuldoon@vmware.com>
@killianmuldoon killianmuldoon force-pushed the topology/introduce-mhc-from-clusterclass branch from 4de172a to 1b760e0 Compare January 14, 2022 11:36
@sbueringer
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 Jan 14, 2022
vincepri
vincepri approved these changes Jan 14, 2022
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

@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 Jan 14, 2022
@k8s-ci-robot k8s-ci-robot merged commit 6a3a91b into kubernetes-sigs:main Jan 14, 2022
@k8s-ci-robot k8s-ci-robot added this to the v1.1 milestone Jan 14, 2022
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

@fabriziopandini fabriziopandini fabriziopandini left review comments

@vincepri vincepri vincepri approved these changes

@stmcginnis stmcginnis Awaiting requested review from stmcginnis

Assignees

@sbueringer sbueringer

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
v1.1
Development

Successfully merging this pull request may close these issues.

Support MHC in ClusterClass
5 participants
@killianmuldoon @sbueringer @k8s-ci-robot @vincepri @fabriziopandini