🐛 Fix multiline Ready condition in clusterctl describe for v1beta2 #11781
Conversation
k8s-ci-robot
added
cncf-cla: yes
| "Object/root", | ||
| "│ 2", | ||
| "├─Object/child1", | ||
| "│ 2", | ||
| "└─Object/child2", |
There was a problem hiding this comment.
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
| "Object/root", | ||
| "├─Object/child1", | ||
| "└─Object/child2", | ||
| " │ 2", | ||
| " └─Object/child2.1", |
There was a problem hiding this comment.
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
force-pushed
the
fix-multiline-parent-conditions-tests
branch
4 times, most recently
from
7f71cbc to
79b13f5
Compare
tobiasgiese
changed the title
tobiasgiese
force-pushed
the
fix-multiline-parent-conditions-tests
branch
5 times, most recently
from
c8bfa74 to
8c38475
Compare
| "Object/root", | ||
| "├─Object/child1", | ||
| "├─Object/child2", | ||
| "│ │ 2", | ||
| "│ └─Object/child2.1", | ||
| "│ 2", | ||
| "└─Object/child3", | ||
| " │ 2", | ||
| " └─Object/child3.1", | ||
| " 2", |
There was a problem hiding this comment.
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
ac341c5 to
3aa0b3b
Compare
|
/assign @fabriziopandini |
tobiasgiese
force-pushed
the
fix-multiline-parent-conditions-tests
branch
from
3aa0b3b to
eac73ac
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
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]>
tobiasgiese
force-pushed
the
fix-multiline-parent-conditions-tests
branch
from
eac73ac to
3c489c9
Compare
There was a problem hiding this comment.
@tobiasgiese sorry for the delay in the review, there is one last comment to be addressed, otherwise lgtm
| func getOrderedTreeObjects(objectTree *tree.ObjectTree) []ctrlclient.Object { | ||
| rootObjs := objectTree.GetObjectsByParent(objectTree.GetRoot().GetUID()) | ||
| rootObjs = orderChildrenObjects(rootObjs) | ||
| objs := []ctrlclient.Object{objectTree.GetRoot()} | ||
| for _, obj := range rootObjs { | ||
| childrenObjs := objectTree.GetObjectsByParent(obj.GetUID()) | ||
| childrenObjs = orderChildrenObjects(childrenObjs) | ||
| objs = append(objs, obj) | ||
| objs = append(objs, childrenObjs...) | ||
| } | ||
| return objs | ||
| } |
There was a problem hiding this comment.
This doesn't work when the last node is nested two (or more) levels, e.g.
NAME REPLICAS AVAILABLE READY UP TO DATE STATUS REASON SINCE MESSAGE
Cluster/classy-base-8 2/4 0 0 2 Available: False NotAvailable ...
└─Workers
└─MachineDeployment/classy-base-8-md1-4xc49 1/1 0 0 1 Available: False NotAvailable 55m 0 available replicas, at least 1 required (spec.strategy.rollout.maxUnavailable is 0, spec.replicas
is 1)
└─Machine/classy-base-8-md1-4xc49-w5696-lcdc6 1 0 0 1
...
(the line right after MachineDeployment doesn't have the right prefix)
I would also suggest that we compute the last node once up in the call stack (instead of recomputing it many time) and then pass it around
There was a problem hiding this comment.
If nested multiple times it's even worse - let me fix that
Object/root Available: True Available 292y
└─Object/child1 Available: True Available 292y
└─Object/child2 Available: False NotAvailable 292y 1
2
└─Object/child3 Available: False NotAvailable 292y 1
│ 2
└─Object/child4 Available: False NotAvailable 292y 1
2
There was a problem hiding this comment.
I would also suggest that we compute the last node once up in the call stack (instead of recomputing it many time) and then pass it around
I don't know if this is possible as addObjectRowV1Beta2() is a recursion. If we only want to compute it once we can add a new field e.g. orderedItems to the ObjectTree.
Edit: we can just put the last object as an annotation to the root object on the first call, sounds reasonable to me.
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