Reapply "Merge pull request #476 from mfbonfigli/SPLAT-2713_update-cc…#492
Reapply "Merge pull request #476 from mfbonfigli/SPLAT-2713_update-cc…#492mfbonfigli wants to merge 1 commit into
Conversation
…update-cccmo-tests" This reverts commit cfb95ec.
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
WalkthroughAWS test region resolution now uses shared validation and Infrastructure lookup, with environment-based fallback selection. The BareMetal config-sync test now waits for the expected empty ConfigMap state. ChangesAWS region discovery
BareMetal test synchronization
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant TestInitialization
participant Environment
participant Infrastructure
participant AWSConfig
TestInitialization->>Environment: read and validate region candidates
Environment-->>TestInitialization: valid region or no match
TestInitialization->>Infrastructure: request cluster AWS region
Infrastructure-->>TestInitialization: return region
TestInitialization->>AWSConfig: set validated region
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 10 | ❌ 5❌ Failed checks (4 warnings, 1 inconclusive)
✅ Passed checks (10 passed)
✨ 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 |
![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 `@pkg/controllers/cloud_config_sync_controller_test.go`:
- Around line 607-611: Update the Eventually callback in the config map cleanup
test to use an injected Gomega instance, replacing the global Expect around
cl.List with g.Expect so transient list errors are retried. Preserve the
existing namespace filter and zero-length assertion.
🪄 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: Enterprise
Run ID: bad9daf6-aecd-49da-a165-e3a0fa5371c9
⛔ Files ignored due to path filters (4)
openshift-tests/ccm-aws-tests/go.sumis excluded by!**/*.sumopenshift-tests/ccm-aws-tests/vendor/k8s.io/cloud-provider-aws/tests/e2e/cloudconfig.gois excluded by!**/vendor/**openshift-tests/ccm-aws-tests/vendor/k8s.io/cloud-provider-aws/tests/e2e/loadbalancer.gois excluded by!**/vendor/**openshift-tests/ccm-aws-tests/vendor/modules.txtis excluded by!**/vendor/**
📒 Files selected for processing (5)
openshift-tests/ccm-aws-tests/e2e/aws/helper.goopenshift-tests/ccm-aws-tests/e2e/common/helper.goopenshift-tests/ccm-aws-tests/go.modopenshift-tests/ccm-aws-tests/main.gopkg/controllers/cloud_config_sync_controller_test.go
| Eventually(func() int { | ||
| allCMs := &corev1.ConfigMapList{} | ||
| Expect(cl.List(ctx, allCMs, &client.ListOptions{Namespace: targetNamespaceName})).To(Succeed()) | ||
| return len(allCMs.Items) | ||
| }, 2*time.Minute, 5*time.Second).Should(BeZero()) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the surrounding section of the test
sed -n '560,635p' pkg/controllers/cloud_config_sync_controller_test.go
# Look for nearby Eventually usage patterns in the repo
rg -n "Eventually\\(func\\(" pkg/controllers -g '*.go' | head -n 40
# Check whether this file imports and uses Gomega's callback-with-g pattern elsewhere
rg -n "func\\(g Gomega\\)|WithT\\(|Expect\\(" pkg/controllers/cloud_config_sync_controller_test.goRepository: openshift/cluster-cloud-controller-manager-operator
Length of output: 15174
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '560,635p' pkg/controllers/cloud_config_sync_controller_test.go
rg -n "Eventually\\(func\\(" pkg/controllers -g '*.go' | head -n 40
rg -n "func\\(g Gomega\\)|WithT\\(|Expect\\(" pkg/controllers/cloud_config_sync_controller_test.goRepository: openshift/cluster-cloud-controller-manager-operator
Length of output: 15174
🌐 Web query:
Gomega Eventually callback global Expect inside function fails immediately transient error retry behavior
💡 Result:
In Gomega, if you use the global Expect function inside a callback passed to Eventually, any failure of that assertion will cause the entire test to fail immediately, bypassing the retry mechanism [1][2][3]. This happens because the global Expect is not designed to be caught and retried by Eventually [2]. To achieve the desired transient error retry behavior, you must use a Gomega Gomega instance provided by the callback itself [2][4]. When you define your callback to accept a Gomega argument, Gomega passes a special instance that captures failures and allows Eventually to treat them as transient, triggering a retry instead of an immediate test failure [5][2][4]. Correct usage pattern: Eventually(func(g Gomega) { // Use the provided 'g' instead of the global 'Expect' g.Expect(actual).To(Equal(expected)) }, "5s", "100ms").Should(Succeed) By using the injected g.Expect, failures inside the function are intercepted by Eventually, allowing it to poll and retry until the assertion succeeds or the timeout is reached [5][2]. Using the global Expect will cause the test to exit immediately upon the first failure [1][3].
Citations:
- 1: Gomega v1.15.0 behaviour change of Expect() within Eventually() onsi/gomega#460
- 2: https://onsi.github.io/ginkgo/
- 3: Ginkgohelper does not work well with Eventually onsi/ginkgo#1322
- 4: Gomega not catching nils for pointers to structs onsi/gomega#198
- 5: is nested Eventually supported onsi/gomega#458
Use g.Expect inside Eventually
The global Expect here can fail the spec immediately on a transient List error, bypassing Eventually retries. Use Eventually(func(g Gomega) int { ... g.Expect(...) ... }) or return (int, error) from the callback instead.
🤖 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 `@pkg/controllers/cloud_config_sync_controller_test.go` around lines 607 - 611,
Update the Eventually callback in the config map cleanup test to use an injected
Gomega instance, replacing the global Expect around cl.List with g.Expect so
transient list errors are retried. Preserve the existing namespace filter and
zero-length assertion.
1 participant
Summary
Re-applies #476, which was reverted in #491 due to two NLB BYO Security Group transition tests failing in upgrade jobs.
The root cause of the issue is not related to the tests, but related to the fact that during cluster upgrades the master node IAM permissions do not get updated by the installer. This results in a 5.0 cluster without the new
SetSecurityGrouppermission, which is required for the e2e tests to pass, causing the failures. Since 4.22 > 5.0 upgrade jobs do not run during presubmits this was noticed only after the PR was merged.Those failures are being addressed separately (openshift/release#81974) by patching the master IAM role in the CI upgrade step (openshift/release) prior to the test execution. This PR is to be merged only after that one is merged.
What this brings back
Updates the vendored cloud-provider-aws e2e test module, pulling in upstream NLB BYO Security Group transition tests
Fixes getRegionFromEnv() to validate the value of LEASED_RESOURCE against the AWS region format before using it as AWS_REGION, this prevents HyperShift CI from poisoning the region with a UUID lease value
Adds Infrastructure object fallback for region discovery when no valid region is found in environment variables
Moves AWSRegionPattern and GetRegionFromInfrastructure to the common package for shared use between main.go and the aws e2e package
Fixes a flaky unit test for BareMetal platform config sync
Summary by CodeRabbit