Skip to content

⚠️ Move Machine deletion timeout fields into deletion group, move KCP machineTemplate spec fields to machineTemplate.spec #12499

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
Merged
merged 4 commits into from
Jul 17, 2025
Merged

⚠️ Move Machine deletion timeout fields into deletion group, move KCP machineTemplate spec fields to machineTemplate.spec #12499

merged 4 commits into from
Jul 17, 2025

Conversation

@sbueringer
Copy link
Member

@sbueringer sbueringer commented Jul 16, 2025 •
edited
Loading

Signed-off-by: Stefan Büringer [email protected]

What this PR does / why we need it:

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Part of #12497

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/needs-area PR is missing an area label labels Jul 16, 2025
@k8s-ci-robot k8s-ci-robot requested review from elmiko and g-gaston July 16, 2025 15:51
@k8s-ci-robot k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Jul 16, 2025
@sbueringer
Copy link
Member Author

/hold

Will push an additional commit with doc updates, otherwise ready for review

/test pull-cluster-api-e2e-main-gke

@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 Jul 16, 2025
@sbueringer sbueringer added area/api Issues or PRs related to the APIs area/machine Issues or PRs related to machine lifecycle management and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Jul 16, 2025
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/needs-area PR is missing an area label label Jul 16, 2025
@sbueringer
Copy link
Member Author

/assign @JoelSpeed @fabriziopandini

sbueringer
sbueringer commented Jul 16, 2025
View reviewed changes
internal/util/ssa/patch.go
}
// NOTE: DropEmptyStruct is required for typed objects, because they are converted to unstructured using the DefaultUnstructuredConverter,
// and it does not handle omitzero (yet).
dropEmptyStruct = true
Copy link
Member Author

@sbueringer sbueringer Jul 16, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Without this the MD controller will try to create an MS with deletion: {} because the v0.33 DefaultUnstructuredConverter does not handle omitzero correctly yet (It will with v0.34)

Now that I'm setting this here the MP Machine controller cannot create MP Machines with bootstrap: {} anymore, so I'm setting bootstrap.dataSecretName: &"" there

@sbueringer sbueringer changed the title ⚠️ Move Machine deletion timeout fields into deletion group ⚠️ Move Machine deletion timeout fields into deletion group, move KCP machineTemplate spec fields to machineTemplate.spec Jul 17, 2025
@sbueringer
Copy link
Member Author

/test pull-cluster-api-e2e-main-gke

@sbueringer
Copy link
Member Author

/assign @sivchari

@sbueringer
Copy link
Member Author

Pushed another commit with doc updates

@sbueringer sbueringer force-pushed the pr-deletion-grouping branch from db01963 to b9e31f7 Compare July 17, 2025 11:07
@sbueringer
Copy link
Member Author

/test pull-cluster-api-e2e-main-gke

@k8s-ci-robot
Copy link
Contributor

@sbueringer: The following test 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 b9e31f7 link false /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-sigs/prow repository. I understand the commands that are listed here.

@sbueringer
Copy link
Member Author

I can't reproduce the linter findings locally. This should be all good after #12500.

On my machine I can fix this by running ./hack/tools/bin/golangci-lint-kube-api-linter cache clean. I'm not sure how I can do it with GitHub actions

@sbueringer
Copy link
Member Author

cc @JoelSpeed just fyi. In general I had to run cache clean quite a few times with KAL in the last few weeks. Not sure if that's expected

@JoelSpeed
Copy link
Contributor

In general I had to run cache clean quite a few times with KAL in the last few weeks. Not sure if that's expected

It's a golangci-lint thing, it doesn't detect that the configuration changes and caches results that aren't actually true for the new config, there are issues on the golangci-lint project about this

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 17, 2025
@sbueringer
Copy link
Member Author

In general I had to run cache clean quite a few times with KAL in the last few weeks. Not sure if that's expected

It's a golangci-lint thing, it doesn't detect that the configuration changes and caches results that aren't actually true for the new config, there are issues on the golangci-lint project about this

kk, thx for the info!

I"ll have to rebase to fix the conflict, that will "fix" the caching issue

JoelSpeed
JoelSpeed reviewed Jul 17, 2025
View reviewed changes
Copy link
Contributor

@JoelSpeed JoelSpeed 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 Jul 17, 2025
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 1db28076427a29111e3225363a2fccbf9c7e3021

fabriziopandini
fabriziopandini reviewed Jul 17, 2025
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.

Great work!

api/controlplane/kubeadm/v1beta2/kubeadmcontrolplanetemplate_types.go

// KubeadmControlPlaneTemplateMachineTemplateSpec defines the spec for Machines
// in a KubeadmControlPlane object.
type KubeadmControlPlaneTemplateMachineTemplateSpec struct {
Copy link
Member

Choose a reason for hiding this comment

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

q: should we add minProperties?

@sbueringer sbueringer force-pushed the pr-deletion-grouping branch from b9e31f7 to 1cf8729 Compare July 17, 2025 13:18
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 17, 2025
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 17, 2025
@sbueringer
Copy link
Member Author

/test pull-cluster-api-e2e-main-gke

@sbueringer
Copy link
Member Author

I'll merge independent of the GitHub action. This was just rebased onto main + locally all linter are green.

@fabriziopandini fabriziopandini added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Jul 17, 2025
@fabriziopandini
Copy link
Member

/lgtm
/approve

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

LGTM label has been added.

Git tree hash: 3ee9428d40a8cea125dcfed1ae4f6340ef878274

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: fabriziopandini

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 Jul 17, 2025
@sbueringer sbueringer merged commit c30eaea into kubernetes-sigs:main Jul 17, 2025
16 of 24 checks passed
@k8s-ci-robot k8s-ci-robot added this to the v1.11 milestone Jul 17, 2025
@sbueringer sbueringer deleted the pr-deletion-grouping branch July 17, 2025 17:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@elmiko elmiko Awaiting requested review from elmiko

@g-gaston g-gaston Awaiting requested review from g-gaston

@fabriziopandini fabriziopandini Awaiting requested review from fabriziopandini

@JoelSpeed JoelSpeed Awaiting requested review from JoelSpeed

@sivchari sivchari Awaiting requested review from sivchari

Assignees

@fabriziopandini fabriziopandini

@JoelSpeed JoelSpeed

@sivchari sivchari

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. area/api Issues or PRs related to the APIs area/machine Issues or PRs related to machine lifecycle management 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. tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges.

Projects

None yet

Milestone

v1.11

Development

Successfully merging this pull request may close these issues.

5 participants

@sbueringer @k8s-ci-robot @JoelSpeed @fabriziopandini @sivchari