✨ Add command and client for `clusterctl alpha rollout restart` #3838

Arvinderpal · 2020-10-21T16:40:03Z

What this PR does / why we need it:
Adds command and client for the clusterctl alpha rollout restart.
Also implements restart sub-command for MachineDeployments.

Fixes:
#3401

Related Issue:
#3439

k8s-ci-robot · 2020-10-21T16:40:12Z

Hi @Arvinderpal. 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.

Arvinderpal · 2020-10-21T16:40:57Z

@fabriziopandini @wfernandes @vincepri
Can I get an early review of this; in particular, how I have structured the code for alpha features?

I still need to do other things like unit tests and clean up, but you should be able to execute the restart command:
clusterctl alpha rollout restart --namespace default machinedeployment/test-md-0

fabriziopandini · 2020-10-23T09:54:42Z

/ok-to-test

Arvinderpal · 2020-10-23T16:07:00Z

cmd/clusterctl/client/client.go

@@ -65,6 +65,15 @@ type Client interface {
 	// ProcessYAML provides a direct way to process a yaml and inspect its
 	// variables.
 	ProcessYAML(options ProcessYAMLOptions) (YamlPrinter, error)
+
+	// Interface for alpha features in clusterctl
+	AlphaClient


@joelanford @alexeldeib Can you help me make sense of why this is flagged as an "incompatible change" with go-apidiff? I also tried removing the embedded interface and just putting the func directly in Client interface, but similar issue.

*** Running go-apidiff *** sigs.k8s.io/cluster-api/cmd/clusterctl/client Incompatible changes: - AlphaClient.RolloutRestart: added Compatible changes: - (*clusterctlClient).RolloutRestart: added - AlphaClient: added - RolloutRestartOptions: added sigs.k8s.io/cluster-api/cmd/clusterctl/client/rollout Compatible changes: - ObjectRestarter: added sigs.k8s.io/cluster-api/cmd/clusterctl/cmd/rollout Compatible changes: - NewCmdRolloutRestart: added + EXIT_VALUE=1 + set +o xtrace

https://storage.googleapis.com/kubernetes-jenkins/pr-logs/pull/kubernetes-sigs_cluster-api/3838/pull-cluster-api-apidiff-main/1319649725397340160/build-log.txt

The test is more advisory than required -- it's okay that it fails for PRs to be merged some times. The PR does make all of the changes mentioned, but they all seem reasonable to me (actually a bit surprised the first one is incompatible and the second one is compatible, would have expected those to be flipped since adding to an existing interface would require implementers to update their code).

Ah okay. Unless others see any problems with what I'm doing, I'll just ignore this for now.

Arvinderpal · 2020-12-08T16:22:53Z

/assign @vincepri

wfernandes · 2020-12-08T16:26:27Z

I can take another look at this sometime today.
/assign @wfernandes

vincepri · 2020-12-08T16:26:44Z

LGTM
@wfernandes @fabriziopandini over to you

wfernandes

Just one nit. We should also add some documentation for this command in the CAPI book. See https://github.com/kubernetes-sigs/cluster-api/blob/master/docs/book/src/clusterctl/overview.md and
https://github.com/kubernetes-sigs/cluster-api/blob/master/docs/book/src/clusterctl/commands/commands.md

cmd/clusterctl/cmd/rollout.go

Arvinderpal · 2020-12-08T21:06:01Z

Just one nit. We should also add some documentation for this command in the CAPI book. See https://github.com/kubernetes-sigs/cluster-api/blob/master/docs/book/src/clusterctl/overview.md and
https://github.com/kubernetes-sigs/cluster-api/blob/master/docs/book/src/clusterctl/commands/commands.md

If you don't mind, can I update the docs in a followup PR?
I have created a task for this in the tracking issue: #3439

Also, I spoke with @fabriziopandini on slack. He didn't feel the need to review as long as you and @vincepri had done so.

If the tests pass, I think we should go ahead and merge.

wfernandes · 2020-12-08T21:58:37Z

/lgtm

wfernandes · 2020-12-08T21:59:24Z

/test pull-cluster-api-test-main

wfernandes · 2020-12-08T22:50:04Z

cmd/clusterctl/client/rollout_test.go

+	md1 := &clusterv1.MachineDeployment{
+		TypeMeta: metav1.TypeMeta{
+			Kind:       "MachineDeployment",
+			APIVersion: "cluster.x-k8s.io/v1alpha3",


From the test failures, you may need to update these to v1alpha4. However, I didn't think we needed the APIVersion specified, so I think you can remove this.

wfernandes · 2020-12-08T23:59:19Z

@Arvinderpal Ok..I was able to reproduce this failure by rebasing this branch with the main branch. I think the import in the rollout_test should be sigs.k8s.io/cluster-api/api/v1alpha4 but I haven't confirmed that fix yet.

wfernandes · 2020-12-09T00:05:13Z

@Arvinderpal Yeah..I think that's it. We recently bumped to using v1alpha4 types everywhere instead of v1alpha3. Please change all the sigs.k8s.io/cluster-api/api/v1alpha3 imports to sigs.k8s.io/cluster-api/api/v1alpha4.
Sorry missed this in the review 😄

Arvinderpal · 2020-12-09T00:47:43Z

@Arvinderpal Yeah..I think that's it. We recently bumped to using v1alpha4 types everywhere instead of v1alpha3. Please change all the sigs.k8s.io/cluster-api/api/v1alpha3 imports to sigs.k8s.io/cluster-api/api/v1alpha4.

Thanks for pointing that out. Just made the change. 🤞

k8s-ci-robot · 2020-12-09T00:54:37Z

@Arvinderpal: The following test failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
pull-cluster-api-apidiff-main	`970212f`	link	`/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/test-infra repository. I understand the commands that are listed here.

wfernandes · 2020-12-09T17:23:11Z

/lgtm

Arvinderpal · 2020-12-09T18:51:53Z

@vincepri should we merge this?

vincepri · 2020-12-15T21:03:57Z

/approve

k8s-ci-robot · 2020-12-15T21:04:08Z

[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:

~~OWNERS~~ [vincepri]

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 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. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Oct 21, 2020

k8s-ci-robot requested review from CecileRobertMichon and JoelSpeed October 21, 2020 16:40

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Oct 21, 2020

Arvinderpal mentioned this pull request Oct 21, 2020

clusterctl rollout #3439

Closed

9 tasks

Arvinderpal changed the title ~~WIP: ✨ Add command and client for clusterctl alpha rollout restart~~ ✨ WIP: Add command and client for clusterctl alpha rollout restart Oct 22, 2020

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 Oct 22, 2020

Arvinderpal force-pushed the clusterctl-alpha-rollout branch from 1b33672 to 841edff Compare October 22, 2020 20:53

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 Oct 22, 2020

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 Oct 23, 2020

Arvinderpal force-pushed the clusterctl-alpha-rollout branch from 841edff to 9cd49db Compare October 23, 2020 14:33

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 23, 2020

Arvinderpal force-pushed the clusterctl-alpha-rollout branch from 9cd49db to f451f00 Compare October 23, 2020 14:39

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 23, 2020

Arvinderpal force-pushed the clusterctl-alpha-rollout branch 2 times, most recently from 027e433 to 3b54548 Compare October 23, 2020 16:02

Arvinderpal commented Oct 23, 2020

View reviewed changes

Arvinderpal force-pushed the clusterctl-alpha-rollout branch 4 times, most recently from 8d3157a to a6e0851 Compare October 24, 2020 21:35

superbrothers mentioned this pull request Oct 26, 2020

clusterctl completion: Provide dynamic auto completion #3867

Closed

Arvinderpal changed the title ~~✨ WIP: Add command and client for clusterctl alpha rollout restart~~ ✨ Add command and client for clusterctl alpha rollout restart Oct 26, 2020

k8s-ci-robot assigned vincepri Dec 8, 2020

wfernandes reviewed Dec 8, 2020

View reviewed changes

cmd/clusterctl/cmd/rollout.go Outdated Show resolved Hide resolved

Arvinderpal force-pushed the clusterctl-alpha-rollout branch from e013039 to 3fdbc53 Compare December 8, 2020 20:59

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 8, 2020

wfernandes reviewed Dec 8, 2020

View reviewed changes

Arvinderpal force-pushed the clusterctl-alpha-rollout branch from 3fdbc53 to 73c047c Compare December 8, 2020 23:23

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 8, 2020

Arvinderpal force-pushed the clusterctl-alpha-rollout branch from 73c047c to 9a39493 Compare December 8, 2020 23:56

Arvinderpal force-pushed the clusterctl-alpha-rollout branch from 9a39493 to e2c12b5 Compare December 9, 2020 00:46

Add command and client for clusterctl alpha rollout.

970212f

Arvinderpal force-pushed the clusterctl-alpha-rollout branch from e2c12b5 to 970212f Compare December 9, 2020 00:52

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 9, 2020

fabriziopandini linked an issue Dec 10, 2020 that may be closed by this pull request

Provide "at glance" view of cluster conditions #3802

Closed

fabriziopandini removed a link to an issue Dec 10, 2020

Provide "at glance" view of cluster conditions #3802

Closed

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 15, 2020

k8s-ci-robot merged commit e6c20f7 into kubernetes-sigs:master Dec 15, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ Add command and client for `clusterctl alpha rollout restart` #3838

✨ Add command and client for `clusterctl alpha rollout restart` #3838

Arvinderpal commented Oct 21, 2020 •

edited

Loading

k8s-ci-robot commented Oct 21, 2020

Arvinderpal commented Oct 21, 2020 •

edited

Loading

fabriziopandini commented Oct 23, 2020

Arvinderpal Oct 23, 2020

alexeldeib Oct 23, 2020

Arvinderpal Oct 23, 2020

Arvinderpal commented Dec 8, 2020

wfernandes commented Dec 8, 2020

vincepri commented Dec 8, 2020

wfernandes left a comment

Arvinderpal commented Dec 8, 2020

wfernandes commented Dec 8, 2020

wfernandes commented Dec 8, 2020

wfernandes Dec 8, 2020

wfernandes commented Dec 8, 2020 •

edited

Loading

wfernandes commented Dec 9, 2020

Arvinderpal commented Dec 9, 2020

k8s-ci-robot commented Dec 9, 2020 •

edited

Loading

wfernandes commented Dec 9, 2020

Arvinderpal commented Dec 9, 2020

vincepri commented Dec 15, 2020

k8s-ci-robot commented Dec 15, 2020

✨ Add command and client for clusterctl alpha rollout restart #3838

✨ Add command and client for clusterctl alpha rollout restart #3838

Conversation

Arvinderpal commented Oct 21, 2020 • edited Loading

k8s-ci-robot commented Oct 21, 2020

Arvinderpal commented Oct 21, 2020 • edited Loading

fabriziopandini commented Oct 23, 2020

Arvinderpal Oct 23, 2020

Choose a reason for hiding this comment

alexeldeib Oct 23, 2020

Choose a reason for hiding this comment

Arvinderpal Oct 23, 2020

Choose a reason for hiding this comment

Arvinderpal commented Dec 8, 2020

wfernandes commented Dec 8, 2020

vincepri commented Dec 8, 2020

wfernandes left a comment

Choose a reason for hiding this comment

Arvinderpal commented Dec 8, 2020

wfernandes commented Dec 8, 2020

wfernandes commented Dec 8, 2020

wfernandes Dec 8, 2020

Choose a reason for hiding this comment

wfernandes commented Dec 8, 2020 • edited Loading

wfernandes commented Dec 9, 2020

Arvinderpal commented Dec 9, 2020

k8s-ci-robot commented Dec 9, 2020 • edited Loading

wfernandes commented Dec 9, 2020

Arvinderpal commented Dec 9, 2020

vincepri commented Dec 15, 2020

k8s-ci-robot commented Dec 15, 2020

✨ Add command and client for `clusterctl alpha rollout restart` #3838

✨ Add command and client for `clusterctl alpha rollout restart` #3838

Arvinderpal commented Oct 21, 2020 •

edited

Loading

Arvinderpal commented Oct 21, 2020 •

edited

Loading

wfernandes commented Dec 8, 2020 •

edited

Loading

k8s-ci-robot commented Dec 9, 2020 •

edited

Loading