fix flash log streaming and add colored output#114
Conversation
Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>
fixes centos-automotive-suite#104 Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>
bennyz
changed the title
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughAdds colorized terminal output and flash-aware UX to the CLI (cmd/caib), vendores a terminal color library and colorable writers, adjusts server log streaming and Jumpstarter mapping, and removes a large set of vendored OpenShift config v1 API type files and generated manifests. Changes
Sequence DiagramsequenceDiagram
participant User
participant CLI as cmd/caib<br/>(main.go)
participant Server as buildapi<br/>(internal/buildapi)
participant Term as Terminal
User->>CLI: start build (with/without flash)
CLI->>Server: request build start / status
Server->>Server: run pipeline, stream logs
Server->>Term: stream logs (plain or colored)
Note over CLI,Term: CLI checks supportsColorOutput()
Server->>CLI: build completes/fails + Jumpstarter info
alt build success
CLI->>Term: displayBuildResults() (colored labels/values)
CLI->>Term: displayFlashInstructions() (if flash)
else build failure
CLI->>Term: handleFlashError() (colored error + recovery options)
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmd/caib/main.go (1)
2564-2574:⚠️ Potential issue | 🟠 MajorFlash log client has a hard 10-minute timeout, unlike the build log client.
The build log path (Line 1586–1591) explicitly avoids setting
Timeouton the HTTP client because log streams can run for the entire build duration, relying on the context timeout instead. The flash log client here setsTimeout: 10 * time.Minute, but the flash lease duration defaults to 3 hours and is user-configurable. If a flash operation streams logs for longer than 10 minutes, thishttp.Clienttimeout will kill the connection, potentially losing log output.Apply the same pattern as the build path — omit the hard
Timeoutand lettimeoutCtxgovern cancellation:Proposed fix
logClient := &http.Client{ - Timeout: 10 * time.Minute, Transport: flashLogTransport, }
🧹 Nitpick comments (3)
internal/buildapi/server.go (1)
820-838: Keepalive message is well-designed for the flash log streaming fix.The unconditional 2-second sleep prevents tight-looping, and the
[Waiting for remaining pipeline tasks...]keepalive whenallPodsComplete && hadStreamserves dual purpose: preventing proxy idle-timeouts and keeping the client's line-based scanner alive while waiting for the flash pod to appear.One minor consideration: this keepalive message will be emitted every 2 seconds and will appear in the client's log output. If the wait is long (e.g., flash pod scheduling takes minutes), the client will see many repeated lines. Consider whether this is acceptable UX or if a longer interval / deduplication would be preferable.
cmd/caib/main.go (2)
123-144:supportsColorOutputmay enable color even whenTERM=dumb.When
TERM=dumb(common in CI, Emacs shells, etc.),color.NoColoristrue, so!color.NoColorisfalse. However, the||fallback toisSupportedShellmeans color is still enabled ifSHELLcontainsbash/zsh/fish— overriding the explicitTERM=dumbsignal. This can produce garbled output in those environments.Consider adding an early-out for
TERM=dumb:Proposed fix
func supportsColorOutput() bool { if os.Getenv("NO_COLOR") != "" { return false } + if os.Getenv("TERM") == "dumb" { + return false + } + if !term.IsTerminal(int(os.Stdout.Fd())) { return false }
2488-2499:parseLeaseDurationsilently falls back on parse errors.
fmt.Sscanferrors are discarded, so malformed input like"abc:def:ghi"would silently result in 0s duration (not the 1-hour default, which only applies when the part count ≠ 3). This is acceptable since the format is validated elsewhere (the CLI flag default is well-formed), but consider logging a warning for unexpected formats if this function may be called with arbitrary input in the future.
Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>
Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmd/caib/main.go (1)
2492-2502:⚠️ Potential issue | 🟡 Minor
parseLeaseDurationsilently swallows parse errors.
fmt.Sscanferrors are discarded, so malformed input like"abc:def:ghi"returns0sinstead of the 1-hour default (which is only triggered when the colon-split doesn't produce 3 parts). Consider validating the Sscanf results or returning an error for clearly invalid values.Suggested fix
func parseLeaseDuration(duration string) time.Duration { parts := strings.Split(duration, ":") if len(parts) != 3 { return time.Hour // Default 1 hour } - var hours, mins, secs int - _, _ = fmt.Sscanf(parts[0], "%d", &hours) - _, _ = fmt.Sscanf(parts[1], "%d", &mins) - _, _ = fmt.Sscanf(parts[2], "%d", &secs) + hours, err1 := strconv.Atoi(parts[0]) + mins, err2 := strconv.Atoi(parts[1]) + secs, err3 := strconv.Atoi(parts[2]) + if err1 != nil || err2 != nil || err3 != nil { + return time.Hour // Default 1 hour for invalid input + } return time.Duration(hours)*time.Hour + time.Duration(mins)*time.Minute + time.Duration(secs)*time.Second }
🤖 Fix all issues with AI agents
In `@cmd/caib/main.go`:
- Around line 1670-1697: The code currently sets isFlashFailure = true whenever
flashAfterBuild is true, which misclassifies build-time failures as flash
failures; update the failure-classification logic in the block that inspects
st.Phase/lastPhase/flashAfterBuild so that isFlashFailure is only true when the
build actually reached a flash-related phase or the status message explicitly
indicates a flash error (e.g., when lastPhase == "Flashing" or
strings.Contains(strings.ToLower(st.Message), errPrefixFlash) is true), and
remove the unconditional flashAfterBuild branch (or make it conditional on
lastPhase indicating flashing) before calling handleFlashError(err, st) vs
handleError(err). Use the existing symbols st.Phase, st.Message, lastPhase,
flashAfterBuild, errPrefixFlash, handleFlashError and handleError to locate and
change the logic.
🧹 Nitpick comments (1)
cmd/caib/main.go (1)
123-147:supportsColorOutput()can disagree withfatih/color's internalNoColorcheck.The custom detection on lines 128–146 uses
term.IsTerminalwhilefatih/coloruses its ownisatty.IsTerminal. When these disagree,supportsColorOutput()may returntruebutcolor.New(...).SprintFunc()still produces plain text (becausecolor.NoColoristrue). The opposite is also possible due to the extraisSupportedShell/hasColorTermfallbacks overridingcolor.NoColor.In practice the output is always correct (fatih/color governs actual escape codes), but the branching is misleading. Consider aligning with the library by using
color.NoColordirectly, or explicitly settingcolor.NoColor = falsewhen you want to override it.Suggested simplification
func supportsColorOutput() bool { if os.Getenv("NO_COLOR") != "" { return false } - if !term.IsTerminal(int(os.Stdout.Fd())) { return false } - termType := os.Getenv("TERM") if termType == "dumb" { return false } - - shell := os.Getenv("SHELL") - - isSupportedShell := strings.Contains(shell, "bash") || - strings.Contains(shell, "fish") || - strings.Contains(shell, "zsh") - - hasColorTerm := termType != "" && - !strings.Contains(termType, "mono") - - return !color.NoColor || isSupportedShell || hasColorTerm + return !color.NoColor }
Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>
2 participants
Summary by CodeRabbit
New Features
Chores