Fix ListImages JSON parsing when extension binaries emit log lines#31009
Fix ListImages JSON parsing when extension binaries emit log lines#31009stbenjam wants to merge 3 commits intoopenshift:mainfrom
Conversation
… lines ListImages was unmarshalling the entire stdout output directly, so any warning or debug log lines emitted by extension binaries before the JSON payload caused parsing failures (e.g. "invalid character 'W' looking for beginning of value"). The Info method already handled this by scanning for the first JSON-starting line, but ListImages did not. Extract a shared extractJSON helper that both Info and ListImages now use, eliminating the duplicated JSON detection logic. The helper also handles the case where a binary outputs "null" (no images), which previously fell through to an unhelpful parse error. Co-Authored-By: Claude Opus 4.6 <[email protected]>
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdded an Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (8 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: stbenjam The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/test/extensions/binary.go`:
- Around line 354-356: The helper extractJSON currently returns JSON "null" to
all callers which lets callers like Info unmarshal into zero-value Extension
silently; change extractJSON to either accept an allowNull bool parameter or
create a separate extractNullableJSON so that extractJSON fails when trimmed ==
[]byte("null") unless the caller explicitly allows null; update callers
(including Info and the other callers referenced around the same spots) to pass
true only where null is acceptable and false (or call the non-nullable helper)
where it should be treated as an error.
- Around line 359-365: The current end detection using LastIndexByte on output
is brittle (variables: jsonBegins, jsonEnds, endChar, output); replace the
manual end-boundary logic by feeding output[jsonBegins:] into a json.Decoder and
decode into a json.RawMessage (or interface{}) so the decoder extracts exactly
the first JSON value and returns its raw bytes, rather than relying on
LastIndexByte which can be thrown off by trailing logs; update the return to use
the decoded RawMessage result instead of slicing by jsonEnds.
🪄 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: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 3a352c1f-4639-4ef9-b0e6-88ce63de0946
📒 Files selected for processing (1)
pkg/test/extensions/binary.go
…tImages - Replace brittle LastIndexByte end-boundary detection with json.Decoder, which correctly extracts the first complete JSON value regardless of trailing log output. - Remove allowNull parameter from extractJSON; handle the null case in ListImages where it's semantically relevant, keeping the helper simple. Co-Authored-By: Claude Opus 4.6 <[email protected]>
Drop the startChar parameter — the helper now finds the first line
starting with '{' or '[' automatically. Use json.Decoder to extract
the complete JSON value, which correctly handles trailing log lines
that might contain braces. Keep null handling in ListImages where
it's semantically relevant.
Co-Authored-By: Claude Opus 4.6 <[email protected]>
|
Scheduling required tests: |
|
/test e2e-agnostic-ipv6 |
|
/test e2e-metal-ipi-ovn-ipv6 |
|
@stbenjam: The following tests failed, say
Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
2 participants
Summary
ListImageswas directly unmarshalling the entire stdout from extension binaries, so any warning/debug log lines before the JSON payload caused parse failures (invalid character 'W' looking for beginning of value).extractJSONhelper used by bothInfoandListImages, eliminating duplicated JSON detection logic.null(no images to report), which previously caused an unhelpful parse error.Test plan
openshift-tests images --to-repository=repo/localimages/asdfwith a local extension binary override that emits warning lines before JSON output — previously failed, now succeeds.🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Refactor
Tests