Display desired status unconditionally#6377
Conversation
| expect(out).toContain('deployment_runtime_status: (none)') | ||
| expect(out).not.toContain('desired is') | ||
| expect(out).toContain('deployment_resources_status: Stopped (desired is undefined)') | ||
| expect(out).toContain('deployment_runtime_status: (none) (desired is null)') |
There was a problem hiding this comment.
Can you make it "desired is (none)" for both?
mythical-fred
left a comment
There was a problem hiding this comment.
Removing the conditional is fine if the team wants the desired status always visible (good for debugging state transitions), but the new spec test pins behaviour I'd flag as a UX regression: when deployment_runtime_desired_status is null/missing in the API payload, the UI now renders the literal string (desired is null) or (desired is undefined). That's worse than the old "just hide it" path the previous version took, and it leaks JS coercion into user-visible text.
Two ways to keep the unconditional intent without the ugly string:
- Show
(none)for nullish desired, matching howformatScalaralready handles missing values:`${base} (desired is ${formatScalar(desired)})`. - Or keep the always-append behaviour for present values but suppress only the nullish branch (a one-line guard), which preserves the "same as current" rendering that this PR is correctly trying to add back.
Either keeps the new "always shows desired even when it matches current" semantics that the PR title is going for, without paying the cost of String(undefined) ending up on screen. Not a blocker — your call which framing fits the UX you want — but the current spec test is effectively asserting a bug.
Also tiny: the new comment // Render a status, always appending the desired value:"Running (desired is Stopped)". is missing a space after the colon.
332041a to
14f6710
Compare
Signed-off-by: Karakatiza666 <bulakh.96@gmail.com>
14f6710 to
7b59137
Compare
|
Changed to ", desired is X" |
| return base | ||
| } | ||
| return `${base} (desired is ${String(desired)})` | ||
| return `${base}, desired is ${desired ?? `(none)`}` |
There was a problem hiding this comment.
Nit: this file already defines const NONE = '(none)' just above (line 229) and uses it in formatScalar/formatJson. Consider reusing it here for consistency, e.g. `${base}, desired is ${desired ?? NONE}`. Also the comment on line 235 still shows the old parenthesized example ("Running (desired is Stopped)") — worth updating to the new comma form ("Running, desired is Stopped").
Tested: manually, updated the unit test