From 2a000d43d444edd70dfcc8d5f70f9231ddf42572 Mon Sep 17 00:00:00 2001 From: Hiromu Asahina Date: Wed, 15 Feb 2023 01:15:57 +0900 Subject: [PATCH] Apply suggestions from code review Co-authored-by: Jonathan Tong --- cmd/clusterctl/client/alpha/rollout_viewer.go | 34 ++++--- .../client/alpha/rollout_viewer_test.go | 98 ++++++++++--------- cmd/clusterctl/cmd/rollout/history.go | 2 +- go.mod | 2 +- 4 files changed, 78 insertions(+), 58 deletions(-) diff --git a/cmd/clusterctl/client/alpha/rollout_viewer.go b/cmd/clusterctl/client/alpha/rollout_viewer.go index fb6a65e3f878..db80f848c293 100644 --- a/cmd/clusterctl/client/alpha/rollout_viewer.go +++ b/cmd/clusterctl/client/alpha/rollout_viewer.go @@ -1,5 +1,5 @@ /* -Copyright 2022 The Kubernetes Authors. +Copyright 2023 The Kubernetes Authors. Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. @@ -25,7 +25,7 @@ import ( "github.com/olekukonko/tablewriter" "github.com/pkg/errors" - "gopkg.in/yaml.v2" + "gopkg.in/yaml.v3" corev1 "k8s.io/api/core/v1" clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1" @@ -34,6 +34,11 @@ import ( "sigs.k8s.io/cluster-api/internal/controllers/machinedeployment/mdutil" ) +type historyInfo struct { + revisions string + changeCause string +} + // ObjectViewer will issue a view on the specified cluster-api resource. func (r *rollout) ObjectViewer(ctx context.Context, proxy cluster.Proxy, ref corev1.ObjectReference, revision int64) error { switch ref.Kind { @@ -66,7 +71,7 @@ func viewMachineDeployment(ctx context.Context, proxy cluster.Proxy, d *clusterv if revision > 0 { ms, err := findMachineDeploymentRevision(revision, msList) if err != nil { - return errors.Errorf("unable to find the spcified revision") + return errors.Errorf("unable to find the specified revision %d for MachineDeployment %s", revision, d.Name) } output, err := yaml.Marshal(ms.Spec.Template) if err != nil { @@ -78,26 +83,33 @@ func viewMachineDeployment(ctx context.Context, proxy cluster.Proxy, d *clusterv // Print an overview of all revisions // Create a revisionToChangeCause map - historyInfo := make(map[int64]string) + histInfo := make(map[int64]historyInfo) for _, ms := range msList { v, err := mdutil.Revision(ms) if err != nil { - log.V(7).Error(err, fmt.Sprintf("unable to get revision from machineset %s for machinedeployment %s in namespace %s", ms.Name, d.Name, d.Namespace)) + log.Error(err, fmt.Sprintf("unable to get revision from machineset %s for machinedeployment %s in namespace %s", ms.Name, d.Name, d.Namespace)) continue } - historyInfo[v] = ms.Annotations[clusterv1.ChangeCauseAnnotation] + revisions := strconv.FormatInt(v, 10) + if revHistory := ms.Annotations[clusterv1.RevisionHistoryAnnotation]; revHistory != "" { + revisions = revHistory + "," + revisions + } + histInfo[v] = historyInfo{ + revisions, + ms.Annotations[clusterv1.ChangeCauseAnnotation], + } } // Sort the revisions - revisions := make([]int64, 0, len(historyInfo)) - for r := range historyInfo { + revisions := make([]int64, 0, len(histInfo)) + for r := range histInfo { revisions = append(revisions, r) } sort.Slice(revisions, func(i, j int) bool { return revisions[i] < revisions[j] }) // Output the revisionToChangeCause map table := tablewriter.NewWriter(os.Stdout) - table.SetHeader([]string{"REVISION", "CHANGE-CAUSE"}) + table.SetHeader([]string{"REVISIONS", "CHANGE-CAUSE"}) table.SetHeaderAlignment(tablewriter.ALIGN_LEFT) table.SetAlignment(tablewriter.ALIGN_LEFT) table.SetCenterSeparator("") @@ -107,12 +119,12 @@ func viewMachineDeployment(ctx context.Context, proxy cluster.Proxy, d *clusterv table.SetBorder(false) for _, r := range revisions { - changeCause := historyInfo[r] + changeCause := histInfo[r].changeCause if changeCause == "" { changeCause = "" } table.Append([]string{ - strconv.FormatInt(r, 10), + histInfo[r].revisions, changeCause, }) } diff --git a/cmd/clusterctl/client/alpha/rollout_viewer_test.go b/cmd/clusterctl/client/alpha/rollout_viewer_test.go index f9b970d8aa8b..4408d43a99b4 100644 --- a/cmd/clusterctl/client/alpha/rollout_viewer_test.go +++ b/cmd/clusterctl/client/alpha/rollout_viewer_test.go @@ -1,5 +1,5 @@ /* -Copyright 2020 The Kubernetes Authors. +Copyright 2023 The Kubernetes Authors. Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. @@ -33,7 +33,7 @@ import ( "sigs.k8s.io/cluster-api/cmd/clusterctl/internal/test" ) -func captureStdout(fnc func()) string { +func captureStdout(fnc func()) (string, error) { r, w, _ := os.Pipe() stdout := os.Stdout @@ -46,8 +46,11 @@ func captureStdout(fnc func()) string { _ = w.Close() var buf bytes.Buffer - _, _ = io.Copy(&buf, r) - return buf.String() + _, err := io.Copy(&buf, r) + if err != nil { + return "", err + } + return buf.String(), nil } func Test_ObjectViewer(t *testing.T) { @@ -102,8 +105,9 @@ func Test_ObjectViewer(t *testing.T) { }, Labels: labels, Annotations: map[string]string{ - clusterv1.RevisionAnnotation: "1", - clusterv1.ChangeCauseAnnotation: "update to the latest version", + clusterv1.RevisionAnnotation: "11", + clusterv1.RevisionHistoryAnnotation: "1,3,5,7,9", + clusterv1.ChangeCauseAnnotation: "update to the latest version", }, }, }, @@ -119,7 +123,8 @@ func Test_ObjectViewer(t *testing.T) { }, Labels: labels, Annotations: map[string]string{ - clusterv1.RevisionAnnotation: "3", + clusterv1.RevisionAnnotation: "10", + clusterv1.RevisionHistoryAnnotation: "4,6,8", }, }, }, @@ -146,10 +151,10 @@ func Test_ObjectViewer(t *testing.T) { Namespace: namespace, }, }, - expectedOutput: ` REVISION CHANGE-CAUSE - 1 update to the latest version - 2 - 3 + expectedOutput: ` REVISIONS CHANGE-CAUSE + 2 + 4,6,8,10 + 1,3,5,7,9,11 update to the latest version `, }, { @@ -164,7 +169,7 @@ func Test_ObjectViewer(t *testing.T) { Namespace: namespace, }, }, - expectedOutput: ` REVISION CHANGE-CAUSE + expectedOutput: ` REVISIONS CHANGE-CAUSE `, }, { @@ -253,39 +258,39 @@ func Test_ObjectViewer(t *testing.T) { revision: int64(999), }, expectedOutput: `objectmeta: - labels: - cluster.x-k8s.io/cluster-name: test - annotations: - foo: bar + labels: + cluster.x-k8s.io/cluster-name: test + annotations: + foo: bar spec: - clustername: test - bootstrap: - configref: - kind: KubeadmConfigTemplate - namespace: default - name: md-template - uid: "" - apiversion: bootstrap.cluster.x-k8s.io/v1beta1 - resourceversion: "" - fieldpath: "" - datasecretname: secret-name - infrastructureref: - kind: InfrastructureMachineTemplate - namespace: default - name: md-template - uid: "" - apiversion: infrastructure.cluster.x-k8s.io/v1beta1 - resourceversion: "" - fieldpath: "" - version: v1.25.1 - providerid: test://id-1 - failuredomain: one - nodedraintimeout: - duration: 0s - nodevolumedetachtimeout: - duration: 0s - nodedeletiontimeout: - duration: 0s + clustername: test + bootstrap: + configref: + kind: KubeadmConfigTemplate + namespace: default + name: md-template + uid: "" + apiversion: bootstrap.cluster.x-k8s.io/v1beta1 + resourceversion: "" + fieldpath: "" + datasecretname: secret-name + infrastructureref: + kind: InfrastructureMachineTemplate + namespace: default + name: md-template + uid: "" + apiversion: infrastructure.cluster.x-k8s.io/v1beta1 + resourceversion: "" + fieldpath: "" + version: v1.25.1 + providerid: test://id-1 + failuredomain: one + nodedraintimeout: + duration: 0s + nodevolumedetachtimeout: + duration: 0s + nodedeletiontimeout: + duration: 0s `, }, { @@ -325,7 +330,7 @@ spec: g := NewWithT(t) r := newRolloutClient() proxy := test.NewFakeProxy().WithObjs(tt.fields.objs...) - output := captureStdout(func() { + output, err := captureStdout(func() { err := r.ObjectViewer(context.Background(), proxy, tt.fields.ref, tt.fields.revision) if tt.wantErr { g.Expect(err).To(HaveOccurred()) @@ -333,6 +338,9 @@ spec: g.Expect(err).ToNot(HaveOccurred()) } }) + if err != nil { + t.Fatalf("unable to captureStdout") + } g.Expect(output).To(Equal(tt.expectedOutput)) }) } diff --git a/cmd/clusterctl/cmd/rollout/history.go b/cmd/clusterctl/cmd/rollout/history.go index 791db01926c8..add5fe896403 100644 --- a/cmd/clusterctl/cmd/rollout/history.go +++ b/cmd/clusterctl/cmd/rollout/history.go @@ -1,5 +1,5 @@ /* -Copyright 2022 The Kubernetes Authors. +Copyright 2023 The Kubernetes Authors. Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. diff --git a/go.mod b/go.mod index 01a4e4f558f7..3377dbfcf9a2 100644 --- a/go.mod +++ b/go.mod @@ -171,7 +171,7 @@ require ( gopkg.in/ini.v1 v1.67.0 // indirect gopkg.in/natefinch/lumberjack.v2 v2.2.1 // indirect gopkg.in/yaml.v2 v2.4.0 // indirect - gopkg.in/yaml.v3 v3.0.1 // indirect + gopkg.in/yaml.v3 v3.0.1 k8s.io/cli-runtime v0.28.1 // indirect k8s.io/component-helpers v0.28.1 // indirect k8s.io/kms v0.28.1 // indirect