OCPBUGS-85608: Fix for OCP E2E Flakiness - [sig-cli] oc adm release extract image-references [Suite:openshift/conformance/parallel]#31175
Open
YamunadeviShanmugam wants to merge 1 commit into
Conversation
Contributor
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
openshift-ci-robot
added
jira/valid-reference
|
@YamunadeviShanmugam: This pull request references Jira Issue OCPBUGS-85608, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
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. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughRemoves an unused import and updates the ChangesRelease Extract Test Environment Awareness
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 10 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (10 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
|
@YamunadeviShanmugam: This pull request references Jira Issue OCPBUGS-85608, which is valid. 3 validation(s) were run on this bug
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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
openshift-merge-bot
Bot
added
the
ready-for-human-review
Contributor
|
Scheduling required tests: |
mgencur
reviewed
May 14, 2026
Contributor
Author
|
/retest |
|
Job Failure Risk Analysis for sha: 4d4accf
|
Contributor
Author
|
/retest |
YamunadeviShanmugam
force-pushed
the
fix-e2e-extract-image-ref
branch
from
42ca197 to
e6cc720
Compare
Contributor
|
Scheduling required tests: |
mgencur
reviewed
May 15, 2026
There was a problem hiding this comment.
🧹 Nitpick comments (2)
test/extended/cli/admin.go (2)
557-559: ⚡ Quick winRemove redundant error check.
The
if err != nilwrapper aroundo.Expect(err).NotTo(o.HaveOccurred())is redundant. Gomega'sExpectalready handles both nil and non-nil cases correctly. This pattern is inconsistent with Line 550, which uses the more idiomatic direct call.♻️ Simplify to match idiomatic Ginkgo/Gomega pattern
cleanup, regArgs, err := exutil.PrepareImagePullSecretAndCABundle(ocns) if cleanup != nil { defer cleanup() } - if err != nil { - o.Expect(err).NotTo(o.HaveOccurred(), "PrepareImagePullSecretAndCABundle failed") - } + o.Expect(err).NotTo(o.HaveOccurred(), "PrepareImagePullSecretAndCABundle failed")🤖 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 `@test/extended/cli/admin.go` around lines 557 - 559, Remove the redundant nil check around the Gomega expectation: delete the surrounding "if err != nil { ... }" and call o.Expect(err).NotTo(o.HaveOccurred(), "PrepareImagePullSecretAndCABundle failed") directly (the call to PrepareImagePullSecretAndCABundle and the o.Expect invocation are the symbols to update) so the test uses the idiomatic direct Gomega assertion like the earlier occurrence on line 550.
563-565: ⚡ Quick winRemove redundant error check and fix error message format.
Two issues here:
- The
if err != nilwrapper is redundant (same as lines 557-559).- The format string
"error%s"is missing a separator. Also,err.Error()is redundant since Gomega already includes the error details in the failure output.♻️ Simplify to idiomatic pattern
out, err := oc.AsAdmin().Run("adm", "release", "extract").Args(args...).Output() - if err != nil { - o.Expect(err).NotTo(o.HaveOccurred(), "oc adm release extract failed with error%s", err.Error()) - } + o.Expect(err).NotTo(o.HaveOccurred(), "oc adm release extract failed")🤖 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 `@test/extended/cli/admin.go` around lines 563 - 565, Remove the redundant "if err != nil" wrapper and simplify the failure assertion so it uses Gomega idiom directly: replace the whole conditional block that calls o.Expect(err).NotTo(o.HaveOccurred(), "oc adm release extract failed with error%s", err.Error()) with a single o.Expect(err).NotTo(o.HaveOccurred(), "oc adm release extract failed"), removing the explicit err.Error() and the malformed format string; locate the occurrence that references the err variable and o.Expect in the same vicinity as the earlier assertion (the duplicate check around the oc adm release extract call) and delete the redundant conditional.
🤖 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.
Nitpick comments:
In `@test/extended/cli/admin.go`:
- Around line 557-559: Remove the redundant nil check around the Gomega
expectation: delete the surrounding "if err != nil { ... }" and call
o.Expect(err).NotTo(o.HaveOccurred(), "PrepareImagePullSecretAndCABundle
failed") directly (the call to PrepareImagePullSecretAndCABundle and the
o.Expect invocation are the symbols to update) so the test uses the idiomatic
direct Gomega assertion like the earlier occurrence on line 550.
- Around line 563-565: Remove the redundant "if err != nil" wrapper and simplify
the failure assertion so it uses Gomega idiom directly: replace the whole
conditional block that calls o.Expect(err).NotTo(o.HaveOccurred(), "oc adm
release extract failed with error%s", err.Error()) with a single
o.Expect(err).NotTo(o.HaveOccurred(), "oc adm release extract failed"), removing
the explicit err.Error() and the malformed format string; locate the occurrence
that references the err variable and o.Expect in the same vicinity as the
earlier assertion (the duplicate check around the oc adm release extract call)
and delete the redundant conditional.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: bdaa9cab-3ae2-451f-a2b1-53ff3c35186e
📒 Files selected for processing (1)
test/extended/cli/admin.go
|
Let's just fix the two nitpicks from CodeRabbit. They're valid. Otherwise LGTM |
|
/lgtm |
addressed review comments
YamunadeviShanmugam
force-pushed
the
fix-e2e-extract-image-ref
branch
from
77e4ab7 to
de9c335
Compare
|
/lgtm |
Contributor
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: mgencur, YamunadeviShanmugam 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 |
Contributor
|
Scheduling required tests: |
Contributor
Author
|
/retest-required |
Contributor
Author
|
/retest |
Contributor
Author
|
/retest |
Contributor
|
@YamunadeviShanmugam: 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. |
|
Job Failure Risk Analysis for sha: de9c335
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The test was brittle as it relied on a hardcoded OpenShift 4.13 image reference from a public registry (quay.io), leading to "502 Bad Gateway" or "401 Unauthorized" errors. To resolve this, the test was refactored . It now retrieves the payload image directly from the cluster’s own clusterversion API, ensuring the test always targets the relevant version without hardcoded strings.
Summary by CodeRabbit