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.

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

✨ Backport KCP Rollout Strategy to v1alpha3 api #4293

Merged

Conversation

jan-est
Copy link
Contributor

@jan-est jan-est commented Mar 11, 2021

This PR backport KCP Rollout Strategy feature 4073 to v1alpha3 api for next v0.3.x release.

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Mar 11, 2021
@k8s-ci-robot
Copy link
Contributor

Hi @jan-est. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Mar 11, 2021
@neolit123
Copy link
Member

neolit123 commented Mar 11, 2021

what is the CAPI stance on backporting features?
upstream k8s forbids it unless something is completely blocking (or broken), but subprojects can define their own rules.

EDIT: this might be a detail missing in https://github.com/kubernetes-sigs/cluster-api/blob/1c541d922f20b343ff68be459ca489d8d28745b3/VERSIONING.md

@jan-est
Copy link
Contributor Author

jan-est commented Mar 11, 2021

what is the CAPI stance on backporting features?
upstream k8s forbids it unless something is completely blocking (or broken), but subprojects can define their own rules.

EDIT: this might be a detail missing in https://github.com/kubernetes-sigs/cluster-api/blob/1c541d922f20b343ff68be459ca489d8d28745b3/VERSIONING.md

Backporting this feature was agreed on community meeting yesterday. This is not breaking change if I understood right? And my understanding is that backporting in CAPI was relaxed a while ago.

@CecileRobertMichon
Copy link
Contributor

@CecileRobertMichon
Copy link
Contributor

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Mar 11, 2021
@CecileRobertMichon
Copy link
Contributor

/retest

@CecileRobertMichon
Copy link
Contributor

/lgtm
/milestone v0.3.x
/assign @vincepri

@k8s-ci-robot k8s-ci-robot added this to the v0.3.x milestone Mar 11, 2021
@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 11, 2021
@jan-est jan-est force-pushed the kcp-rollout-strategy-backport branch from 5630cbc to 91944aa Compare March 15, 2021 06:19
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 15, 2021
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.

Question: should we wait for back porting the E2E test before merging this change in 0.3, which is our current "production" release?

controlplane/kubeadm/controllers/upgrade.go Outdated Show resolved Hide resolved
@CecileRobertMichon
Copy link
Contributor

CecileRobertMichon commented Mar 24, 2021

@vincepri @fabriziopandini this PR is a backport of #4073, aren't we introducing risk by suggesting changes here that weren't in the additional PR? I would prefer if there is anything that needs to be changed and wasn't caught in the initial review of #4073 that a follow up PR be open against main and then both be backported together, that way we stick to clean cherry picks (unless anything is v1alpha3 specific).

@fabriziopandini
Copy link
Member

@CecileRobertMichon 100% agreed to strive for a clean cherry pick (and document this in our contributor guidelines)
Unfortunately, given that the differences between the two branches are considerable, this is getting really difficult, but I think that for the changes required in this PR this is doable

@jan-est
Copy link
Contributor Author

jan-est commented Mar 24, 2021

@CecileRobertMichon @vincepri @fabriziopandini just let me know if you want me to do something else for the PR.

@fabriziopandini
Copy link
Member

@jan-est let's merge this PR as it is, implement the fix proposed in #4293 (comment) in a new PR on v1alpha4, and then backport.

What is the status of the PR implementing E2E tests for scale in?

@jan-est
Copy link
Contributor Author

jan-est commented Mar 25, 2021

@jan-est let's merge this PR as it is, implement the fix proposed in #4293 (comment) in a new PR on v1alpha4, and then backport.

Ok, I will work on that. @fabriziopandini could you quickly check is the what we are after 4376

What is the status of the PR implementing E2E tests for scale in?

Not any comments so far and tests are passing. 4347

@jan-est
Copy link
Contributor Author

jan-est commented Mar 28, 2021

@jan-est let's merge this PR as it is, implement the fix proposed in #4293 (comment) in a new PR on v1alpha4, and then backport.

@fabriziopandini now when 4376 is merged, should I wait this PR to be merged before backport 4376?

@vincepri
Copy link
Member

@jan-est Feel free to include the commit here

@jan-est jan-est force-pushed the kcp-rollout-strategy-backport branch from 91944aa to be7ea2f Compare March 29, 2021 06:22
@jan-est
Copy link
Contributor Author

jan-est commented Mar 29, 2021

@jan-est Feel free to include the commit here

4376 changes added to this PR.

@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 Mar 29, 2021
@fabriziopandini
Copy link
Member

/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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 30, 2021
@k8s-ci-robot k8s-ci-robot merged commit d731781 into kubernetes-sigs:release-0.3 Mar 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants