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

📖 ClusterClass patch proposal amendment #5212

Merged
merged 1 commit into from
Oct 9, 2021
Merged

📖 ClusterClass patch proposal amendment #5212

merged 1 commit into from
Oct 9, 2021

Conversation

sbueringer
Copy link
Member

Signed-off-by: Stefan Büringer buringerst@vmware.com

What this PR does / why we need it:
This amendment introduces ClusterClass patches.

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 #5142

@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 6, 2021
@k8s-ci-robot k8s-ci-robot added 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 Sep 6, 2021
@sbueringer sbueringer force-pushed the pr-clusterclass-patches-amendment branch from 7eb3421 to 607e7fc Compare September 6, 2021 15:34
@sbueringer sbueringer changed the title [WIP] 📖 ClusterClass patch proposal amendment 📖 ClusterClass patch proposal amendment Sep 7, 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 Sep 7, 2021
@sbueringer
Copy link
Member Author

/retest

MarcelMue
MarcelMue reviewed Sep 7, 2021
View reviewed changes
Copy link
Contributor

@MarcelMue MarcelMue left a comment

Choose a reason for hiding this comment

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

This makes sense to me after the added context from slack. I personally believe that some more context should be added here to explain the operational benefits which are achieved with the introduction of patches.

docs/proposals/202105256-cluster-class-and-managed-topologies.md Outdated Show resolved Hide resolved
@sbueringer
Copy link
Member Author

This makes sense to me after the added context from slack. I personally believe that some more context should be added here to explain the operational benefits which are achieved with the introduction of patches.

Which operational benefits are you referring to specifically?

@MarcelMue
Copy link
Contributor

This makes sense to me after the added context from slack. I personally believe that some more context should be added here to explain the operational benefits which are achieved with the introduction of patches.

Which operational benefits are you referring to specifically?

Basically the use-case of infra machine templates being mutated / rolled out through this workflow in the long run. This also aligns with the comment from Cecile about the mutation use case: #5212 (comment)

@sbueringer
Copy link
Member Author

This makes sense to me after the added context from slack. I personally believe that some more context should be added here to explain the operational benefits which are achieved with the introduction of patches.

Which operational benefits are you referring to specifically?

Basically the use-case of infra machine templates being mutated / rolled out through this workflow in the long run. This also aligns with the comment from Cecile about the mutation use case: #5212 (comment)

Thx, that's helpful. Will try to make that clear and especially the mutability we already have without patches and how patches relate to that.

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 9, 2021
@sbueringer sbueringer force-pushed the pr-clusterclass-patches-amendment branch from 607e7fc to f838966 Compare September 9, 2021 06:33
@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 9, 2021
@sbueringer
Copy link
Member Author

@enxebre @schrej @MarcelMue @CecileRobertMichon @killianmuldoon @vincepri @fabriziopandini

Thx for your reviews, ptal.

I tried to address the comments. The biggest change is that I tried to clarify how Cluster mutability works (even before introducing patches).

@sbueringer
Copy link
Member Author

sbueringer commented Sep 15, 2021 •
edited

Hey folks,
I think I addressed all comments. I would like to propose to start a lazy consensus if there are no other concerns. It looks to me like we have a consensus about how patches/variables are supposed to look like.

It would be great to get start with the implementation and iterate on it. It would also make it a lot easier for our team to coordinate the work to get v1beta1 and ClusterClass patches done.

I'll also bring this up in the meeting today.

@vincepri
Copy link
Member

/milestone v1.0

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 4, 2021
@fabriziopandini
Copy link
Member

/lgtm

@vincepri
Copy link
Member

vincepri commented Oct 4, 2021

@sbueringer please squash

@sbueringer
ClusterClass patch amendment
eae3ace 
Signed-off-by: Stefan Büringer buringerst@vmware.com
@sbueringer sbueringer force-pushed the pr-clusterclass-patches-amendment branch from e57af3e to eae3ace Compare October 4, 2021 16:19
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 4, 2021
@sbueringer
Copy link
Member Author

@sbueringer please squash

Done

@sbueringer
Copy link
Member Author

/retest

@fabriziopandini
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 Oct 4, 2021
@MarcelMue
Copy link
Contributor

/lgtm

@vincepri
Copy link
Member

vincepri commented Oct 6, 2021

/approve
/hold for community meeting

@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. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Oct 6, 2021
CecileRobertMichon
CecileRobertMichon reviewed Oct 6, 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

@sbueringer
Copy link
Member Author

As discussed in the CAPI meeting today, lazy consensus until ~Friday

@vincepri
Copy link
Member

vincepri commented Oct 9, 2021

/approve
/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 Oct 9, 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 a60f5f5 into kubernetes-sigs:main Oct 9, 2021
@k8s-ci-robot k8s-ci-robot modified the milestones: v1.0, v0.4 Oct 9, 2021
@sbueringer sbueringer deleted the pr-clusterclass-patches-amendment branch October 11, 2021 07:50
@sbueringer sbueringer mentioned this pull request Oct 11, 2021
Merged
@killianmuldoon killianmuldoon mentioned this pull request Oct 20, 2021
Closed
@schrej schrej mentioned this pull request Jan 4, 2022
Closed
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joejulian joejulian joejulian left review comments

@enxebre enxebre enxebre left review comments

@fabriziopandini fabriziopandini fabriziopandini left review comments

@puja108 puja108 puja108 left review comments

@CecileRobertMichon CecileRobertMichon CecileRobertMichon left review comments

@MarcelMue MarcelMue MarcelMue left review comments

@killianmuldoon killianmuldoon killianmuldoon left review comments

@schrej schrej schrej approved these changes

@vincepri vincepri vincepri approved these changes

@JoelSpeed JoelSpeed Awaiting requested review from JoelSpeed

Assignees

@enxebre enxebre

@fabriziopandini fabriziopandini

@CecileRobertMichon CecileRobertMichon

@MarcelMue MarcelMue

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.

ClusterClass transformations/patches