Add aws-us-east-1 cluster profile#5191
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughRelocates cluster-profile API from ChangesClusterProfile API and refactoring
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 12 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (12 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Comment |
openshift-ci
Bot
added
the
approved
![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
🤖 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/api/clusterprofile.go`:
- Around line 1202-1208: The method ClusterProfilesList.Resolve should guard
against a nil receiver to avoid panics when called on a nil
*ClusterProfilesList; add an early nil check at the start of Resolve (e.g., if
cpl == nil) and return an appropriate error or nil (consistent with the
function's contract) before any dereference (such as accessing cpl.KonfluxConfig
or modifying errs/clusterGroups) so all subsequent code in Resolve is safe;
update any callers/tests if they rely on panics.
- Around line 1162-1182: LeaseTypeFromClusterType rejects cluster names that
ClusterType() can produce; update LeaseTypeFromClusterType to accept the same
set (including aliases like alibabacloud, ibmcloud, aro, oci-edge and the
additional openstack/metal variants emitted by ClusterType()) by adding those
missing case strings or normalizing ClusterType() outputs to the canonical names
before the switch; modify the switch in LeaseTypeFromClusterType to include the
missing identifiers (or add a small mapping/normalization step) so valid
profiles no longer return "invalid cluster type", and add unit tests covering
the ClusterType() -> LeaseTypeFromClusterType path to prevent regressions.
🪄 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: Enterprise
Run ID: 555b9ad8-81c7-49c6-b54b-9cf95251fb4e
📒 Files selected for processing (2)
pkg/api/clusterprofile.gopkg/api/types.go
💤 Files with no reviewable changes (1)
- pkg/api/types.go
| func LeaseTypeFromClusterType(t string) (string, error) { | ||
| switch t { | ||
| case | ||
| "aws", "aws-c2s", "aws-china", "aws-usgov", "aws-sc2s", "aws-eusc", "aws-osd-msp", "aws-opendatahub", | ||
| "alibaba", "azure-2", "azure4", "azure-arm64", "azurestack", "azuremag", "equinix-ocp-metal", | ||
| "gcp", "gcp-arm64", "gcp-opendatahub", "libvirt-ppc64le", "libvirt-ppc64le-s2s", "libvirt-s390x", | ||
| "libvirt-s390x-1", "libvirt-s390x-2", "libvirt-s390x-amd64", "libvirt-s390x-vpn", "ibmcloud-multi-ppc64le", | ||
| "ibmcloud-multi-s390x", "nutanix", "nutanix-qe", "nutanix-qe-dis", "nutanix-qe-zone", "nutanix-qe-gpu", | ||
| "nutanix-qe-flow", "openstack", "openstack-osuosl", "openstack-vexxhost", "openstack-ppc64le", | ||
| "openstack-nerc-dev", "vsphere", "ovirt", "packet", "packet-edge", "powervc-1", "powervs-multi-1", | ||
| "powervs-1", "powervs-2", "powervs-3", "powervs-4", "powervs-5", "powervs-6", "powervs-7", "powervs-8", "powervs-9", | ||
| "kubevirt", "aws-cpaas", "osd-ephemeral", "gcp-virtualization", "aws-virtualization", | ||
| "azure-virtualization", "hypershift-aws", "hypershift-aks", "hypershift-azure", | ||
| "hypershift-powervs", "hypershift-powervs-cb", "hypershift-gcp", "aws-mco-qe", | ||
| "equinix-edge-enablement", "aws-oadp-qe", "azure-oadp-qe", "aws-lp-chaos", "aws-osp-qe", | ||
| "metal-redhat-gs", "aro-hcp-int", "aro-hcp-stg", "aro-hcp-prod", "aro-hcp-dev", "rosa-regional-platform-int", "hyperfleet-e2e", | ||
| "aro-classic-int", "aro-classic-stg", "aro-classic-prod", "aro-classic-dev", "rosa-e2e-01", "rosa-e2e-02", "rosa-e2e-03": | ||
| return t + "-quota-slice", nil | ||
| default: | ||
| return "", fmt.Errorf("invalid cluster type %q", t) | ||
| } |
There was a problem hiding this comment.
LeaseTypeFromClusterType() is out of sync with ClusterType() outputs.
This function rejects valid cluster types produced by ClusterType() (for example alibabacloud, ibmcloud, aro, oci-edge, and several openstack/metal variants), which can trigger invalid cluster type on valid profiles.
Suggested direction
func LeaseTypeFromClusterType(t string) (string, error) {
switch t {
case
"aws", "aws-c2s", "aws-china", "aws-usgov", "aws-sc2s", "aws-eusc", "aws-osd-msp", "aws-opendatahub",
- "alibaba", "azure-2", "azure4", "azure-arm64", "azurestack", "azuremag", "equinix-ocp-metal",
+ "alibabacloud", "azure-2", "azure4", "azure-arm64", "azurestack", "azuremag", "equinix-ocp-metal",
"gcp", "gcp-arm64", "gcp-opendatahub", "libvirt-ppc64le", "libvirt-ppc64le-s2s", "libvirt-s390x",
"libvirt-s390x-1", "libvirt-s390x-2", "libvirt-s390x-amd64", "libvirt-s390x-vpn", "ibmcloud-multi-ppc64le",
- "ibmcloud-multi-s390x", "nutanix", ...
+ "ibmcloud-multi-s390x", "ibmcloud", "nutanix", ...
+ "openstack-hwoffload", "openstack-ibm-osp", "openstack-nfv", "openstack-vh-mecha-central",
+ "openstack-vh-bm-rhos", "openstack-operators-vexxhost", "openstack-rhoso", "openstack-rhos-ci",
+ "metal-perfscale-cpt", "metal-perfscale-jetlag", "metal-perfscale-osp", "metal-perfscale-selfsched", "metal-perfscale-telco",
+ "oci-agent-qe", "oci-edge", "aro":
return t + "-quota-slice", nil
default:
return "", fmt.Errorf("invalid cluster type %q", t)
}
}🤖 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/api/clusterprofile.go` around lines 1162 - 1182, LeaseTypeFromClusterType
rejects cluster names that ClusterType() can produce; update
LeaseTypeFromClusterType to accept the same set (including aliases like
alibabacloud, ibmcloud, aro, oci-edge and the additional openstack/metal
variants emitted by ClusterType()) by adding those missing case strings or
normalizing ClusterType() outputs to the canonical names before the switch;
modify the switch in LeaseTypeFromClusterType to include the missing identifiers
(or add a small mapping/normalization step) so valid profiles no longer return
"invalid cluster type", and add unit tests covering the ClusterType() ->
LeaseTypeFromClusterType path to prevent regressions.
There was a problem hiding this comment.
This is a refactor and, if this is a real issue, I don't want to address it in this PR.
There was a problem hiding this comment.
@danilo-gemoli Understood. Would you like me to open a follow-up issue to track the LeaseTypeFromClusterType / ClusterType() sync gap so it doesn't get lost?
| func (cpl *ClusterProfilesList) Resolve() error { | ||
| errs := make([]error, 0) | ||
|
|
||
| clusterGroups := make(map[string][]string) | ||
| if cpl.KonfluxConfig != nil { | ||
| clusterGroups = cpl.KonfluxConfig.ClusterGroups | ||
| } |
There was a problem hiding this comment.
Guard nil receiver in Resolve() to prevent panic.
Line 1206 dereferences cpl unconditionally. A nil receiver call will panic.
Proposed fix
func (cpl *ClusterProfilesList) Resolve() error {
+ if cpl == nil {
+ return nil
+ }
+
errs := make([]error, 0)
clusterGroups := make(map[string][]string)
if cpl.KonfluxConfig != nil {
clusterGroups = cpl.KonfluxConfig.ClusterGroups
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (cpl *ClusterProfilesList) Resolve() error { | |
| errs := make([]error, 0) | |
| clusterGroups := make(map[string][]string) | |
| if cpl.KonfluxConfig != nil { | |
| clusterGroups = cpl.KonfluxConfig.ClusterGroups | |
| } | |
| func (cpl *ClusterProfilesList) Resolve() error { | |
| if cpl == nil { | |
| return nil | |
| } | |
| errs := make([]error, 0) | |
| clusterGroups := make(map[string][]string) | |
| if cpl.KonfluxConfig != nil { | |
| clusterGroups = cpl.KonfluxConfig.ClusterGroups | |
| } |
🤖 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/api/clusterprofile.go` around lines 1202 - 1208, The method
ClusterProfilesList.Resolve should guard against a nil receiver to avoid panics
when called on a nil *ClusterProfilesList; add an early nil check at the start
of Resolve (e.g., if cpl == nil) and return an appropriate error or nil
(consistent with the function's contract) before any dereference (such as
accessing cpl.KonfluxConfig or modifying errs/clusterGroups) so all subsequent
code in Resolve is safe; update any callers/tests if they rely on panics.
There was a problem hiding this comment.
Same as above, this is a refactor and I don't want to address these things here.
There was a problem hiding this comment.
@danilo-gemoli Understood, this is purely a refactor. Would you like me to open a follow-up issue to track adding the nil guard to (*ClusterProfilesList).Resolve()?
danilo-gemoli
force-pushed
the
chore/add-aws-us-east-1-profile
branch
from
b486b15 to
bdbb1a8
Compare
|
/test e2e |
|
/retest-required |
|
/test e2e |
|
/override ci/prow/images |
|
@danilo-gemoli: Overrode contexts on behalf of danilo-gemoli: ci/prow/images 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 kubernetes-sigs/prow repository. |
|
Tests from second stage were triggered manually. Pipeline can be controlled only manually, until HEAD changes. Use command to trigger second stage. |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: danilo-gemoli, deepsm007 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 |
|
@danilo-gemoli: 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. |
2 participants
and move the cluster profile related types from
pkg/api/types.gotopkg/api/clusterprofile.go.I think cluster profile are important enough to live into its own file.
Summary
This PR adds support for an aws-us-east-1 cluster profile and refactors cluster-profile API surface out of pkg/api/types.go into a dedicated file pkg/api/clusterprofile.go.
Changes
Affected components and behavior
Practical impact for CI users and operators