Apply suggestions from code review

Co-authored-by: Jonathan Tong <[email protected]>

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.
@@ -24,7 +24,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"
@@ -33,6 +33,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(proxy cluster.Proxy, ref corev1.ObjectReference, revision int64) error {
 	switch ref.Kind {
@@ -65,7 +70,7 @@ func viewMachineDeployment(proxy cluster.Proxy, d *clusterv1.MachineDeployment,
 	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 {
@@ -77,26 +82,33 @@ func viewMachineDeployment(proxy cluster.Proxy, d *clusterv1.MachineDeployment,
 
 	// 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("")
@@ -106,12 +118,12 @@ func viewMachineDeployment(proxy cluster.Proxy, d *clusterv1.MachineDeployment,
 	table.SetBorder(false)
 
 	for _, r := range revisions {
-		changeCause := historyInfo[r]
+		changeCause := histInfo[r].changeCause
 		if changeCause == "" {
 			changeCause = "<none>"
 		}
 		table.Append([]string{
-			strconv.FormatInt(r, 10),
+			histInfo[r].revisions,
 			changeCause,
 		})
 	}

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.
@@ -32,7 +32,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
@@ -45,8 +45,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) {
@@ -101,8 +104,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",
 							},
 						},
 					},
@@ -118,7 +122,8 @@ func Test_ObjectViewer(t *testing.T) {
 							},
 							Labels: labels,
 							Annotations: map[string]string{
-								clusterv1.RevisionAnnotation: "3",
+								clusterv1.RevisionAnnotation:        "10",
+								clusterv1.RevisionHistoryAnnotation: "4,6,8",
 							},
 						},
 					},
@@ -145,10 +150,10 @@ func Test_ObjectViewer(t *testing.T) {
 					Namespace: namespace,
 				},
 			},
-			expectedOutput: `  REVISION  CHANGE-CAUSE                  
-  1         update to the latest version  
-  2         <none>                        
-  3         <none>                        
+			expectedOutput: `  REVISIONS     CHANGE-CAUSE                  
+  2             <none>                        
+  4,6,8,10      <none>                        
+  1,3,5,7,9,11  update to the latest version  
 `,
 		},
 		{
@@ -163,7 +168,7 @@ func Test_ObjectViewer(t *testing.T) {
 					Namespace: namespace,
 				},
 			},
-			expectedOutput: `  REVISION  CHANGE-CAUSE  
+			expectedOutput: `  REVISIONS  CHANGE-CAUSE  
 `,
 		},
 		{
@@ -252,39 +257,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
 `,
 		},
 		{
@@ -324,14 +329,17 @@ spec:
 			g := NewWithT(t)
 			r := newRolloutClient()
 			proxy := test.NewFakeProxy().WithObjs(tt.fields.objs...)
-			output := captureStdout(func() {
+			output, err := captureStdout(func() {
 				err := r.ObjectViewer(proxy, tt.fields.ref, tt.fields.revision)
 				if tt.wantErr {
 					g.Expect(err).To(HaveOccurred())
 				} else {
 					g.Expect(err).ToNot(HaveOccurred())
 				}
 			})
+			if err != nil {
+				t.Fatalf("unable to captureStdout")
+			}
 			g.Expect(output).To(Equal(tt.expectedOutput))
 		})
 	}

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.

go.mod

@@ -133,8 +133,8 @@ require (
 	google.golang.org/protobuf v1.30.0 // indirect
 	gopkg.in/inf.v0 v0.9.1 // indirect
 	gopkg.in/ini.v1 v1.67.0 // indirect
-	gopkg.in/yaml.v2 v2.4.0
-	gopkg.in/yaml.v3 v3.0.1 // indirect
+	gopkg.in/yaml.v2 v2.4.0 //indirect
+	gopkg.in/yaml.v3 v3.0.1
 	k8s.io/cli-runtime v0.27.2 // indirect
 	k8s.io/kube-openapi v0.0.0-20230501164219-8b0f38b5fd1f
 	sigs.k8s.io/json v0.0.0-20221116044647-bc3834ca7abd // indirect