Enable external platform support for s390x#10424
Open
Neeraj8418 wants to merge 2 commits into
Open
Conversation
This change allows the external platform type to be used with s390x and ppc64le architectures, which previously only supported the 'none' platform. Changes: - Updated GetActualCreateClusterPlatformParams to allow external platform for s390x and ppc64le in addition to the existing 'none' platform - Added logic to preserve external platform configuration when specified - User-managed networking remains mandatory for these architectures Fixes the error: 'Can't set external platform on s390x architecture' Signed-off-by: Neeraj Mishra <neeraj.mishra3@ibm.com>
WalkthroughThe change refines CPU-architecture-specific platform validation in ChangesArchitecture-specific platform validation
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes 🚥 Pre-merge checks | ✅ 13 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (13 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
openshift-ci
Bot
added
the
size/XS
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Neeraj8418 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 |
![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 `@internal/provider/platform.go`:
- Around line 313-317: Add table-driven unit tests that directly call
GetActualCreateClusterPlatformParams to exercise the cpuArchitecture == "s390x"
and "ppc64le" branches: for each arch, include cases where (1) platform is
external and userManagedNetworking (UMN) is true — expect success, (2) UMN is
false with external platform — expect rejection/error, and (3) platform is a
non-none/non-external type — expect rejection/error. Use the same platform
helper constructors used in production (e.g., createPlatformFromType and
common.IsPlatformExternal behavior) and assert returned platform, boolean flag,
and error matches expectations; reference the function name
GetActualCreateClusterPlatformParams, the cpuArchitecture parameter, and
models.PlatformTypeNone in your tests so the coverage specifically targets the
rejected/accepted logic for s390x/ppc64le.
🪄 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: b6baca35-9d26-43f5-950c-fd23e3885cf6
📒 Files selected for processing (1)
internal/provider/platform.go
Comment on lines
+313
to
317
| // If platform is external, keep it; otherwise default to none | ||
| if platform != nil && common.IsPlatformExternal(platform) { | ||
| return platform, swag.Bool(true), nil | ||
| } | ||
| return createPlatformFromType(models.PlatformTypeNone), swag.Bool(true), nil |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find existing tests for GetActualCreateClusterPlatformParams and any s390x/ppc64le external coverage
rg -nP --type=go -C3 'GetActualCreateClusterPlatformParams' -g '*_test.go'
rg -nP --type=go -C3 '(S390x|Ppc64le).*([Ee]xternal)|([Ee]xternal).*(S390x|Ppc64le)' -g '*_test.go'Repository: openshift/assisted-service
Length of output: 52
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Locate platform external handling and the broader rejection logic
rg -n "IsPlatformExternal" -S --glob='*.go' .
rg -n "userManagedNetworking|UMN|ManagedNetworking" -S --glob='*.go' .
rg -n "ppc64le|s390x" -S --glob='*.go' .
rg -n "GetActualCreateClusterPlatformParams|create cluster platform|PlatformTypeExternal|PlatformTypeNone" -S --glob='*_test.go' .
# 2) Focus tests likely touching internal/provider/platform.go
rg -n "internal/provider/platform.go|platform\.go" -S --glob='*_test.go' .
rg -n "createPlatformFromType|PlatformTypeNone" -S --glob='*_test.go' .
# 3) List tests mentioning s390x/ppc64le explicitly
rg -n "ppc64le|s390x" -S --glob='*_test.go' .Repository: openshift/assisted-service
Length of output: 50383
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Inspect the exact create params logic around the requested line range
echo "---- internal/provider/platform.go: GetActualCreateClusterPlatformParams ----"
sed -n '260,360p' internal/provider/platform.go | nl -ba | sed -n '1,120p'
# 2) Find s390x/ppc64le occurrences in tests only (small, architecture-scoped)
echo "---- s390x occurrences in *_test.go ----"
rg -n --glob='*_test.go' -S "s390x" . | head -n 50
echo "---- ppc64le occurrences in *_test.go ----"
rg -n --glob='*_test.go' -S "ppc64le" . | head -n 50
# 3) Find External platform tests and check if they mention cpuArchitecture/arch
echo "---- External platform + architecture mentions in *_test.go ----"
rg -n --glob='*_test.go' -S "(External platform|PlatformTypeExternal|platform_is_external|IsPlatformExternal)" . | head -n 50
echo "---- External platform test file: agentclusterinstall_validation_hook_test.go (spot check around External cases) ----"
rg -n --glob='agentclusterinstall_validation_hook_test.go' -S "External platform" pkg/webhooks/hiveextension/v1beta1/agentclusterinstall_validation_hook_test.go -nRepository: openshift/assisted-service
Length of output: 209
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Inspect the exact create params logic around the requested line range
echo "---- internal/provider/platform.go: GetActualCreateClusterPlatformParams ----"
sed -n '260,360p' internal/provider/platform.go | nl -ba | sed -n '1,120p'
# 2) Find s390x/ppc64le occurrences in tests only
echo "---- s390x occurrences in *_test.go ----"
rg -n --glob='*_test.go' -S "s390x" . | head -n 50
echo "---- ppc64le occurrences in *_test.go ----"
rg -n --glob='*_test.go' -S "ppc64le" . | head -n 50
# 3) Find External platform tests (and quickly spot-check)
echo "---- External platform / PlatformTypeExternal occurrences in *_test.go ----"
rg -n --glob='*_test.go' -S "(External platform|PlatformTypeExternal|IsPlatformExternal|platform_is_external)" . | head -n 80
# 4) Check whether the webhook tests pass cpuArchitecture through (relevant to the s390x/ppc64le branch)
echo "---- cpuArchitecture mentions in hiveextension v1beta1 tests ----"
rg -n -S --glob='*_test.go' "cpuArchitecture" pkg/webhooks/hiveextension/v1beta1 . | head -n 50Repository: openshift/assisted-service
Length of output: 209
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "---- internal/provider/platform.go: GetActualCreateClusterPlatformParams ----"
# show with line numbers via cat -n
sed -n '260,360p' internal/provider/platform.go | cat -n | sed -n '1,120p'
echo "---- s390x occurrences in *_test.go (head 50) ----"
rg -n --glob='*_test.go' -S "s390x" . | head -n 50
echo "---- ppc64le occurrences in *_test.go (head 50) ----"
rg -n --glob='*_test.go' -S "ppc64le" . | head -n 50
echo "---- External platform / PlatformTypeExternal / IsPlatformExternal occurrences in *_test.go (head 80) ----"
rg -n --glob='*_test.go' -S "(External platform|PlatformTypeExternal|IsPlatformExternal|platform_is_external)" . | head -n 80
echo "---- cpuArchitecture mentions in hiveextension v1beta1 tests (head 50) ----"
rg -n -S --glob='*_test.go' "cpuArchitecture" pkg/webhooks/hiveextension/v1beta1 . | head -n 50Repository: openshift/assisted-service
Length of output: 38281
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "---- usages of GetActualCreateClusterPlatformParams in repo (tests + prod) ----"
rg -n "GetActualCreateClusterPlatformParams\\(" . --glob='*.go' | head -n 50
echo "---- look for exact error strings in *_test.go ----"
rg -n --glob='*_test.go' "Can't disable User Managed Networking on" . || true
rg -n --glob='*_test.go' "Can't set .* platform on .* architecture" . || true
echo "---- inspect hiveextension validation hook tests for cpuArchitecture in External platform cases ----"
rg -n --glob='agentclusterinstall_validation_hook_test.go' "External platform" pkg/webhooks/hiveextension/v1beta1/agentclusterinstall_validation_hook_test.go
rg -n --glob='agentclusterinstall_validation_hook_test.go' "CPUArchitecture|cpuArchitecture" pkg/webhooks/hiveextension/v1beta1/agentclusterinstall_validation_hook_test.goRepository: openshift/assisted-service
Length of output: 2276
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="pkg/webhooks/hiveextension/v1beta1/agentclusterinstall_validation_hook_test.go"
# Inspect the External-platform create/update test cases to see what cpuArchitecture/CPUArchitecture is used
sed -n '620,820p' "$FILE" | cat -n
sed -n '820,900p' "$FILE" | cat -n
# Search within the same file for s390x/ppc64le mentions
rg -n "s390x|ppc64le|CPUArchitecture|cpuArchitecture" "$FILE" | head -n 200
# Look for tests combining external platform with s390x/ppc64le elsewhere
rg -n --glob='*_test.go' -S "(PlatformTypeExternal|External platform).*?(s390x|ppc64le)|(s390x|ppc64le).{0,120}?(PlatformTypeExternal|External platform)" . | head -n 200Repository: openshift/assisted-service
Length of output: 11710
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="pkg/webhooks/hiveextension/v1beta1/agentclusterinstall_validation_hook_test.go"
echo "---- CPUArchitecture/cpuArchitecture mentions in $FILE ----"
rg -n --glob='*.go' -S "CPUArchitecture|cpuArchitecture" "$FILE" || true
rg -n --glob='*.go' -S "s390x|ppc64le|PlatformTypeExternal|ExternalPlatformType" "$FILE" | head -n 200 || true
echo "---- look for s390x/ppc64le + External platform combinations in *_test.go ----"
rg -n --glob='*_test.go' -S "(PlatformTypeExternal|External platform).{0,200}(s390x|ppc64le)|(s390x|ppc64le).{0,200}(PlatformTypeExternal|External platform)" . | head -n 200
echo "---- search for exact architecture-branch error strings in *_test.go ----"
rg -n --glob='*_test.go' -S "Can't disable User Managed Networking on|Can't set .* platform on .* architecture" . || true
echo "---- inspect provider call site in agentclusterinstall_validation_hook.go ----"
sed -n '330,430p' pkg/webhooks/hiveextension/v1beta1/agentclusterinstall_validation_hook.go | cat -nRepository: openshift/assisted-service
Length of output: 5524
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="pkg/webhooks/hiveextension/v1beta1/agentclusterinstall_validation_hook_test.go"
echo "---- CPUArchitecture/cpuArchitecture mentions in $FILE ----"
rg -n -S "CPUArchitecture|cpuArchitecture" "$FILE" || true
rg -n -S "s390x|ppc64le|PlatformTypeExternal|ExternalPlatformType" "$FILE" | head -n 200 || true
echo "---- look for s390x/ppc64le + External platform combinations in *_test.go ----"
rg -n --glob='*_test.go' -S "(PlatformTypeExternal|External platform).{0,200}(s390x|ppc64le)|(s390x|ppc64le).{0,200}(PlatformTypeExternal|External platform)" . | head -n 200
echo "---- search for exact architecture-branch error strings in *_test.go ----"
rg -n --glob='*_test.go' -S "Can't disable User Managed Networking on|Can't set .* platform on .* architecture" . || true
echo "---- inspect provider call site in agentclusterinstall_validation_hook.go ----"
sed -n '330,430p' pkg/webhooks/hiveextension/v1beta1/agentclusterinstall_validation_hook.go | cat -nRepository: openshift/assisted-service
Length of output: 5524
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search unit tests for direct calls
rg -n --glob='*_test.go' "GetActualCreateClusterPlatformParams\\(" . || true
# Search for the s390x/ppc64le-specific error messages
rg -n --glob='*_test.go' "Can't disable User Managed Networking on|Can't set .* platform on .* architecture|User Managed Networking on s390x|User Managed Networking on ppc64le" . || trueRepository: openshift/assisted-service
Length of output: 52
Add unit tests for the s390x/ppc64le external-platform handling in GetActualCreateClusterPlatformParams
- The
cpuArchitecture == s390x/ppc64lebranch (UMN=false rejection, non-none/non-external rejection, external accepted) isn’t exercised by existing webhook unit tests becausepkg/webhooks/hiveextension/v1beta1/agentclusterinstall_validation_hook.gopassescpuArchitectureas"", and there are no tests callingGetActualCreateClusterPlatformParamsdirectly. - Add table-driven tests covering, for both s390x and ppc64le: external+UMN=true allowed; UMN=false rejected; non-none/non-external rejected.
🤖 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 `@internal/provider/platform.go` around lines 313 - 317, Add table-driven unit
tests that directly call GetActualCreateClusterPlatformParams to exercise the
cpuArchitecture == "s390x" and "ppc64le" branches: for each arch, include cases
where (1) platform is external and userManagedNetworking (UMN) is true — expect
success, (2) UMN is false with external platform — expect rejection/error, and
(3) platform is a non-none/non-external type — expect rejection/error. Use the
same platform helper constructors used in production (e.g.,
createPlatformFromType and common.IsPlatformExternal behavior) and assert
returned platform, boolean flag, and error matches expectations; reference the
function name GetActualCreateClusterPlatformParams, the cpuArchitecture
parameter, and models.PlatformTypeNone in your tests so the coverage
specifically targets the rejected/accepted logic for s390x/ppc64le.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #10424 +/- ##
==========================================
- Coverage 44.31% 44.30% -0.01%
==========================================
Files 418 418
Lines 72844 72846 +2
==========================================
- Hits 32280 32277 -3
- Misses 37639 37643 +4
- Partials 2925 2926 +1
🚀 New features to boost your workflow:
|
|
@Neeraj8418: The following test 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. |
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
1 participant
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.
Summary
Enable external platform support for s390x architectures in assisted-service.
This change removes the restriction that prevented s390x from using
platform: external, allowing these architectures to support custom platform integrations beyond the basicplatform: noneconfiguration.Changes
GetActualCreateClusterPlatformParams()ininternal/provider/platform.goto accept external platform for s390x architecturesProblem Solved
Fixes runtime error:
Can't set external platform on s390x architectureBackground: Previously, s390x architectures were hardcoded to only accept
platform: none. This limitation prevented users from:Solution: This PR enables
platform: externalfor s390x while preserving all existing validation and networking requirements, providing feature parity with other supported architectures.List all the issues related to this PR
What environments does this code impact?
How was this code tested?
Manual Testing Details
Test Environment: s390x LPAR (a314lp41)
Test Steps: