HIVE-3148: Hive test extension binary#31059
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
openshift-ci
Bot
added
the
do-not-merge/hold
WalkthroughAdds an Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller as Caller
participant Registry as extensionBinaries (registry)
participant Payload as ReleasePayload
participant Extractor as Extractor
participant Logger as Logger
Caller->>Registry: request list of test binaries
Registry->>Caller: returns entries (some optional)
loop for each entry
Caller->>Payload: request binary data for entry
Payload->>Extractor: provide data / path
Extractor->>Extractor: attempt extraction
alt extraction succeeds
Extractor->>Caller: success
else extraction fails
alt entry.optional == true
Extractor->>Logger: warn and skip
Extractor->>Caller: continue without error
else entry.optional == false
Extractor->>Caller: send error via errCh (propagate failure)
end
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15 minutes 🚥 Pre-merge checks | ✅ 12✅ Passed checks (12 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: miyadav 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 |
![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 the current code and only fix it if needed.
Inline comments:
In `@pkg/test/extensions/binary.go`:
- Around line 694-697: The current optional-binary handling inside the
extraction loop (where b.optional and b.imageTag are used) skips on any error,
which can hide real failures; change the logic to call a helper
isMissingFromPayloadError(err) (implement as per the suggested string checks)
and only warn+continue when that helper returns true; for any other error, log
it as an error/return it (don’t silently continue) so auth/transport/path issues
surface. Ensure the helper is used where the extraction error is checked and
maintain the existing log format for the skip case.
🪄 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: eeab91a2-f7da-467b-84d1-e0b02adaf8c2
📒 Files selected for processing (1)
pkg/test/extensions/binary.go
| if b.optional { | ||
| logrus.Warnf("Skipping optional extension %s (not in payload): %v", b.imageTag, err) | ||
| continue | ||
| } |
There was a problem hiding this comment.
Handle only “missing from payload” as skippable for optional binaries.
At Line 694, the current logic skips on any extraction error for optional binaries. That can mask real failures (auth/transport/path regressions) and silently drop coverage for callers that continue on success.
Proposed hardening
- if err != nil {
- if b.optional {
- logrus.Warnf("Skipping optional extension %s (not in payload): %v", b.imageTag, err)
- continue
- }
- errCh <- err
- continue
- }
+ if err != nil {
+ if b.optional && isMissingFromPayloadError(err) {
+ logrus.Warnf("Skipping optional extension %s (%s): %v", b.imageTag, b.binaryPath, err)
+ continue
+ }
+ errCh <- fmt.Errorf("failed extracting extension %s (%s): %w", b.imageTag, b.binaryPath, err)
+ continue
+ }func isMissingFromPayloadError(err error) bool {
msg := strings.ToLower(err.Error())
return strings.Contains(msg, "not found") ||
strings.Contains(msg, "manifest unknown") ||
strings.Contains(msg, "no such image")
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/test/extensions/binary.go` around lines 694 - 697, The current
optional-binary handling inside the extraction loop (where b.optional and
b.imageTag are used) skips on any error, which can hide real failures; change
the logic to call a helper isMissingFromPayloadError(err) (implement as per the
suggested string checks) and only warn+continue when that helper returns true;
for any other error, log it as an error/return it (don’t silently continue) so
auth/transport/path issues surface. Ensure the helper is used where the
extraction error is checked and maintain the existing log format for the skip
case.
|
Scheduling required tests: |
miyadav
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@miyadav: This pull request references HIVE-3148 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "5.0.0" version, but no target version was set. 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. |
openshift-ci
Bot
added
the
needs-rebase
miyadav
force-pushed
the
hivetestsbinary
branch
from
7e1febd to
150f48f
Compare
openshift-ci
Bot
removed
the
needs-rebase
|
/unhold |
openshift-ci
Bot
removed
the
do-not-merge/hold
There was a problem hiding this comment.
♻️ Duplicate comments (1)
pkg/test/extensions/binary.go (1)
697-700:⚠️ Potential issue | 🟠 MajorOnly skip optional binaries when the error indicates “missing from payload”.
At Line 697, skipping on any error can hide real extraction failures. Keep optional skips scoped to missing-image conditions; propagate all other errors.
🔧 Proposed hardening
testBinary, err := externalBinaryProvider.ExtractBinaryFromReleaseImage(b.imageTag, b.binaryPath) if err != nil { - if b.optional { + if b.optional && isMissingFromPayloadError(err) { logrus.Warnf("Skipping optional extension %s (not in payload): %v", b.imageTag, err) continue } - errCh <- err + errCh <- fmt.Errorf("failed extracting extension %s (%s): %w", b.imageTag, b.binaryPath, err) continue }+func isMissingFromPayloadError(err error) bool { + msg := strings.ToLower(err.Error()) + return strings.Contains(msg, "not found") || + strings.Contains(msg, "manifest unknown") || + strings.Contains(msg, "no such image") +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/test/extensions/binary.go` around lines 697 - 700, The current logic in the binary extraction loop (around b.optional and b.imageTag in pkg/test/extensions/binary.go) skips any error for optional binaries; change this so only errors that explicitly indicate "missing from payload" (or the specific sentinel/error type used by the extractor) are treated as benign: when err indicates missing-image, log the Warnf and continue, otherwise do not continue—propagate or return the error so extraction failures are surfaced; update the conditional to inspect the error message/type instead of blindly checking b.optional.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@pkg/test/extensions/binary.go`:
- Around line 697-700: The current logic in the binary extraction loop (around
b.optional and b.imageTag in pkg/test/extensions/binary.go) skips any error for
optional binaries; change this so only errors that explicitly indicate "missing
from payload" (or the specific sentinel/error type used by the extractor) are
treated as benign: when err indicates missing-image, log the Warnf and continue,
otherwise do not continue—propagate or return the error so extraction failures
are surfaced; update the conditional to inspect the error message/type instead
of blindly checking b.optional.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: accf18c8-4ad4-4ab8-89eb-1f457fd5b472
📒 Files selected for processing (1)
pkg/test/extensions/binary.go
|
@miyadav: 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. |
|
/hold |
openshift-ci
Bot
added
the
do-not-merge/hold
|
/close |
|
@miyadav: Closed this PR. 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 kubernetes-sigs/prow repository. |
2 participants
/hold
depends on - openshift/hive#2894
Summary by CodeRabbit