Print waiting message when --wait is used for install/upgrade#32019
Print waiting message when --wait is used for install/upgrade#32019
Conversation
pull-request-size
bot
added
the
size/S
Closes helm#12710 Signed-off-by: John Lin <johnlinp@gmail.com>
johnlinp
force-pushed
the
add-wait-message
branch
from
b8d1bc0 to
1bbfadd
Compare
There was a problem hiding this comment.
Pull request overview
This PR adds a user-facing progress line when helm install --wait / helm upgrade --wait enters the blocking resource-wait phase, addressing the “looks stalled” UX reported in #12710.
Changes:
- Print
Waiting for resources to become ready (timeout: …)before the wait begins forupgrade(table output). - Add a
printWaitMessagehelper and call it from the install flow. - Update golden test outputs for install/upgrade wait scenarios.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| pkg/cmd/upgrade.go | Prints a wait/progress line for table output before running the upgrade when --wait is in effect. |
| pkg/cmd/install.go | Adds printWaitMessage helper and prints the wait/progress line in the shared install execution path. |
| pkg/cmd/testdata/output/upgrade-with-wait.txt | Updates expected output to include the new wait/progress line. |
| pkg/cmd/testdata/output/upgrade-with-wait-for-jobs.txt | Updates expected output to include the new wait/progress line. |
| pkg/cmd/testdata/output/install-with-wait.txt | Updates expected output to include the new wait/progress line. |
| pkg/cmd/testdata/output/install-with-wait-for-jobs.txt | Updates expected output to include the new wait/progress line. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| printWaitMessage(out, client.WaitStrategy, client.Timeout) | ||
|
|
There was a problem hiding this comment.
printWaitMessage writes directly to out unconditionally (except for hookOnly). Since install supports -o json|yaml, this will prepend a plain-text line and corrupt machine-readable output when users run e.g. helm install --wait -o json (and also affects upgrade --install and template, which both reuse runInstall). Consider gating this message on outfmt == output.Table (like upgrade.go already does) and/or only enabling it for the actual install/upgrade commands (not template), and add a unit test that asserts JSON output remains valid with --wait enabled.
| printWaitMessage(out, client.WaitStrategy, client.Timeout) | |
| // Do not print wait progress here: runInstall is shared by commands that | |
| // may produce machine-readable output, and writing plain text to `out` | |
| // would corrupt JSON/YAML responses. |
utafrali
left a comment
utafrali
left a comment
There was a problem hiding this comment.
Clean, focused change that solves a real UX pain point. The logic is correct, tests are included, and the output-format guard in upgrade.go is consistent with surrounding code. Two minor style points around symmetry and function placement, neither of which blocks merging.
| cancel() | ||
| }() | ||
|
|
||
| printWaitMessage(out, client.WaitStrategy, client.Timeout) |
There was a problem hiding this comment.
printWaitMessage is called here without an output-format guard, while upgrade.go wraps the same call in if outfmt == output.Table. Since install is always table-only this makes no practical difference, but the inconsistency could confuse future maintainers. Consider adding the same guard here for symmetry, or add a comment explaining why the guard is intentionally omitted.
| return fmt.Errorf("%s charts are not installable", meta["Type"]) | ||
| } | ||
|
|
||
| func printWaitMessage(out io.Writer, strategy kube.WaitStrategy, timeout time.Duration) { |
There was a problem hiding this comment.
printWaitMessage is defined in install.go but is also called from upgrade.go. In Go this works fine within the same package, but a helper shared by two sibling command files is more naturally home in a small shared file (e.g. pkg/cmd/common.go or similar) so readers of upgrade.go can find it without hunting. Not a blocker, just a discoverability concern.
Closes #12710
Signed-off-by: John Lin johnlinp@gmail.com
What this PR does / why we need it:
This PR adds a user-facing progress message for
helm install --waitandhelm upgrade --wait.Today, Helm can appear to stall after printing earlier output such as dependency update messages, because there is no explicit indication that it has entered the resource-wait phase. This change prints
Waiting for resources to become ready...before the blocking wait begins, so users have clearer feedback about what Helm is doing.Special notes for your reviewer:
This intentionally changes the default table output for
installandupgradewhen--waitis used.A Helm maintainer previously noted in the issue discussion that the main concern was changing command output that some users may consume from scripts, and that a Helm v4-targeted PR would be welcome:
#12710 (comment)
If that is considered too compatibility-sensitive for Helm v4.x, I'm happy to rework this behind a gate or defer it to the next major release.
If applicable:
docs neededlabel should be applied if so)