fix: do not show flashing instructions when build failed#294
Conversation
|
Warning Review limit reached
More reviews will be available in 19 minutes and 44 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, 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 include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughAdds a helper to decide when build artifact URLs should be considered produced (based on phase and task-run names), applies that gating in ChangesBuild Artifact Availability
Sequence Diagram(s)sequenceDiagram
participant Client
participant APIServer
participant buildProducedArtifacts
Client->>APIServer: listBuilds/getBuild request
APIServer->>buildProducedArtifacts: evaluate build.Status.Phase and task-run names
buildProducedArtifacts-->>APIServer: true/false
APIServer-->>Client: response (include artifact URLs only if true)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
| // push completed (e.g. during flash). | ||
| func buildProducedArtifacts(build *automotivev1alpha1.ImageBuild) bool { | ||
| switch build.Status.Phase { | ||
| case phaseCompleted, phaseFlashing, phasePushing: |
There was a problem hiding this comment.
the function returns true for phasePushing but if a build is currently in the pushing phase(the push hasn't completed yet) so the artifact url may not point to real object.
Should it only return true once pushing is done (i.e., phaseFlashing or phaseCompleted)?
bennyz
force-pushed
the
fix-flash-instruction
branch
from
96a2640 to
f4fca3d
Compare
maboras-rh
left a comment
maboras-rh
left a comment
There was a problem hiding this comment.
lgtm, overall is clean, well-scoped fix
left one minor comment related to phasePushing
![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 `@cmd/caib/buildcmd/logs.go`:
- Line 169: Add unit tests that exercise the flash-specific branch in
waitForBuildCompletion by constructing build status states where hasImage (set
DiskImage or ContainerImage on the status) and isFlashFailure (simulate a
phaseFailed flash error) are true; invoke waitForBuildCompletion (or the minimal
exported function that calls it) and assert that displayBuildResults and
handleFlashError behaviors occur (capture/log output or use test doubles/mocks
for displayBuildResults and handleFlashError) and verify the flash error
message/output is produced; focus tests on the phaseFailed scenario to ensure
coverage for the hasImage && isFlashFailure 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 05c187e2-4e03-48fb-ba22-2a3c96cbb3be
📒 Files selected for processing (3)
cmd/caib/buildcmd/logs.gointernal/buildapi/server.gointernal/buildapi/server_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/buildapi/server_test.go
- internal/buildapi/server.go
| // Only show push/flash results when an image was actually produced. | ||
| hasImage := st.DiskImage != "" || st.ContainerImage != "" | ||
| if hasImage && (isFlashFailure || *h.opts.FlashAfterBuild) { | ||
| if hasImage && isFlashFailure { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Search for tests covering the waitForBuildCompletion failure path
# Find test files for buildcmd package
fd -e go -t f 'test' cmd/caib/buildcmd
# Search for tests that cover the phaseFailed or failure scenarios
rg -n -A5 -B5 'phaseFailed|TestWaitForBuildCompletion.*[Ff]ail' --type=go --glob 'cmd/caib/buildcmd/*test*.go'
# Look for tests covering hasImage and isFlashFailure conditions
rg -n 'hasImage|isFlashFailure' --type=go --glob 'cmd/caib/buildcmd/*test*.go'Repository: centos-automotive-suite/automotive-dev-operator
Length of output: 205
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== waitForBuildCompletion in logs.go =="
rg -n "func waitForBuildCompletion" -S cmd/caib/buildcmd/logs.go
rg -n "hasImage|isFlashFailure|phaseFailed|FlashAfterBuild|flash" -S cmd/caib/buildcmd/logs.go
echo
echo "== Test files in cmd/caib/buildcmd =="
fd -e go -t f 'test' cmd/caib/buildcmd | sort
echo
echo "== Search for relevant test coverage =="
# Search for waitForBuildCompletion / phaseFailed / flash-related messaging in tests
rg -n -S "waitForBuildCompletion|phaseFailed|FlashAfterBuild|isFlashFailure|hasImage|flash" --type=go cmd/caib/buildcmd/*test*.go || true
echo
echo "== Show any tests mentioning 'flash' in buildcmd tests =="
rg -n -S "flash" --type=go cmd/caib/buildcmd/*test*.go || true
echo
echo "== Show any tests that mention 'DiskImage' or 'ContainerImage' =="
rg -n -S "DiskImage|ContainerImage" --type=go cmd/caib/buildcmd/*test*.go || trueRepository: centos-automotive-suite/automotive-dev-operator
Length of output: 131
🏁 Script executed:
#!/bin/bash
set -uo pipefail
echo "== Locate relevant code in logs.go =="
# Show the relevant area around where phaseFailed handling likely lives
rg -n "phaseFailed|FlashAfterBuild|flash" -S cmd/caib/buildcmd/logs.go || true
# Try to find the function by looking for nearby identifiers (safer than exact func name)
rg -n "waitFor|Completion|BuildCompletion|phaseFailed" -S cmd/caib/buildcmd/logs.go || true
# Print the exact section around the line numbers mentioned in the review comment
# (169 was the changed line; include a wider window)
sed -n '130,210p' cmd/caib/buildcmd/logs.go
echo
echo "== Test files in cmd/caib/buildcmd =="
fd -e go -t f 'test' cmd/caib/buildcmd | sort
echo
echo "== Search for flash/phaseFailed coverage in tests =="
rg -n -S "phaseFailed|FlashAfterBuild|isFlashFailure|hasImage|flash" --type=go cmd/caib/buildcmd/*test*.go || true
echo
echo "== Search for any tests calling the logs/wait logic =="
rg -n -S "waitFor|logs.go|buildcmd/logs" --type=go cmd/caib/buildcmd/*test*.go || trueRepository: centos-automotive-suite/automotive-dev-operator
Length of output: 4353
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Inspect build_disk_test.go for phaseFailed/flash assertions =="
sed -n '1,220p' cmd/caib/buildcmd/build_disk_test.go
echo
echo "== Search within tests for flash-related output/assertions =="
rg -n -S "phaseFailed|flash was not executed|Warning: --flash|Build and flash completed successfully|lease has been acquired|displayBuildResults|handleFlashError|displayFlashInstructions|JumpstarterTargetMappings|errPrefixFlash|failed" cmd/caib/buildcmd/*test*.go
echo
echo "== Search within tests for DiskImage/ContainerImage population on failure stubs =="
rg -n -S "DiskImage|ContainerImage|Message:|Phase:|ImageBuildPhase|phaseFlashing|phaseFailed|errPrefixFlash" cmd/caib/buildcmd/*test*.goRepository: centos-automotive-suite/automotive-dev-operator
Length of output: 6974
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== File sizes =="
wc -l cmd/caib/buildcmd/logs.go cmd/caib/buildcmd/build_disk_test.go cmd/caib/buildcmd/build_define_test.go cmd/caib/buildcmd/build_creds_test.go
echo
echo "== Remainder of build_disk_test.go =="
sed -n '220,500p' cmd/caib/buildcmd/build_disk_test.go
echo
echo "== Search all buildcmd tests for flash/error handling assertions =="
rg -n -S "handleFlashError|displayFlashInstructions|errPrefixFlash|flashWasExecuted|flash failure|Build and flash|Warning: --flash|To release the lease|Check OperatorConfig|phaseFlashing|phaseFailed" cmd/caib/buildcmd/*test*.go || true
echo
echo "== Search all buildcmd tests for st.Phase == phaseFailed or errPrefixFlash usage =="
rg -n -S "st\.Phase\s*==\s*phaseFailed|errPrefixFlash|phaseFlashing|flashAfterBuild" cmd/caib/buildcmd/*test*.go || true
echo
echo "== Search for any mock build API returning phases/images =="
rg -n -S "buildapiclient|Stream|Get.*Build|Phase|DiskImage|ContainerImage|phasePending|phaseCancelled" cmd/caib/buildcmd/*test*.go || true
echo
echo "== Quick peek: build_define_test.go =="
sed -n '1,240p' cmd/caib/buildcmd/build_define_test.go
echo
echo "== Quick peek: build_creds_test.go =="
sed -n '1,260p' cmd/caib/buildcmd/build_creds_test.goRepository: centos-automotive-suite/automotive-dev-operator
Length of output: 4990
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== cat -n build_disk_test.go =="
cat -n cmd/caib/buildcmd/build_disk_test.go
echo
echo "== rg for phaseFailed / handleFlashError / displayBuildResults in tests =="
rg -n -S "phaseFailed|handleFlashError|displayBuildResults|displayFlashInstructions|errPrefixFlash|FlashAfterBuild" cmd/caib/buildcmd/*test*.go || true
echo
echo "== rg for hasImage / isFlashFailure in tests =="
rg -n -S "hasImage|isFlashFailure" cmd/caib/buildcmd/*test*.go || trueRepository: centos-automotive-suite/automotive-dev-operator
Length of output: 8112
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Repo-wide searches in *_test.go for flash failure path identifiers =="
rg -n -S "waitForBuildCompletion" --type=go --glob '**/*_test.go' . || true
rg -n -S "handleFlashError|displayBuildResults|displayFlashInstructions|errPrefixFlash|phaseFlashing|isFlashFailure|hasImage && isFlashFailure|hasImage.*isFlashFailure" --type=go --glob '**/*_test.go' . || trueRepository: centos-automotive-suite/automotive-dev-operator
Length of output: 254
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Search for CLI flash error handlers in tests =="
rg -n -S "handleFlashError|displayBuildResults|displayFlashInstructions|errPrefixFlash" --type=go --glob '**/*_test.go' . || true
echo
echo "== Search for the specific tightened condition components in tests =="
rg -n -S "hasImage\s*&&\s*isFlashFailure|isFlashFailure|errPrefixFlash|phaseFailed.*(flash|Flash)|phaseFlashing" --type=go --glob '**/*_test.go' . || true
echo
echo "== Search for waitForBuildCompletion usage in tests =="
rg -n -S "waitForBuildCompletion" --type=go --glob '**/*_test.go' . || true
echo
echo "== Any tests under cmd/caib/buildcmd besides *_test.go already found? =="
fd -e go -t f '*_test.go' cmd/caib/buildcmd | sortRepository: centos-automotive-suite/automotive-dev-operator
Length of output: 864
Add tests covering flash-specific phaseFailed handling (hasImage && isFlashFailure)
cmd/caib/buildcmd/logs.go gates the flash-error path in waitForBuildCompletion with hasImage := st.DiskImage != "" || st.ContainerImage != "" and isFlashFailure := ..., then calls displayBuildResults + handleFlashError only when hasImage && isFlashFailure. Current tests under cmd/caib/buildcmd/*_test.go don’t exercise the phaseFailed branch or assert any flash error/results output (no coverage of waitForBuildCompletion, handleFlashError, or displayBuildResults).
🤖 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 `@cmd/caib/buildcmd/logs.go` at line 169, Add unit tests that exercise the
flash-specific branch in waitForBuildCompletion by constructing build status
states where hasImage (set DiskImage or ContainerImage on the status) and
isFlashFailure (simulate a phaseFailed flash error) are true; invoke
waitForBuildCompletion (or the minimal exported function that calls it) and
assert that displayBuildResults and handleFlashError behaviors occur
(capture/log output or use test doubles/mocks for displayBuildResults and
handleFlashError) and verify the flash error message/output is produced; focus
tests on the phaseFailed scenario to ensure coverage for the hasImage &&
isFlashFailure path.
If build failed we do not have a usable image to flash Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com> Assisted-by: claude-opus-4.6
bennyz
force-pushed
the
fix-flash-instruction
branch
from
f4fca3d to
a698323
Compare
2 participants
If build failed we do not have a usable image to flash
Summary
Related Issues
Type of Change
Testing
make test)make lint)make manifests generate)Summary by CodeRabbit
Bug Fixes
Tests