CNTRLPLANE-3222: Migrate self-managed Azure e2e tests to v2 Ginkgo framework - #8204
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bryan-cox 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
|
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:
📝 WalkthroughWalkthroughThis PR extracts validation logic from several e2e test wrappers into standalone helpers that accept testing.TB, widens multiple helper signatures from *testing.T to testing.TB, and adds focused validation functions for Azure workload identity webhook mutation, OAuth via LoadBalancer, kube-apiserver allowed CIDRs, and ingress operator configuration. It registers two optional Azure env vars and introduces a new e2e test suite (build tag e2ev2) with Azure-specific tests for public clusters, private topology, and OAuth LoadBalancer flows. Minor adjustments to polling utilities and kubeconfig validation were made. Sequence Diagram(s)mermaid TestRunner->>MgmtAPI: Read HostedCluster (platform, topology, publishingStrategy) Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 2 warnings)
✅ Passed checks (7 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
Bot
added
area/documentation
f5acfaf to
a2c6727
Compare
![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
🧹 Nitpick comments (3)
test/e2e/util/util_ingress_operator_configuration.go (1)
20-20: UnusedmgmtClientparameter.The
mgmtClientparameter is declared but not used withinValidateIngressOperatorConfiguration. If it's not needed, consider removing it to keep the function signature clean.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/util/util_ingress_operator_configuration.go` at line 20, The function ValidateIngressOperatorConfiguration declares an unused parameter mgmtClient; remove mgmtClient from the function signature and update all call sites to stop passing that argument (or, if removal is risky, replace the parameter with _ to explicitly ignore it). Search for ValidateIngressOperatorConfiguration usages and adjust them accordingly so the signature and callers remain consistent.test/e2e/v2/internal/test_context.go (1)
178-182: Consider checking for empty kubeconfig data.The code checks if the
kubeconfigkey exists but doesn't verify if the data is non-empty. An empty byte slice would pass this check but fail during REST config creation with a potentially confusing error.Suggested improvement
kubeconfigData, ok := kubeconfigSecret.Data["kubeconfig"] - if !ok { - return fmt.Errorf("kubeconfig key not found in secret %s/%s", hc.Namespace, kubeconfigSecretName) + if !ok || len(kubeconfigData) == 0 { + return fmt.Errorf("kubeconfig key not found or empty in secret %s/%s", hc.Namespace, kubeconfigSecretName) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/v2/internal/test_context.go` around lines 178 - 182, The current extraction of kubeconfigData only checks presence of the "kubeconfig" key but not that its value is non-empty; update the logic around kubeconfigData (the variable extracted from kubeconfigSecret.Data["kubeconfig"]) to verify len(kubeconfigData) > 0 and return a clear error (including hc.Namespace and kubeconfigSecretName) when the slice is empty so the subsequent REST config creation doesn't fail with a confusing error.test/e2e/util/azure.go (1)
23-25: Consider adding cleanup for test resources.The test creates a namespace, service account, and pod but doesn't clean them up. While this may be acceptable if tests run in ephemeral clusters, adding deferred cleanup would make the test more robust and prevent resource accumulation in long-running test environments.
Suggested approach
// After creating the namespace t.Cleanup(func() { _ = guestClient.Delete(context.Background(), testNamespace) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/util/azure.go` around lines 23 - 25, Add deferred cleanup after creating test resources: after creating testNamespace (nsName) call t.Cleanup (or defer) to delete the namespace and any created ServiceAccount/Pod using guestClient.Delete so resources are removed when the test finishes; reference guestClient.Delete(ctx, testNamespace) and similar deletes for the ServiceAccount and Pod objects (use context.Background() or the existing ctx) to ensure robust teardown.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/e2e/util/util.go`:
- Around line 3478-3490: The ValidateKubeAPIServerAllowedCIDRs function mutates
hc.Spec.Networking.APIServer.AllowedCIDRBlocks (via ensureAPIServerAllowedCIDRs)
and does not restore it; capture the original APIServer networking value at the
start (e.g., save a copy of hc.Spec.Networking.APIServer or AllowedCIDRBlocks),
then defer an UpdateObject call that restores the saved value to the hosted
cluster before returning. Use the same mgmtClient.Update/UpdateObject helper
used elsewhere, reference ValidateKubeAPIServerAllowedCIDRs,
hc.Spec.Networking.APIServer.AllowedCIDRBlocks, and ensureAPIServerAllowedCIDRs
so the original CIDRs are reinstated even if the test fails.
In `@test/e2e/v2/selfmanagedazure/private_topology_test.go`:
- Around line 67-101: The current EventuallyObjects usage applies per-object
predicates to every AzurePrivateLinkService returned by the List, causing
failures when unrelated or still-reconciling PLSes exist; update the logic in
the AzurePrivateLinkService check (the anonymous collection predicate passed to
EventuallyObjects that inspects AzurePrivateLinkServiceList /
AzurePrivateLinkService items) so it either filters the list down to the target
object(s) before returning or implements an "any-match" readiness test (i.e.,
scan items and succeed as soon as one AzurePrivateLinkService has a non-empty
Status.PrivateLinkServiceAlias / endpoint IP / DNS zone), rather than requiring
all items to be ready. Ensure this change is applied to the three similar blocks
that check alias/endpoint IP/DNS zone so EventuallyObjects observes at least one
ready item.
---
Nitpick comments:
In `@test/e2e/util/azure.go`:
- Around line 23-25: Add deferred cleanup after creating test resources: after
creating testNamespace (nsName) call t.Cleanup (or defer) to delete the
namespace and any created ServiceAccount/Pod using guestClient.Delete so
resources are removed when the test finishes; reference guestClient.Delete(ctx,
testNamespace) and similar deletes for the ServiceAccount and Pod objects (use
context.Background() or the existing ctx) to ensure robust teardown.
In `@test/e2e/util/util_ingress_operator_configuration.go`:
- Line 20: The function ValidateIngressOperatorConfiguration declares an unused
parameter mgmtClient; remove mgmtClient from the function signature and update
all call sites to stop passing that argument (or, if removal is risky, replace
the parameter with _ to explicitly ignore it). Search for
ValidateIngressOperatorConfiguration usages and adjust them accordingly so the
signature and callers remain consistent.
In `@test/e2e/v2/internal/test_context.go`:
- Around line 178-182: The current extraction of kubeconfigData only checks
presence of the "kubeconfig" key but not that its value is non-empty; update the
logic around kubeconfigData (the variable extracted from
kubeconfigSecret.Data["kubeconfig"]) to verify len(kubeconfigData) > 0 and
return a clear error (including hc.Namespace and kubeconfigSecretName) when the
slice is empty so the subsequent REST config creation doesn't fail with a
confusing error.
🪄 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 YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 1799fad7-46e9-4a05-9392-fccbd51ba988
📒 Files selected for processing (14)
Makefiledocs/superpowers/plans/2026-04-10-migrate-self-managed-azure-e2e-to-v2.mddocs/superpowers/specs/2026-04-10-migrate-self-managed-azure-e2e-to-v2-design.mdtest/e2e/util/azure.gotest/e2e/util/eventually.gotest/e2e/util/oauth.gotest/e2e/util/util.gotest/e2e/util/util_ingress_operator_configuration.gotest/e2e/v2/internal/env_vars.gotest/e2e/v2/internal/test_context.gotest/e2e/v2/selfmanagedazure/oauth_lb_test.gotest/e2e/v2/selfmanagedazure/private_topology_test.gotest/e2e/v2/selfmanagedazure/public_cluster_test.gotest/e2e/v2/selfmanagedazure/suite_test.go
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/util/oauth.go (1)
128-137:⚠️ Potential issue | 🟠 MajorAdd per-attempt request context and client timeout in OAuth token polling.
httpClient.Do(request)reuses a request not bound to the poll callback context and uses no client timeout. A slow/hung network call can outlivePollUntilContextTimeout, causing long or stuck test runs.Suggested fix
- httpClient := &http.Client{Transport: transport} + httpClient := &http.Client{ + Transport: transport, + Timeout: 15 * time.Second, + } httpClient.CheckRedirect = func(req *http.Request, via []*http.Request) error { // don't resolve redirects and return the response instead return http.ErrUseLastResponse } @@ - resp, err := httpClient.Do(request) + req := request.Clone(ctx) + resp, err := httpClient.Do(req)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/util/oauth.go` around lines 128 - 137, The polling callback uses a pre-built request and an http.Client with no timeout, so individual attempts can hang beyond PollUntilContextTimeout; inside the wait.PollUntilContextTimeout callback create a new per-attempt request bound to the provided ctx (use http.NewRequestWithContext or request.Clone().WithContext(ctx)) and use an http.Client with a per-attempt timeout (set httpClient.Timeout or construct a short-lived client inside the callback) before calling httpClient.Do(request) so each attempt is cancellable by the poll context and cannot hang indefinitely; update references around httpClient.Do, request, and the transport/CheckRedirect setup accordingly.
🧹 Nitpick comments (1)
test/e2e/v2/selfmanagedazure/suite_test.go (1)
27-39: Fail fast when only one hosted-cluster env var is set.Line 36-Line 39 currently skips guest setup silently if one of the two hosted-cluster env vars is missing, which can defer failures into specs with less actionable errors. Add an explicit pair-validation check in
BeforeSuite.Suggested patch
var _ = BeforeSuite(func() { ctx := context.Background() ctrl.SetLogger(zap.New()) + hcName := internal.GetEnvVarValue("E2E_HOSTED_CLUSTER_NAME") + hcNamespace := internal.GetEnvVarValue("E2E_HOSTED_CLUSTER_NAMESPACE") + if (hcName == "") != (hcNamespace == "") { + Fail("E2E_HOSTED_CLUSTER_NAME and E2E_HOSTED_CLUSTER_NAMESPACE must be set together") + } + testCtx, err := internal.SetupTestContextFromEnv(ctx) Expect(err).NotTo(HaveOccurred(), "failed to setup test context") Expect(testCtx).NotTo(BeNil(), "test context should not be 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/v2/selfmanagedazure/suite_test.go` around lines 27 - 39, BeforeSuite currently silently skips guest client setup when only one of testCtx.ClusterName or testCtx.ClusterNamespace is set; add an explicit pair-validation right after testCtx is created: if exactly one of testCtx.ClusterName or testCtx.ClusterNamespace is empty, fail fast (use Expect/Fail) with a clear message indicating both env vars must be provided together. Locate the BeforeSuite block and add the check referencing testCtx.ClusterName, testCtx.ClusterNamespace and testCtx.SetupGuestClient so the suite errors immediately instead of deferring to later specs.
🤖 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/util/oauth.go`:
- Around line 128-137: The polling callback uses a pre-built request and an
http.Client with no timeout, so individual attempts can hang beyond
PollUntilContextTimeout; inside the wait.PollUntilContextTimeout callback create
a new per-attempt request bound to the provided ctx (use
http.NewRequestWithContext or request.Clone().WithContext(ctx)) and use an
http.Client with a per-attempt timeout (set httpClient.Timeout or construct a
short-lived client inside the callback) before calling httpClient.Do(request) so
each attempt is cancellable by the poll context and cannot hang indefinitely;
update references around httpClient.Do, request, and the transport/CheckRedirect
setup accordingly.
---
Nitpick comments:
In `@test/e2e/v2/selfmanagedazure/suite_test.go`:
- Around line 27-39: BeforeSuite currently silently skips guest client setup
when only one of testCtx.ClusterName or testCtx.ClusterNamespace is set; add an
explicit pair-validation right after testCtx is created: if exactly one of
testCtx.ClusterName or testCtx.ClusterNamespace is empty, fail fast (use
Expect/Fail) with a clear message indicating both env vars must be provided
together. Locate the BeforeSuite block and add the check referencing
testCtx.ClusterName, testCtx.ClusterNamespace and testCtx.SetupGuestClient so
the suite errors immediately instead of deferring to later specs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: e4476d71-f433-4cea-8376-4e688ec4a6fb
📒 Files selected for processing (12)
Makefiletest/e2e/util/azure.gotest/e2e/util/eventually.gotest/e2e/util/oauth.gotest/e2e/util/util.gotest/e2e/util/util_ingress_operator_configuration.gotest/e2e/v2/internal/env_vars.gotest/e2e/v2/internal/test_context.gotest/e2e/v2/selfmanagedazure/oauth_lb_test.gotest/e2e/v2/selfmanagedazure/private_topology_test.gotest/e2e/v2/selfmanagedazure/public_cluster_test.gotest/e2e/v2/selfmanagedazure/suite_test.go
✅ Files skipped from review due to trivial changes (3)
- test/e2e/v2/selfmanagedazure/oauth_lb_test.go
- test/e2e/v2/selfmanagedazure/private_topology_test.go
- test/e2e/v2/selfmanagedazure/public_cluster_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
- test/e2e/util/util_ingress_operator_configuration.go
- test/e2e/v2/internal/test_context.go
- test/e2e/util/azure.go
- test/e2e/util/util.go
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8204 +/- ##
==========================================
+ Coverage 37.23% 37.42% +0.18%
==========================================
Files 752 751 -1
Lines 91829 91850 +21
==========================================
+ Hits 34195 34373 +178
+ Misses 54993 54840 -153
+ Partials 2641 2637 -4 see 38 files with indirect coverage changes
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/e2e/v2/selfmanagedazure/suite_test.go (1)
27-42: Consider using Ginkgo-managed context in BeforeSuite for proper cancellation handling.
BeforeSuitein Ginkgo v2 can accept acontext.Contextparameter that Ginkgo manages. Usingcontext.Background()means the context won't be cancelled if the suite is interrupted during setup, potentially causing hanging operations.♻️ Suggested improvement
-var _ = BeforeSuite(func() { - ctx := context.Background() +var _ = BeforeSuite(func(ctx context.Context) { ctrl.SetLogger(zap.New()) testCtx, err := internal.SetupTestContextFromEnv(ctx)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/v2/selfmanagedazure/suite_test.go` around lines 27 - 42, The BeforeSuite is creating ctx with context.Background() so it isn’t cancelled by Ginkgo; change the BeforeSuite handler to accept the Ginkgo-managed context parameter (use BeforeSuite(func(ctx context.Context) { ... })), then pass that ctx into internal.SetupTestContextFromEnv and into testCtx.SetupHostedClusterClient (instead of the background context) so both SetupTestContextFromEnv and SetupHostedClusterClient use the Ginkgo-managed cancellable context, and keep the final internal.SetTestContext(testCtx) call as-is.
🤖 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/v2/selfmanagedazure/suite_test.go`:
- Around line 27-42: The BeforeSuite is creating ctx with context.Background()
so it isn’t cancelled by Ginkgo; change the BeforeSuite handler to accept the
Ginkgo-managed context parameter (use BeforeSuite(func(ctx context.Context) {
... })), then pass that ctx into internal.SetupTestContextFromEnv and into
testCtx.SetupHostedClusterClient (instead of the background context) so both
SetupTestContextFromEnv and SetupHostedClusterClient use the Ginkgo-managed
cancellable context, and keep the final internal.SetTestContext(testCtx) call
as-is.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: c527c971-2ae7-499f-9221-8499b7350e2e
📒 Files selected for processing (5)
test/e2e/v2/internal/test_context.gotest/e2e/v2/selfmanagedazure/oauth_lb_test.gotest/e2e/v2/selfmanagedazure/private_topology_test.gotest/e2e/v2/selfmanagedazure/public_cluster_test.gotest/e2e/v2/selfmanagedazure/suite_test.go
✅ Files skipped from review due to trivial changes (3)
- test/e2e/v2/selfmanagedazure/oauth_lb_test.go
- test/e2e/v2/selfmanagedazure/public_cluster_test.go
- test/e2e/v2/selfmanagedazure/private_topology_test.go
|
Now I have all the evidence I need for a comprehensive report. Let me compile the final analysis. Test Failure Analysis CompleteJob Information
Key Finding: These Are NOT Prow CI JobsThese are Red Hat Konflux Enterprise Contract (EC) verification checks, not Prow CI test jobs. EC verification runs after Konflux builds the container image and checks the image against compliance policies (signatures, attestations, provenance, SBOM, etc.). The failure details are only accessible through the Konflux UI (a single-page app requiring authentication), not through the GitHub API or GCS artifacts. Root Cause AnalysisThe PR Does Not Cause These FailuresPR #8204 modifies only test files and the Makefile — none of which affect the container image:
The Root Cause: Transient Attestation/Signing Race ConditionTiming analysis of the Konflux pipeline runs reveals a clear pattern — EC checks that started too soon after the image build completed failed, while the one that had more propagation time passed:
The The Corroborating Evidence
EvidenceStack Trace / Error DetailsThe specific 2 policy violations per check are not accessible through the GitHub API (only summary counts are exposed). The detailed violation messages are in the Konflux UI pipeline logs at:
Based on the timing pattern, the 2 failures are most likely in attestation-related policies (e.g., Recommendations
Artifacts
|
689ece0 to
a6b95c7
Compare
|
Addressed all CodeRabbit review feedback in 161bb4a: Major fixes:
Nitpick fixes:
Not applicable (files deleted):
|
161bb4a to
6b4f9ea
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/e2e/util/util.go (1)
320-324:⚠️ Potential issue | 🟠 MajorRequire non-empty kubeconfig data before returning.
This helper still treats
secret.Data["kubeconfig"]as ready when the value is empty, and several callers immediately pass the bytes intoRESTConfigFromKubeConfig. That reintroduces the same empty-secret flake you just fixed intest/e2e/v2/internal/test_context.go.💡 Suggested fix
[]Predicate[*corev1.Secret]{ func(secret *corev1.Secret) (done bool, reasons string, err error) { var hasData bool data, hasData = secret.Data["kubeconfig"] - return hasData, "expected secret to contain kubeconfig in data", nil + return hasData && len(data) > 0, "expected secret to contain non-empty kubeconfig data", nil }, }, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/util/util.go` around lines 320 - 324, The predicate that checks secret.Data["kubeconfig"] should require non-empty bytes before returning success: in the anonymous Predicate[*corev1.Secret] function (the one that currently does `data, hasData = secret.Data["kubeconfig"]`), change the condition to ensure data is not nil and len(data) > 0 (e.g., `hasData && len(data) > 0`) and only then return done=true; otherwise return done=false with the same reason and nil error so callers (like RESTConfigFromKubeConfig) never receive an empty kubeconfig byte slice.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/e2e/v2/tests/hosted_cluster_azure_test.go`:
- Around line 54-56: Do not call TestContext.GetHostedClusterClient() before the
HostedCluster kubeconfig is published because GetHostedClusterClient is cached
via sync.Once and will return nil if hc.Status.KubeConfig isn't set; instead,
for specs that need a guest client (where you currently call getTestCtx() and
hostedClusterClient := testCtx.GetHostedClusterClient()), first wait for the
HostedCluster kubeconfig to be published (e.g., poll/wait for
hc.Status.KubeConfig to be non-nil or use the existing wait helper), and only
then invoke TestContext.GetHostedClusterClient(); for the CIDR spec that doesn't
use the client at all, remove the preflight GetHostedClusterClient() call
entirely.
---
Outside diff comments:
In `@test/e2e/util/util.go`:
- Around line 320-324: The predicate that checks secret.Data["kubeconfig"]
should require non-empty bytes before returning success: in the anonymous
Predicate[*corev1.Secret] function (the one that currently does `data, hasData =
secret.Data["kubeconfig"]`), change the condition to ensure data is not nil and
len(data) > 0 (e.g., `hasData && len(data) > 0`) and only then return done=true;
otherwise return done=false with the same reason and nil error so callers (like
RESTConfigFromKubeConfig) never receive an empty kubeconfig byte slice.
🪄 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 YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: ef569464-8941-442e-ab92-348ffd21b768
📒 Files selected for processing (7)
test/e2e/create_cluster_test.gotest/e2e/util/azure.gotest/e2e/util/oauth.gotest/e2e/util/util.gotest/e2e/util/util_ingress_operator_configuration.gotest/e2e/v2/internal/test_context.gotest/e2e/v2/tests/hosted_cluster_azure_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- test/e2e/util/azure.go
bryan-cox
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@bryan-cox: This pull request references CNTRLPLANE-3222 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. |
1439b7b to
fd50296
Compare
jparrill
left a comment
jparrill
left a comment
There was a problem hiding this comment.
Dropped some comments. Thanks!
| Namespace: controlPlaneNamespace, | ||
| Name: "private-router", | ||
| }, | ||
| } |
There was a problem hiding this comment.
Nit: the v1 tests use both groupPredicates (at least 1 CR exists) and per-item predicates (each PLS has the field). Here only groupPredicates are used with nil per-item predicates, which changes the semantics: v2 passes if any PLS has the field, v1 required all. Also loses per-CR diagnostic logging on timeout.
Consider keeping the two-predicate-slice pattern from v1 for stricter validation and better CI failure triage.
There was a problem hiding this comment.
Good catch. Extracted a listPLS helper to deduplicate the 3 closures. On the group vs per-item predicates: EventuallyObjects with group predicates is intentional here — there's only ever one PLS CR per private-router Service, so "any" and "all" are equivalent. Used group predicates because the assertion is about the collection state (at least one exists with the field set), not about validating every individual item.
AI-assisted response via Claude Code
| } | ||
| return items, nil | ||
| }, | ||
| []e2eutil.Predicate[[]*hyperv1.AzurePrivateLinkService]{ |
There was a problem hiding this comment.
Nit: this duplicates support/util.ServicePublishingStrategyByTypeByHC(hc, hyperv1.OAuthServer) which already returns the strategy.
Also, the PLS list getter closure above is copy-pasted 3 times (lines 118-128, 152-162, 186-196) — consider extracting a local listPLS helper.
There was a problem hiding this comment.
Done. Replaced hasOAuthLoadBalancerStrategy with supportutil.ServicePublishingStrategyByTypeByHC(hc, hyperv1.OAuthServer). Also extracted the PLS list closure into a listPLS helper as noted in the other comment.
AI-assisted response via Claude Code
| }) | ||
|
|
||
| It("should have Ingress Operator configuration applied", func() { | ||
| e2eutil.GinkgoAtLeast(e2eutil.Version421) |
There was a problem hiding this comment.
Nit: the four private topology specs are independent assertions — none depends on prior spec state. Ordered prevents parallel execution and causes cascade skips on failure. The other two Context groups in this file use BeforeEach without Ordered. Consider matching that pattern here too.
There was a problem hiding this comment.
The Ordered + BeforeAll pattern is intentional here. The PLS specs have an implicit causal dependency: PLS alias must exist before PE IP can be populated, and PE IP before DNS zone ID. Ordered ensures Ginkgo runs them in definition order and short-circuits on the first failure, which is the right behavior — if PLS alias never appears, waiting 15 minutes each for PE IP and DNS zone provides no signal. The shared testCtx/controlPlaneNamespace variables also rely on BeforeAll which requires Ordered.
AI-assisted response via Claude Code
| } | ||
|
|
||
| func printCollectionStatus[T client.Object](t *testing.T, lastTimestamp time.Time, done bool, reasons map[types.NamespacedName]predicateReasons) { | ||
| func printCollectionStatus[T client.Object](t testing.TB, lastTimestamp time.Time, done bool, reasons map[types.NamespacedName]predicateReasons) { |
There was a problem hiding this comment.
Nit: EventuallyObject, EventuallyObjects, printStatus, and printCollectionStatus were all widened to testing.TB, but EventuallyNotFound (line 525) was missed. Not blocking (no v2 caller today), but worth updating for consistency.
There was a problem hiding this comment.
Done. Widened EventuallyNotFound to testing.TB for consistency with the other functions.
AI-assisted response via Claude Code
fd50296 to
58d5d75
Compare
Migrate self-managed Azure e2e tests from v1 (testing.T/t.Run) to v2 (Ginkgo It blocks) and add them to the shared v2 test binary. This includes public cluster, private topology, and OAuth LoadBalancer tests. Key changes: - Widen testing.T to testing.TB and extract Validate helpers for reuse across v1 and v2 test suites - Add WaitForGuestKubeConfig before GetHostedClusterClient to prevent sync.Once from permanently caching a nil client - Wait for HCP propagation before checking KAS reachability - Write PrintEnvVarHelp output to stderr instead of stdout Signed-off-by: Bryan Cox <brcox@redhat.com> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
58d5d75 to
dfa0ec5
Compare
|
/lgtm |
|
Scheduling tests matching the |
|
/verified by E2E |
openshift-ci-robot
added
the
verified
|
@jparrill: This PR has been marked as verified by 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. |
|
/retest |
|
@bryan-cox: all tests passed! 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. |
3 participants
Summary
*testing.T,t.Run()) to v2 (Ginkgo v2It()blocks) so each test case produces a separate JUnit XML entry for individual Sippy reportingValidate*helpers fromEnsure*wrappers acceptingtesting.TBfor cross-framework compatibilityEventuallyObject/EventuallyObjectsto accepttesting.TB(backward-compatible since*testing.Timplementstesting.TB)GetGuestClient()/SetupGuestClient()toTestContextfor guest cluster validationNew test specs (9 total across 3 label groups)
self-managed-azure-public(3 specs):self-managed-azure-private(4 specs,Ordered):self-managed-azure-oauth-lb(2 specs):CI workflow (openshift/release PR to follow)
The test binary is invoked 3 times with
--ginkgo.label-filteragainst different guest clusters (public, private, OAuth LB), each producing its own JUnit XML.Test plan
make e2e— all test binaries compile includingbin/test-e2e-self-managed-azurebin/test-e2e-self-managed-azure --ginkgo.dry-run --ginkgo.v— lists 9 specs--ginkgo.label-filter="self-managed-azure-public"runs 3 specs, skips 6E2E_SHOW_ENV_HELP=1prints Azure-specific env varsgo build -tags e2e ./test/e2e/...— v1 tests still compile (backward compatible)Ref: CNTRLPLANE-3222
🤖 Generated with Claude Code
Summary by CodeRabbit