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

⚠️ in-place propagation from MS to InfraMachine and BootstrapConfig #8060

Merged
merged 1 commit into from
Feb 28, 2023
Merged

⚠️ in-place propagation from MS to InfraMachine and BootstrapConfig #8060

merged 1 commit into from
Feb 28, 2023

Conversation

ykakarap
Copy link
Contributor

@ykakarap ykakarap commented Feb 6, 2023 •
edited

What this PR does / why we need it:

This PR adds in-place propagation support for labels and annotation from MachineSet to InfraMachine and BootstrapConfig.

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

Part of #7731

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Feb 6, 2023
@k8s-ci-robot k8s-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Feb 6, 2023
@ykakarap
Copy link
Contributor Author

ykakarap commented Feb 6, 2023

/hold until #7921 is merged

@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 Feb 6, 2023
@ykakarap ykakarap mentioned this pull request Feb 6, 2023
Closed
sbueringer
sbueringer reviewed Feb 6, 2023
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.

Overall looks good to me

controllers/external/util.go Outdated Show resolved Hide resolved
controllers/external/util.go Outdated Show resolved Hide resolved
internal/controllers/machineset/machineset_controller.go Outdated Show resolved Hide resolved
internal/controllers/machineset/machineset_controller.go Outdated Show resolved Hide resolved
@ykakarap ykakarap force-pushed the node-label_md-ms-in-place_infra-bootstrap branch from 98d9e66 to 5dd7067 Compare February 7, 2023 07:13
@ykakarap
Copy link
Contributor Author

ykakarap commented Feb 7, 2023

/test pull-cluster-api-e2e-full-main
/test pull-cluster-api-e2e-workload-upgrade-1-26-latest-main

sbueringer
sbueringer reviewed Feb 7, 2023
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.

Overall direction sounds good to me.

I would do one final round of nitpickery once we merged the underlying PR

internal/controllers/machineset/machineset_controller.go Outdated Show resolved Hide resolved
fabriziopandini
fabriziopandini reviewed Feb 7, 2023
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.

From a first look the approach is ok, and with some little refactoring we can make the code simpler to read and to unit test

internal/controllers/machineset/machineset_controller.go Outdated Show resolved Hide resolved
controllers/external/util.go Outdated Show resolved Hide resolved
internal/controllers/machineset/machineset_controller.go Outdated Show resolved Hide resolved
@ykakarap ykakarap mentioned this pull request Feb 14, 2023
Merged
@ykakarap ykakarap mentioned this pull request Feb 22, 2023
Closed
19 tasks
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 23, 2023
@ykakarap ykakarap force-pushed the node-label_md-ms-in-place_infra-bootstrap branch from 5dd7067 to 2dc9d5e Compare February 23, 2023 05:34
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Feb 23, 2023
@ykakarap ykakarap changed the title ✨ [WIP]in-place propagation from MS to InfraMachine and BootstrapConfig ⚠️ in-place propagation from MS to InfraMachine and BootstrapConfig Feb 23, 2023
@ykakarap
Copy link
Contributor Author

/test pull-cluster-api-e2e-full-main
/test pull-cluster-api-e2e-workload-upgrade-1-26-latest-main

@ykakarap
Copy link
Contributor Author

/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 Feb 23, 2023
@sbueringer
Copy link
Member

/retest

1 similar comment
@sbueringer
Copy link
Member

/retest

@ykakarap ykakarap force-pushed the node-label_md-ms-in-place_infra-bootstrap branch from 2dc9d5e to d70e7d6 Compare February 24, 2023 10:03
@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 Feb 24, 2023
@ykakarap
Copy link
Contributor Author

/test pull-cluster-api-e2e-full-main
/test pull-cluster-api-e2e-workload-upgrade-1-26-latest-main

@sbueringer
Copy link
Member

Great work!!

/lgtm

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

LGTM label has been added.

Git tree hash: be7e1d85591934234db91f4dc1d54a694e635442

@ykakarap
Copy link
Contributor Author

/assign @fabriziopandini

vincepri
vincepri reviewed Feb 27, 2023
View reviewed changes
internal/util/ssa/filterintent.go
Comment on lines +114 to +115
// IsAllowedPath returns true when the Path is one of the AllowedPaths.
func IsAllowedPath(allowedPaths []contract.Path) func(path contract.Path) bool {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// IsAllowedPath returns true when the Path is one of the AllowedPaths.
func IsAllowedPath(allowedPaths []contract.Path) func(path contract.Path) bool {
// IsPathAllowed returns true when the Path is one of the AllowedPaths.
func IsPathAllowed(allowedPaths []contract.Path) func(path contract.Path) bool {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since these functions where not introduced in this package and just migrated from another internal package I would like to move this discussion into a follow-up issue to unblock this PR.

Since these functions are internal we are safe to change them in a follow-up.

I added a follow-up item for this in the umbrella issue.

(Same for the other ssa util review comments).

Copy link
Member

Choose a reason for hiding this comment

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

+1

Copy link
Member

Choose a reason for hiding this comment

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

+1 to fix this/similar methods in a follow-up now that those methods are part of our internal utils

internal/util/ssa/filterintent.go Show resolved Hide resolved
internal/util/ssa/filterintent.go Show resolved Hide resolved
internal/util/ssa/managedfields.go Outdated Show resolved Hide resolved
@ykakarap ykakarap force-pushed the node-label_md-ms-in-place_infra-bootstrap branch from b4addaa to f405b5a Compare February 28, 2023 00:13
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 28, 2023
fabriziopandini
fabriziopandini reviewed Feb 28, 2023
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!
/lgtm
/approve

/hold
For @ykakarap @sbueringer to evaluate my question above

internal/util/ssa/managedfields.go Show resolved Hide resolved
@k8s-ci-robot k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Feb 28, 2023
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: b3b5256896ad3f2ff1f02e624ea00eb5fee11404

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 28, 2023
@sbueringer
Copy link
Member

/lgtm

@fabriziopandini
Copy link
Member

/lgtm
/approve

@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

@sbueringer
Copy link
Member

/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 Feb 28, 2023
@sbueringer
Copy link
Member

/retest

@k8s-ci-robot k8s-ci-robot merged commit 2aa9c9d into kubernetes-sigs:main Feb 28, 2023
@k8s-ci-robot k8s-ci-robot added this to the v1.4 milestone Feb 28, 2023
This was referenced Mar 28, 2023
Closed
Closed
@muraee muraee mentioned this pull request May 15, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fabriziopandini fabriziopandini fabriziopandini left review comments

@vincepri vincepri vincepri approved these changes

@enxebre enxebre Awaiting requested review from enxebre

@sbueringer sbueringer Awaiting requested review from sbueringer

Assignees

@sbueringer sbueringer

@fabriziopandini fabriziopandini

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.4
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@ykakarap @sbueringer @k8s-ci-robot @fabriziopandini @vincepri