Open
Conversation
WalkthroughAdds context-aware retry polling for image info retrieval in the E2E CLI test and replaces nested architecture-specific conditionals with a switch-based validation that logs detected architecture and fails on unsupported arches. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Comment Tip CodeRabbit can use OpenGrep to find security vulnerabilities and bugs across 17+ programming languages.OpenGrep is compatible with Semgrep configurations. Add an |
Member
|
This looks good to me. Ping me once OTE tests pass. |
![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)
test/e2e/cli.go (1)
475-507:⚠️ Potential issue | 🟠 MajorPoll until the asserted condition is true, not just until
oc image infostops erroring.Both retry loops return success on first successful command execution, but the actual correctness checks happen later (Line 486 and Lines 515/518/521/524). That still leaves a flake window if registry metadata is not fully visible yet. Fold the expected-content check into the poll condition.
Suggested reliability fix
err = wait.Poll(30*time.Second, 180*time.Second, func() (bool, error) { var infoErr error out, infoErr = oc.WithoutNamespace().Run("image").Args("info", serInfo.serviceName+"/testimage:ppc64", "--insecure").Output() if infoErr != nil { e2e.Logf("image info failed, retrying: %v", infoErr) return false, nil } + if !strings.Contains(out, "ppc64le") { + e2e.Logf("image info does not contain ppc64le yet, retrying") + return false, nil + } return true, nil })As per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/cli.go` around lines 475 - 507, The poll loops currently return success as soon as oc image info/mirror stops returning an error, but the actual assertions (strings.Contains checks on out and imageInfo) happen after the poll—move the expected-content checks into the poll closure so the poll only returns true when the command succeeds AND the output contains the expected substring; specifically, in the first wait.Poll that runs oc.WithoutNamespace().Run("image").Args("info", serInfo.serviceName+"/testimage:ppc64"...).Output(), check that out contains "ppc64le" before returning true (and keep logging/returning false on errors or missing content), and in the later poll that queries serInfo.serviceName+"/testimage:default" ensure imageInfo contains the expected indicator (e.g. the mirrored tag or relevant metadata) before returning true; remove or simplify the subsequent o.Expect(...) assertions that duplicate these checks and keep AssertWaitPollNoErr as-is to surface timeout failures.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@test/e2e/cli.go`:
- Around line 475-507: The poll loops currently return success as soon as oc
image info/mirror stops returning an error, but the actual assertions
(strings.Contains checks on out and imageInfo) happen after the poll—move the
expected-content checks into the poll closure so the poll only returns true when
the command succeeds AND the output contains the expected substring;
specifically, in the first wait.Poll that runs
oc.WithoutNamespace().Run("image").Args("info",
serInfo.serviceName+"/testimage:ppc64"...).Output(), check that out contains
"ppc64le" before returning true (and keep logging/returning false on errors or
missing content), and in the later poll that queries
serInfo.serviceName+"/testimage:default" ensure imageInfo contains the expected
indicator (e.g. the mirrored tag or relevant metadata) before returning true;
remove or simplify the subsequent o.Expect(...) assertions that duplicate these
checks and keep AssertWaitPollNoErr as-is to surface timeout failures.
Contributor
Author
|
/payload 4.22 nightly blocking |
Contributor
|
@gangwgr: trigger 14 job(s) of type blocking for the nightly release of OCP 4.22
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/b2428e70-2107-11f1-807e-fdf1dee9b118-0 |
Contributor
Author
|
/payload 4.22 nightly informing |
Contributor
|
@gangwgr: trigger 65 job(s) of type informing for the nightly release of OCP 4.22
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/b98588e0-2107-11f1-9295-2cc5f42bee78-0 |
ardaguclu
reviewed
Mar 16, 2026
test/e2e/cli.go
Outdated
| AssertWaitPollNoErr(err, fmt.Sprintf("max time reached but mirror still falied")) | ||
| out, err := oc.WithoutNamespace().Run("image").Args("info", serInfo.serviceName+"/testimage:ppc64", "--insecure").Output() | ||
| var out string | ||
| err = wait.Poll(30*time.Second, 180*time.Second, func() (bool, error) { |
Member
There was a problem hiding this comment.
30 seconds is long time for polling. Can it be 5 seconds?.
ardaguclu
reviewed
Mar 16, 2026
test/e2e/cli.go
Outdated
| o.Expect(err).NotTo(o.HaveOccurred()) | ||
| imageInfo, err := oc.WithoutNamespace().Run("image").Args("info", serInfo.serviceName+"/testimage:default", "--insecure").Output() | ||
| var imageInfo string | ||
| err = wait.Poll(30*time.Second, 180*time.Second, func() (bool, error) { |
gangwgr
force-pushed
the
new-ci-azure-fix
branch
from
25a54d7 to
1f395ca
Compare
Member
|
/lgtm |
Contributor
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ardaguclu, gangwgr 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
There was a problem hiding this comment.
🧹 Nitpick comments (2)
test/e2e/cli.go (2)
475-486: Polling pattern looks good; consider removing redundant error check.The 5-second polling interval addresses the prior feedback. However, line 486 is redundant —
AssertWaitPollNoErralready fails the test iferr != nil(seetest/e2e/util.go:354-359).♻️ Optional: remove redundant assertion
AssertWaitPollNoErr(err, "max time reached but image info for testimage:ppc64 still failed") - o.Expect(err).NotTo(o.HaveOccurred()) o.Expect(strings.Contains(out, "ppc64le")).To(o.BeTrue())🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/cli.go` around lines 475 - 486, The test duplicates failure handling after polling: AssertWaitPollNoErr(err, ...) already fails on non-nil err, so remove the redundant o.Expect(err).NotTo(o.HaveOccurred()) assertion at the end of the block; keep the wait.PollUntilContextTimeout loop and the AssertWaitPollNoErr call (referencing the err variable and the AssertWaitPollNoErr helper) and delete the extra o.Expect(...) line to avoid duplicate assertions.
498-509: Same redundant assertion pattern.Line 509 duplicates the error check already performed by
AssertWaitPollNoErron line 508.♻️ Optional: remove redundant assertion
AssertWaitPollNoErr(err, "max time reached but image info for testimage:default still failed") - o.Expect(err).NotTo(o.HaveOccurred()) architecture, err := exec.Command("bash", "-c", "uname -a").Output()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/cli.go` around lines 498 - 509, The code calls AssertWaitPollNoErr(err, ...) to fail on timeout and then redundantly asserts o.Expect(err).NotTo(o.HaveOccurred()); remove the duplicate check by deleting the o.Expect(...) line. Keep the wait.PollUntilContextTimeout usage and the AssertWaitPollNoErr(err, "max time reached but image info for testimage:default still failed") call (and the related err variable and imageInfo assignment) so the test still fails correctly on timeout, but do not perform the second assertion using o.Expect/HaveOccurred.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/e2e/cli.go`:
- Around line 475-486: The test duplicates failure handling after polling:
AssertWaitPollNoErr(err, ...) already fails on non-nil err, so remove the
redundant o.Expect(err).NotTo(o.HaveOccurred()) assertion at the end of the
block; keep the wait.PollUntilContextTimeout loop and the AssertWaitPollNoErr
call (referencing the err variable and the AssertWaitPollNoErr helper) and
delete the extra o.Expect(...) line to avoid duplicate assertions.
- Around line 498-509: The code calls AssertWaitPollNoErr(err, ...) to fail on
timeout and then redundantly asserts o.Expect(err).NotTo(o.HaveOccurred());
remove the duplicate check by deleting the o.Expect(...) line. Keep the
wait.PollUntilContextTimeout usage and the AssertWaitPollNoErr(err, "max time
reached but image info for testimage:default still failed") call (and the
related err variable and imageInfo assignment) so the test still fails correctly
on timeout, but do not perform the second assertion using o.Expect/HaveOccurred.
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
2 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.
Fix for failing on azure https://sippy.dptools.openshift.org/sippy-ng/component_readiness/env_test?Network=ovn&Plat[…]stId=openshift-tests%3A6aa411d63187917383ebca75e562e781