feat(bundler): structured deploy.sh output with color and step headers#1393
feat(bundler): structured deploy.sh output with color and step headers#1393mohityadav8 wants to merge 5 commits into
Conversation
- Add NO_COLOR-aware color helpers (_ok, _fail, _warn_line) - Add _step_header/ok/fail/retry for per-component section framing - Show [N/M] progress counter and manual copy-paste command per step - Color-code pre-flight pass/fail and final deployment summary Closes NVIDIA#1362
|
Welcome to AICR, @mohityadav8! Thanks for your first pull request. Before review, please ensure:
A maintainer will review this soon. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Enterprise Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe deploy script template ( Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/bundler/deployer/helm/templates/deploy.sh.tmpl`:
- Around line 70-73: In the _step_header function, the second printf statement
prints the directory path without quotes in the manual execution instruction.
Add quotes around the path parameter in the printf format string so that when
the bundle path is substituted, it will be properly escaped for shell copy-paste
commands. This ensures paths containing spaces or special characters will not
break the displayed command.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: f957c905-4a38-4094-9c8a-ca0384a6bcf5
📒 Files selected for processing (7)
pkg/bundler/deployer/helm/templates/deploy.sh.tmplpkg/bundler/deployer/helm/testdata/kai_scheduler_present/deploy.shpkg/bundler/deployer/helm/testdata/manifest_only/deploy.shpkg/bundler/deployer/helm/testdata/mixed_gpu_operator/deploy.shpkg/bundler/deployer/helm/testdata/mixed_with_pre/deploy.shpkg/bundler/deployer/helm/testdata/nodewright_present/deploy.shpkg/bundler/deployer/helm/testdata/upstream_helm_only/deploy.sh
Signed-off-by: Mohit Yadav <ymohit799057@gmail.com>
mchmarny
left a comment
mchmarny
left a comment
There was a problem hiding this comment.
Welcome to AICR, @mohityadav8 — and thanks for picking this up! This is a genuinely nice quality-of-life improvement to deploy.sh: the NO_COLOR + non-TTY detection is done correctly, the helpers are clean, and the per-step [N/M] framing makes a long install far easier to follow. Regenerating all 6 testdata goldens alongside the template is exactly the right discipline, too.
One thing blocks merge: the goldens are out of sync with the template. The _step_header quoting fix (printf -v manual_dir '%q') landed in deploy.sh.tmpl but the committed testdata/*/deploy.sh files still have the pre-fix version, so the byte-comparing TestBundleGolden_* tests will fail. A quick go test ./pkg/bundler/deployer/helm/... -run TestBundleGolden -update (then commit) fixes all six at once — details inline. The other two are small nits (loop indentation, and the Manual: hint not reflecting the env the real install uses).
Heads up: the PR is behind main, so rebasing on origin/main before re-pushing will also let the real CI gates run. Nice first contribution — this is close.
|
|
||
| function _step_header() { | ||
| printf '\n%s┌─ [%s/%s] %s → %s%s\n' "${_B}" "$1" "$2" "$3" "$4" "${_X}" | ||
| printf '%s│ Manual: cd %s && bash install.sh%s\n' "${_D}" "$5" "${_X}" |
There was a problem hiding this comment.
This golden is out of sync with the template. deploy.sh.tmpl's _step_header now shell-quotes the path (local manual_dir; printf -v manual_dir '%q' "$5" → prints ${manual_dir}), but this golden still prints "$5" directly — same in all 6 testdata/*/deploy.sh.
assertBundleGolden byte-compares every rendered file including deploy.sh, so TestBundleGolden_* will fail on the mismatch. Regenerate and commit:
go test ./pkg/bundler/deployer/helm/... -run TestBundleGolden -update
| # diagnostics and clean up stale Helm hook Jobs between attempts. | ||
| attempt=0 | ||
| while true; do | ||
| attempt=0 |
There was a problem hiding this comment.
nit: attempt=0 / while true; do / done got dedented to column 0, but they're inside the for dir … do loop (body still indented 8 spaces). It's harmless to bash, but in this block — retry + hook cleanup + kai diagnostics — the while now reads as if it sits outside the for-loop. Re-indent to match the enclosing loop.
| local manual_dir | ||
| printf -v manual_dir '%q' "$5" | ||
| printf '\n%s┌─ [%s/%s] %s → %s%s\n' "${_B}" "$1" "$2" "$3" "$4" "${_X}" | ||
| printf '%s│ Manual: cd %s && bash install.sh%s\n' "${_D}" "${manual_dir}" "${_X}" |
There was a problem hiding this comment.
nit: the actual install runs install.sh with exported COMPONENT_WAIT_ARGS, DRY_RUN_FLAG, KUBECONFIG_FLAG, and HELM_DEBUG_FLAG, but this Manual: hint reproduces none of them — a copy-paste won't match the wait/timeout/kubeconfig/dry-run behavior of the real run. Worth a short caveat that it's an approximation, or noting the relevant env.
2 participants
Closes #1362