🐛 Fix multiline Ready condition in clusterctl describe for v1beta2 #11781

tobiasgiese · 2025-02-01T10:52:57Z

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.

What this PR does / why we need it:

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #11782

/area clusterctl

tobiasgiese · 2025-02-01T10:53:32Z

cmd/clusterctl/cmd/describe_cluster_test.go

+				"Object/root",
+				"│                                                              2",
+				"├─Object/child1",
+				"│                                                              2",
+				"└─Object/child2",


Without the fix (where ? is no prefix)

Object/root Available: False NotAvailable 292y 1 ? 2 ├─Object/child1 Available: False NotAvailable 292y 1 │ 2 └─Object/child2 Available: False NotAvailable 292y 1 2

tobiasgiese · 2025-02-01T10:54:05Z

cmd/clusterctl/cmd/describe_cluster_test.go

+				"Object/root",
+				"├─Object/child1",
+				"└─Object/child2",
+				"  │                                                                2",
+				"  └─Object/child2.1",


Without the fix

Object/root Available: True Available 292y ├─Object/child1 Available: True Available 292y └─Object/child2 Available: False NotAvailable 292y 1 2 └─Object/child2.1

tobiasgiese · 2025-02-02T10:54:03Z

cmd/clusterctl/cmd/describe_cluster_test.go

+				"Object/root",
+				"├─Object/child1",
+				"├─Object/child2",
+				"│ │                                                                2",
+				"│ └─Object/child2.1",
+				"│                                                                  2",
+				"└─Object/child3",
+				"  │                                                                2",
+				"  └─Object/child3.1",
+				"                                                                   2",


Without the fix

Object/root Available: True Available 292y ├─Object/child1 Available: True Available 292y ├─Object/child2 Available: False NotAvailable 292y 1 │ 2 │ └─Object/child2.1 Available: False NotAvailable 292y 1 │ 2 └─Object/child3 Available: False NotAvailable 292y 1 2 └─Object/child3.1 Available: False NotAvailable 292y 1 2

sbueringer · 2025-02-03T10:29:50Z

/assign @fabriziopandini

k8s-ci-robot · 2025-02-05T08:21:34Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from fabriziopandini. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

fabriziopandini

Made a first quick pass, really appreciated the test with the example of the output before the change

cmd/clusterctl/cmd/describe_cluster.go

fabriziopandini · 2025-02-05T09:48:44Z

cmd/clusterctl/cmd/describe_cluster_test.go

+			}(),
+			expectPrefix: []string{
+				"Object/root",
+				"│                                                              2",


I'm a little bit confused by seeing only the second line of the message (2) and not the first line (1).
Also, why are we not seeing condition status, reason, age?
Is this correct?

It is just like the other/v1beta1 tests, we just check the prefix. We cannot ensure the age, thus we should only verify the prefix here.

fabriziopandini · 2025-02-05T09:53:36Z

cmd/clusterctl/cmd/describe_cluster_test.go

+	tests := []struct {
+		name         string
+		objectTree   *tree.ObjectTree
+		expectPrefix []string


Should this be

Suggested change

expectPrefix []string

expectOutput []string

or expectedTable given that it is more than the simple prefix

Just copied from the v1beta1 test above :) can change it, but I should change it in both cases then

fabriziopandini · 2025-02-05T09:54:38Z

cmd/clusterctl/cmd/describe_cluster_test.go

@@ -294,6 +294,154 @@ func Test_TreePrefix(t *testing.T) {
 	}
 }

+func Test_V1Beta2TreePrefix(t *testing.T) {


might be

Suggested change

func Test_V1Beta2TreePrefix(t *testing.T) {

func Test_addObjectRowV1Beta2(t *testing.T) {

(this is what we are testing)

Same again as for the above comments, I copied it from the v1beta1 test

fabriziopandini · 2025-02-05T10:00:14Z

cmd/clusterctl/cmd/describe_cluster.go

+		}
+		// We just want to indent the multiline message for the Ready condition.
+		// All ├─ should be replaced by |, so all the existing hierarchic dependencies are carried on
+		multilinePrefix = strings.ReplaceAll(prefix+childrenPipe+filler, firstElemPrefix, pipe)


I'm a little bit confused by having a filler at the end of the prefix given that what comes next goes in a different column (and so it automatically goes in the right position, no need to indent)
Same below for the additional spaces

This is a copy from the code for the conditions

cluster-api/cmd/clusterctl/cmd/describe_cluster.go

Lines 438 to 439 in eac73ac

childPrefix := getChildPrefix(prefix+childrenPipe+filler, i, len(conditions))

c, status, age, reason, message := v1Beta2ConditionInfo(condition, positivePolarity)

This is necessary as the objects condition might have a multiline message and should be handled exactly as conditions via --show-conditions

Maybe it makes sense to reference addOtherConditionsV1Beta2() and getChildPrefix() in a comment

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 <[email protected]>

k8s-ci-robot added the area/clusterctl Issues or PRs related to clusterctl label Feb 1, 2025

k8s-ci-robot requested review from jackfrancis and JoelSpeed February 1, 2025 10:53

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 1, 2025

tobiasgiese commented Feb 1, 2025

View reviewed changes

tobiasgiese force-pushed the fix-multiline-parent-conditions-tests branch 4 times, most recently from 7f71cbc to 79b13f5 Compare February 1, 2025 11:17

tobiasgiese changed the title ~~🐛 Fix multiline Ready condition in clusterctl describe~~ 🐛 Fix multiline Ready condition in clusterctl describe for v1beta2 Feb 1, 2025

tobiasgiese force-pushed the fix-multiline-parent-conditions-tests branch 5 times, most recently from c8bfa74 to 8c38475 Compare February 2, 2025 10:52

tobiasgiese commented Feb 2, 2025

View reviewed changes

tobiasgiese force-pushed the fix-multiline-parent-conditions-tests branch 4 times, most recently from ac341c5 to 3aa0b3b Compare February 2, 2025 11:32

k8s-ci-robot assigned fabriziopandini Feb 3, 2025

tobiasgiese force-pushed the fix-multiline-parent-conditions-tests branch from 3aa0b3b to eac73ac Compare February 5, 2025 08:21

fabriziopandini reviewed Feb 5, 2025

View reviewed changes

tobiasgiese force-pushed the fix-multiline-parent-conditions-tests branch from eac73ac to 3c489c9 Compare February 5, 2025 10:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🐛 Fix multiline Ready condition in clusterctl describe for v1beta2 #11781

🐛 Fix multiline Ready condition in clusterctl describe for v1beta2 #11781

tobiasgiese commented Feb 1, 2025 •

edited

Loading

tobiasgiese Feb 1, 2025 •

edited

Loading

tobiasgiese Feb 1, 2025 •

edited

Loading

tobiasgiese Feb 2, 2025

sbueringer commented Feb 3, 2025

k8s-ci-robot commented Feb 5, 2025

fabriziopandini left a comment

fabriziopandini Feb 5, 2025

tobiasgiese Feb 5, 2025 •

edited

Loading

fabriziopandini Feb 5, 2025

tobiasgiese Feb 5, 2025

fabriziopandini Feb 5, 2025

tobiasgiese Feb 5, 2025

fabriziopandini Feb 5, 2025

tobiasgiese Feb 5, 2025

tobiasgiese Feb 5, 2025 •

edited

Loading

	func Test_V1Beta2TreePrefix(t *testing.T) {
	func Test_addObjectRowV1Beta2(t *testing.T) {

	childPrefix := getChildPrefix(prefix+childrenPipe+filler, i, len(conditions))
	c, status, age, reason, message := v1Beta2ConditionInfo(condition, positivePolarity)

🐛 Fix multiline Ready condition in clusterctl describe for v1beta2 #11781

Are you sure you want to change the base?

🐛 Fix multiline Ready condition in clusterctl describe for v1beta2 #11781

Conversation

tobiasgiese commented Feb 1, 2025 • edited Loading

tobiasgiese Feb 1, 2025 • edited Loading

Choose a reason for hiding this comment

tobiasgiese Feb 1, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbueringer commented Feb 3, 2025

k8s-ci-robot commented Feb 5, 2025

fabriziopandini left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tobiasgiese Feb 5, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tobiasgiese Feb 5, 2025 • edited Loading

Choose a reason for hiding this comment

tobiasgiese commented Feb 1, 2025 •

edited

Loading

tobiasgiese Feb 1, 2025 •

edited

Loading

tobiasgiese Feb 1, 2025 •

edited

Loading

tobiasgiese Feb 5, 2025 •

edited

Loading

tobiasgiese Feb 5, 2025 •

edited

Loading