fix(ho): make e2e HostedCluster interactions platform-aware#7724
fix(ho): make e2e HostedCluster interactions platform-aware#7724devguyio wants to merge 2 commits intoopenshift:mainfrom
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
|
@devguyio: This pull request explicitly references no jira issue. 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. |
|
Skipping CI for Draft Pull Request. |
WalkthroughThe changes introduce platform-specific test configuration handling in e2e tests by adding a per-test-case platformType field to override global platform selection, while centralizing node count calculations through a new helper function that considers platform and zone configurations. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~28 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 APPROVED This pull-request has been approved by: devguyio 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 |
7264156 to
4c3c5d3
Compare
|
/test e2e-aks |
4c3c5d3 to
e68bdff
Compare
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
Add a platformType field to each test case group so platform intent is explicit rather than overriding all cases with globalOpts.Platform. GCP and Azure groups set their platformType; platform-agnostic groups fall back to globalOpts.Platform. Remove redundant Platform.Type assignments from individual mutateInput functions. Add a nil guard for Platform.AWS in the metrics collector to prevent nil dereference on non-AWS environments. Generated-with: Claude Code Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com>
Add numExpectedNodes helper that computes node count based on platform instead of always multiplying by AWSPlatform.Zones. Guard EndpointAccess routing and multi-zone zone override with AWS platform checks. Fix pre-existing staticcheck nil-pointer warning in sts_test.go. Generated with Claude Code (Opus 4.6) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com>
e68bdff to
af0e870
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/e2e/create_cluster_test.go (1)
322-366: RedundantPlatform.Typeassignments inmutateInputfunctions.These GCP test groups correctly set
platformType: hyperv1.GCPPlatformat the test case level. However, the individualmutateInputfunctions (e.g., lines 334, 344, 354, 375, etc.) still explicitly sethc.Spec.Platform.Type = hyperv1.GCPPlatform.Since the test loop at lines 1638-1642 already sets
hostedCluster.Spec.Platform.Typefromtc.platformTypebefore callingmutateInput, these assignments are redundant. Consider removing them to align with the PR's stated goal of removing redundantPlatform.Typeassignments frommutateInputfunctions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/create_cluster_test.go` around lines 322 - 366, Remove redundant assignments to hc.Spec.Platform.Type inside the various mutateInput closures (e.g., the functions in the GCP validation test cases) because the test harness already sets hostedCluster.Spec.Platform.Type from tc.platformType before invoking mutateInput; locate the mutateInput lambdas that set hc.Spec.Platform.Type = hyperv1.GCPPlatform (in the GCP test group) and delete those lines so the closures only mutate the platform-specific fields (like hc.Spec.Platform.GCP) without reassigning Platform.Type.
🤖 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/create_cluster_test.go`:
- Around line 322-366: Remove redundant assignments to hc.Spec.Platform.Type
inside the various mutateInput closures (e.g., the functions in the GCP
validation test cases) because the test harness already sets
hostedCluster.Spec.Platform.Type from tc.platformType before invoking
mutateInput; locate the mutateInput lambdas that set hc.Spec.Platform.Type =
hyperv1.GCPPlatform (in the GCP test group) and delete those lines so the
closures only mutate the platform-specific fields (like hc.Spec.Platform.GCP)
without reassigning Platform.Type.
ℹ️ Review info
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (4)
cmd/infra/aws/util/sts_test.gotest/e2e/create_cluster_test.gotest/e2e/util/hypershift_framework.gotest/e2e/util/util.go
|
/auto-cc |
|
/lgtm |
|
Scheduling tests matching the |
|
/verified by e2e |
|
@bryan-cox: 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. |
|
@devguyio: 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. |
What this PR does / why we need it:
Removes hardcoded AWS platform assumptions from e2e tests so they can run correctly on non-AWS platforms (GCP, Azure, etc.).
platformTypefield to each test case group inTestOnCreateAPIUXso platform intent is explicit. GCP and Azure groups declare their platform; platform-agnostic groups fall back toglobalOpts.Platform.Platform.Typeassignments from individualmutateInputfunctions.tc.platformTypeinstead ofhostedCluster.Spec.Platform.Type, preventing platform-agnostic tests from being incorrectly skipped on GCP environments.numExpectedNodeshelper that computes node count based on platform instead of always multiplying byAWSPlatform.Zones.Platform.AWSin the metrics collector to prevent nil pointer dereference on non-AWS hosted clusters.sts_test.go.Why:
When running e2e tests on non-AWS environments (e.g. e2e-aks), the test framework was creating HostedCluster objects with
platform.type: AWShardcoded in the YAML template. The HO would then receive these HostedClusters with an AWS platform type but noPlatform.AWSspec populated. This triggered unexpected code paths in the metrics collector, which assumedPlatform.AWSwas non-nil wheneverPlatform.Type == AWS, causing a nil pointer dereference. Additionally, the test harness unconditionally multiplied node counts byAWSPlatform.Zonesand checkedAWSPlatform.EndpointAccessregardless of the actual platform, leading to incorrect node expectations and wrong cluster validation paths on non-AWS runs.Which issue(s) this PR fixes:
Fixes
Special notes for your reviewer:
The
hostedcluster-base.yamlasset still hastype: AWS— this is intentional since our loop always overrides it, and changing the asset would affecttest/e2e/v2/tests/api_ux_validation_test.gowhich is out of scope.## Checklist:Summary by CodeRabbit
Release Notes