From 79b13f53f4e8831bb1187f3fb188c28932f50b69 Mon Sep 17 00:00:00 2001 From: Tobias Giese Date: Thu, 30 Jan 2025 22:16:17 +0100 Subject: [PATCH] Fix multiline Ready condition in clusterctl describe The multiline feature in clusterctl has a bug in the multiline conditions. If the Ready condition message has multipile lines the multiline prefix is not calculated properly. To fix this we can use the same logic as for the other conditions and verify if the object is the last object in the tree. Signed-off-by: Tobias Giese --- cmd/clusterctl/client/tree/tree.go | 34 ++++++ cmd/clusterctl/cmd/describe_cluster.go | 55 +++++++-- cmd/clusterctl/cmd/describe_cluster_test.go | 122 ++++++++++++++++++++ 3 files changed, 203 insertions(+), 8 deletions(-) diff --git a/cmd/clusterctl/client/tree/tree.go b/cmd/clusterctl/client/tree/tree.go index 51c463d881d6..5f78bcfc08c9 100644 --- a/cmd/clusterctl/client/tree/tree.go +++ b/cmd/clusterctl/client/tree/tree.go @@ -264,6 +264,40 @@ func (od ObjectTree) GetObjectsByParent(id types.UID) []client.Object { return out } +// IsLastObject returns true if the given UID is the last object in the ObjectTree. +func (od ObjectTree) IsLastObject(childrenObjects []client.Object, id types.UID) bool { + // Check if the target UID exists and has no children + if children, exists := od.ownership[id]; exists && len(children) > 0 { + return false + } + + // Ensure it's not referenced as a child in the child objects. + var isLast bool + for _, children := range childrenObjects { + if children.GetUID() == id { + isLast = true + continue + } + isLast = false + } + + // If it has no children, it's the last item. + if len(childrenObjects) == 0 { + isLast = true + } + + // Ensure it's not referenced as a child in the root items. + for _, children := range od.items { + if children.GetUID() == id { + isLast = true + continue + } + isLast = false + } + + return isLast +} + func hasSameAvailableReadyUptoDateStatusAndReason(availableA, availableB, readyA, readyB, upToDateA, upToDateB *metav1.Condition) bool { if !hasSameStatusAndReason(availableA, availableB) { return false diff --git a/cmd/clusterctl/cmd/describe_cluster.go b/cmd/clusterctl/cmd/describe_cluster.go index 2744fd0934ca..887bd95045ea 100644 --- a/cmd/clusterctl/cmd/describe_cluster.go +++ b/cmd/clusterctl/cmd/describe_cluster.go @@ -282,6 +282,8 @@ func addObjectRowV1Beta2(prefix string, tbl *tablewriter.Table, objectTree *tree rowDescriptor.message = "" } + multilinePrefix := getMultilinePrefix(obj, objectTree, prefix) + // Add the row representing the object that includes // - The row name with the tree view prefix. // - Replica counters @@ -306,7 +308,7 @@ func addObjectRowV1Beta2(prefix string, tbl *tablewriter.Table, objectTree *tree for _, m := range msg[1:] { tbl.Append([]string{ - getMultilinePrefix(gray.Sprint(prefix)), + gray.Sprint(multilinePrefix), "", "", "", @@ -324,7 +326,14 @@ func addObjectRowV1Beta2(prefix string, tbl *tablewriter.Table, objectTree *tree // Add a row for each object's children, taking care of updating the tree view prefix. childrenObj := objectTree.GetObjectsByParent(obj.GetUID()) + orderChildrenObjects(childrenObj) + + for i, child := range childrenObj { + addObjectRowV1Beta2(getChildPrefix(prefix, i, len(childrenObj)), tbl, objectTree, child) + } +} +func orderChildrenObjects(childrenObj []ctrlclient.Object) { // printBefore returns true if children[i] should be printed before children[j]. Objects are sorted by z-order and // row name such that objects with higher z-order are printed first, and objects with the same z-order are // printed in alphabetical order. @@ -336,10 +345,6 @@ func addObjectRowV1Beta2(prefix string, tbl *tablewriter.Table, objectTree *tree return tree.GetZOrder(childrenObj[i]) > tree.GetZOrder(childrenObj[j]) } sort.Slice(childrenObj, printBefore) - - for i, child := range childrenObj { - addObjectRowV1Beta2(getChildPrefix(prefix, i, len(childrenObj)), tbl, objectTree, child) - } } // addObjectRow add a row for a given object, and recursively for all the object's children. @@ -452,7 +457,7 @@ func addOtherConditionsV1Beta2(prefix string, tbl *tablewriter.Table, objectTree for _, m := range msg[1:] { tbl.Append([]string{ - gray.Sprint(getMultilinePrefix(childPrefix)), + gray.Sprint(getMultilineConditionPrefix(childPrefix)), "", "", "", @@ -510,8 +515,8 @@ func getChildPrefix(currentPrefix string, childIndex, childCount int) string { return nextPrefix + lastElemPrefix } -// getMultilinePrefix return the tree view prefix for a multiline condition. -func getMultilinePrefix(currentPrefix string) string { +// getMultilineConditionPrefix return the tree view prefix for a multiline condition. +func getMultilineConditionPrefix(currentPrefix string) string { // All ├─ should be replaced by |, so all the existing hierarchic dependencies are carried on if strings.HasSuffix(currentPrefix, firstElemPrefix) { return strings.TrimSuffix(currentPrefix, firstElemPrefix) + pipe @@ -524,6 +529,40 @@ func getMultilinePrefix(currentPrefix string) string { return "?" } +// getRootMultiLinePrefix return the tree view prefix for a multiline condition on the object. +func getMultilinePrefix(obj ctrlclient.Object, objectTree *tree.ObjectTree, prefix string) string { + // If it is the last object we can return early with an empty string. + childrenObj := objectTree.GetObjectsByParent(obj.GetUID()) + orderChildrenObjects(childrenObj) + if objectTree.IsLastObject(childrenObj, obj.GetUID()) { + return "" + } + + multilinePrefix := getMultilineConditionPrefix(prefix) + + // If it is the top-level root object, set the multiline prefix to a pipe. + if prefix == "" { + multilinePrefix = pipe + } + + // If the multiline prefix is empty, we have to ensure that even multiline conditions on this root object are indented. + if strings.TrimSpace(multilinePrefix) == "" { + filler := strings.Repeat(" ", 10) + childrenPipe := indent + if objectTree.IsObjectWithChild(obj.GetUID()) { + childrenPipe = pipe + } + // We are doing a lightweight calculation of the other conditions multilinePrefix here. + multilinePrefix = prefix + childrenPipe + filler + // All ├─ should be replaced by |, so all the existing hierarchic dependencies are carried on + multilinePrefix = strings.ReplaceAll(multilinePrefix, firstElemPrefix, pipe) + // All └─ should be replaced by " " because we are under the last element of the tree (nothing to carry on) + multilinePrefix = strings.ReplaceAll(multilinePrefix, lastElemPrefix, strings.Repeat(" ", len([]rune(lastElemPrefix)))) + } + + return multilinePrefix +} + // getRowName returns the object name in the tree view according to following rules: // - group objects are represented as #of objects kind, e.g. 3 Machines... // - other virtual objects are represented using the object name, e.g. Workers, or meta name if provided. diff --git a/cmd/clusterctl/cmd/describe_cluster_test.go b/cmd/clusterctl/cmd/describe_cluster_test.go index 2311db9ff05f..1548324ab138 100644 --- a/cmd/clusterctl/cmd/describe_cluster_test.go +++ b/cmd/clusterctl/cmd/describe_cluster_test.go @@ -294,6 +294,121 @@ func Test_TreePrefix(t *testing.T) { } } +func Test_V1Beta2TreePrefix(t *testing.T) { + tests := []struct { + name string + objectTree *tree.ObjectTree + expectPrefix []string + }{ + { + name: "Conditions should get the right prefix with multiline message", + objectTree: func() *tree.ObjectTree { + root := fakeObject("root", + withV1Beta2Condition(metav1.Condition{ + Type: "Available", + Status: metav1.ConditionFalse, + Reason: "NotAvailable", + Message: "1\n2", + }), + ) + obectjTree := tree.NewObjectTree(root, tree.ObjectTreeOptions{ + V1Beta2: true, + }) + + o1 := fakeObject("child1", + withV1Beta2Condition(metav1.Condition{Type: "Available", + Status: metav1.ConditionFalse, + Reason: "NotAvailable", + Message: "1\n2", + }), + ) + + o2 := fakeObject("child2", + withV1Beta2Condition(metav1.Condition{Type: "Available", + Status: metav1.ConditionFalse, + Reason: "NotAvailable", + Message: "1\n2", + }), + ) + obectjTree.Add(root, o1) + obectjTree.Add(root, o2) + return obectjTree + }(), + expectPrefix: []string{ + "Object/root", + "│ 2", + "├─Object/child1", + "│ 2", + "└─Object/child2", + }, + }, + { + name: "Conditions should get the right prefix with multiline message and a child", + objectTree: func() *tree.ObjectTree { + root := fakeObject("root", + withV1Beta2Condition(metav1.Condition{ + Type: "Available", + Status: metav1.ConditionTrue, + Reason: "Available", + }), + ) + obectjTree := tree.NewObjectTree(root, tree.ObjectTreeOptions{ + V1Beta2: true, + }) + + o1 := fakeObject("child1", + withV1Beta2Condition(metav1.Condition{ + Type: "Available", + Status: metav1.ConditionTrue, + Reason: "Available", + }), + ) + + o2 := fakeObject("child2", + withV1Beta2Condition(metav1.Condition{ + Type: "Available", + Status: metav1.ConditionFalse, + Reason: "NotAvailable", + Message: "1\n2", + }), + ) + o2_1 := fakeObject("child2.1") + obectjTree.Add(root, o1) + obectjTree.Add(root, o2) + obectjTree.Add(o2, o2_1) + return obectjTree + }(), + expectPrefix: []string{ + "Object/root", + "├─Object/child1", + "└─Object/child2", + " │ 2", + " └─Object/child2.1", + }, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + g := NewWithT(t) + var output bytes.Buffer + + // Creates the output table + tbl := tablewriter.NewWriter(&output) + + formatTableTreeV1Beta2(tbl) + + // Add row for the root object, the cluster, and recursively for all the nodes representing the cluster status. + addObjectRowV1Beta2("", tbl, tt.objectTree, tt.objectTree.GetRoot()) + tbl.Render() + + // remove empty lines from the output. We need this because v1beta2 adds lines at the beginning and end. + outputString := strings.TrimSpace(output.String()) + + g.Expect(outputString).Should(MatchTable(tt.expectPrefix)) + }) + } +} + type objectOption func(object ctrlclient.Object) func fakeObject(name string, options ...objectOption) ctrlclient.Object { @@ -331,6 +446,13 @@ func withCondition(c *clusterv1.Condition) func(ctrlclient.Object) { } } +func withV1Beta2Condition(c metav1.Condition) func(ctrlclient.Object) { + return func(m ctrlclient.Object) { + cluster := m.(*clusterv1.Cluster) + cluster.SetV1Beta2Conditions([]metav1.Condition{c}) + } +} + func withDeletionTimestamp(object ctrlclient.Object) { now := metav1.Now() object.SetDeletionTimestamp(&now)