OCPBUGS-60768: properly indent multiline node messages in upgrade status output - #2341
OCPBUGS-60768: properly indent multiline node messages in upgrade status output#2341savio87 wants to merge 1 commit into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
jira/severity-moderate
|
@savio87: This pull request references Jira Issue OCPBUGS-60768, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
Walkthrough
ChangesUpgrade status output
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested labels: 🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: savio87 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
needs-ok-to-test
|
Hi @savio87. Thanks for your PR. I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with Regular contributors should join the org to skip this step. Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
Applies the same multiline indentation pattern already used for cluster operator messages in controlplane.go. Could a maintainer update the target version on OCPBUGS-60768 and run /ok-to-test? Thanks! |
![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/cli/admin/upgrade/status/workerpool.go`:
- Around line 560-567: Update WriteNodes to return an error and check/propagate
failures from fmt.Fprintf, tabwriter.Write, and tabwriter.Flush, including
errors while writing node messages. Update both WriteNodes call sites in
status.go to handle and return the propagated error through the command path.
🪄 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: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 6da72201-20f4-4c08-bcbd-9909f666a48b
📒 Files selected for processing (3)
pkg/cli/admin/upgrade/status/examples/ocpbugs-60768.detailed-outputpkg/cli/admin/upgrade/status/examples/ocpbugs-60768.outputpkg/cli/admin/upgrade/status/workerpool.go
| lines := strings.Split(node.Message, "\n") | ||
| for i, line := range lines { | ||
| if i == 0 { | ||
| _, _ = tabw.Write([]byte(line + "\n")) | ||
| } else { | ||
| _, _ = tabw.Write([]byte("\t\t\t\t\t" + line + "\n")) | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 3 '\bWriteNodes\s*\(' pkg/cli/admin/upgrade/statusRepository: openshift/oc
Length of output: 1828
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== workerpool WriteNodes area =="
sed -n '488,572p' pkg/cli/admin/upgrade/status/workerpool.go
echo
echo "== status WriteNodes call sites area =="
sed -n '346,372p' pkg/cli/admin/upgrade/status/status.go
echo
echo "== poolDisplayData WriteNodes definition/uses in workerpool =="
rg -n -C 2 'func \(pool \*poolDisplayData\) WriteNodes|WriteNodes\s*\(' pkg/cli/admin/upgrade/status/workerpool.go pkg/cli/admin/upgrade/status/status.goRepository: openshift/oc
Length of output: 5320
Return WriteNodes output errors to callers.
WriteNodes ignores errors from fmt.Fprintf, tabwriter.Write, and tabwriter.Flush, so an io.Writer failure can leave the status output incomplete and be hidden at the two call sites in pkg/cli/admin/upgrade/status/status.go. Make WriteNodes return an error and propagate the failure up the command path.
🤖 Prompt for 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.
In `@pkg/cli/admin/upgrade/status/workerpool.go` around lines 560 - 567, Update
WriteNodes to return an error and check/propagate failures from fmt.Fprintf,
tabwriter.Write, and tabwriter.Flush, including errors while writing node
messages. Update both WriteNodes call sites in status.go to handle and return
the propagated error through the command path.
Source: Path instructions
2 participants
Summary
When node status messages contain newlines (e.g., verbose podman pull errors), the continuation lines break out of the MESSAGE column and misalign subsequent rows in
oc adm upgrade statusoutput.This applies the same multiline handling pattern already used for cluster operator messages in
controlplane.go— splitting on newlines and indenting continuation lines with tabs to align under the MESSAGE column.Bug: https://redhat.atlassian.net/browse/OCPBUGS-60768
Changes
workerpool.go: Splitnode.Messageon\nand indent continuation lines with tabs inWriteNodes()Summary by CodeRabbit