Introduce Naive Cluster Profile Sets#4983
Merged
openshift-merge-bot[bot] merged 4 commits intoopenshift:mainfrom Mar 6, 2026
Merged
Introduce Naive Cluster Profile Sets#4983openshift-merge-bot[bot] merged 4 commits intoopenshift:mainfrom
openshift-merge-bot[bot] merged 4 commits intoopenshift:mainfrom
Conversation
Contributor
|
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: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughRemoved initializer secret creation and getClusterProfileSecret; introduced ClusterProfileGetter wiring from resolver into defaults.Config; moved cluster-profile lookup and secret import into LeaseStep during lease acquisition; added cluster-profile constants, StepLease fields, updated signatures and tests. Changes
Sequence Diagram(s)sequenceDiagram
participant Config as Configuration
participant LeaseStep as Lease Step
participant ProfileGetter as ClusterProfileGetter
participant KubeClient as Kubernetes Client
participant KubeAPI as Kubernetes API
Config->>LeaseStep: Initialize with kubeClient & ClusterProfileGetter
LeaseStep->>LeaseStep: Acquire leases for test
LeaseStep->>LeaseStep: Extract cluster profile name from leased resources
alt Cluster profile present
LeaseStep->>ProfileGetter: Request ClusterProfileDetails(name)
ProfileGetter-->>LeaseStep: Return ClusterProfileDetails
LeaseStep->>KubeClient: Get secret from "ci" namespace
KubeClient->>KubeAPI: Read Secret
KubeAPI-->>KubeClient: Secret data
LeaseStep->>KubeClient: Create/Upsert immutable secret in target namespace
KubeClient->>KubeAPI: Create/Upsert Secret
KubeAPI-->>KubeClient: Acknowledge
end
LeaseStep->>LeaseStep: Record metrics and provide cluster-profile env vars
LeaseStep-->>Config: Continue pipeline with cluster-profile state
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 |
Contributor
Author
|
/test e2e |
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: 3
🧹 Nitpick comments (1)
pkg/defaults/defaults_test.go (1)
1715-1718: Prefer constants in expected param map.Line 1715 and Line 1716 can use
api.ClusterProfileParamandapi.ClusterProfileSetEnvto avoid future string drift in tests.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/defaults/defaults_test.go` around lines 1715 - 1718, The expected params map is using raw strings "CLUSTER_PROFILE" and "CLUSTER_PROFILE_SET_NAME"; replace those literal keys with the constants api.ClusterProfileParam and api.ClusterProfileSetEnv respectively in the test (the map that also includes api.DefaultLeaseEnv) so the test references stable identifiers and avoids string drift—update the map entries where those two keys appear and run tests to verify no import changes are needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/api/leases.go`:
- Around line 11-13: LeasesForTest currently dereferences test and
test.MultiStageTestConfigurationLiteral (e.g., reading
MultiStageTestConfigurationLiteral.ClusterProfile) and can panic if either is
nil; add defensive nil checks at the top of LeasesForTest (function
LeasesForTest, type TestStepConfiguration and its field
MultiStageTestConfigurationLiteral) that return an empty []StepLease when test
== nil or test.MultiStageTestConfigurationLiteral == nil before accessing
ClusterProfile or other fields, then proceed as before.
In `@pkg/steps/lease_test.go`:
- Around line 454-458: Fix the typo in the test error string: update the
t.Errorf call that currently says "failed to resove provides param %s: %s" (in
the block that populates gotProvides and checks err) to use "resolve" instead of
"resove" so the message reads "failed to resolve provides param %s: %s"; keep
the same formatting and variables (k, err) in the t.Errorf invocation.
In `@pkg/steps/multi_stage/multi_stage_test.go`:
- Around line 401-403: The comparator passed to cmpopts.SortSlices is wrong:
change the anonymous function signature from func(a, b string) bool to func(a, b
coreapi.EnvVar) bool and implement a strict (irreflexive) ordering using < not
<=; for example, compare primary key fields such as a.Name < b.Name and fall
back to a.Value < b.Value (or other deterministic fields) to ensure a total,
strict order for []coreapi.EnvVar before calling cmp.Diff with
cmpopts.SortSlices.
---
Nitpick comments:
In `@pkg/defaults/defaults_test.go`:
- Around line 1715-1718: The expected params map is using raw strings
"CLUSTER_PROFILE" and "CLUSTER_PROFILE_SET_NAME"; replace those literal keys
with the constants api.ClusterProfileParam and api.ClusterProfileSetEnv
respectively in the test (the map that also includes api.DefaultLeaseEnv) so the
test references stable identifiers and avoids string drift—update the map
entries where those two keys appear and run tests to verify no import changes
are needed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1f7092df-c322-4032-b65d-0510e6d22a89
📒 Files selected for processing (13)
cmd/ci-operator/main.gopkg/api/constant.gopkg/api/leases.gopkg/api/leases_test.gopkg/api/types.gopkg/defaults/config.gopkg/defaults/defaults.gopkg/defaults/defaults_test.gopkg/prowgen/prowgen.gopkg/steps/lease.gopkg/steps/lease_test.gopkg/steps/multi_stage/multi_stage.gopkg/steps/multi_stage/multi_stage_test.go
Comment on lines
+11
to
+13
| func LeasesForTest(test *TestStepConfiguration) (ret []StepLease) { | ||
| multiStageTest := test.MultiStageTestConfigurationLiteral | ||
| if p := multiStageTest.ClusterProfile; p != "" { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# First, let's look at the actual function implementation
echo "=== Function implementation ==="
cat -n pkg/api/leases.go | head -30
echo ""
echo "=== All call sites of LeasesForTest ==="
rg -nP --type=go 'LeasesForTest\s*\('Repository: openshift/ci-tools
Length of output: 1563
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Call site in prowgen.go ==="
sed -n '230,240p' pkg/prowgen/prowgen.go
echo ""
echo "=== Call site in defaults.go ==="
sed -n '395,405p' pkg/defaults/defaults.go
echo ""
echo "=== Test context ==="
sed -n '45,55p' pkg/api/leases_test.go
echo ""
echo "=== Full context around prowgen.go call ==="
sed -n '220,245p' pkg/prowgen/prowgen.goRepository: openshift/ci-tools
Length of output: 2107
Add defensive nil guards in LeasesForTest.
Lines 12 and 13 can panic if test or test.MultiStageTestConfigurationLiteral is nil. The function comment states "It is assumed that they have been validated," yet the function itself has no guards. While current call sites in prowgen.go and defaults.go check MultiStageTestConfigurationLiteral before calling, the function should be defensive to prevent panics from new callers.
💡 Proposed fix
func LeasesForTest(test *TestStepConfiguration) (ret []StepLease) {
+ if test == nil || test.MultiStageTestConfigurationLiteral == nil {
+ return nil
+ }
multiStageTest := test.MultiStageTestConfigurationLiteral
if p := multiStageTest.ClusterProfile; p != "" {📝 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.
Suggested change
| func LeasesForTest(test *TestStepConfiguration) (ret []StepLease) { | |
| multiStageTest := test.MultiStageTestConfigurationLiteral | |
| if p := multiStageTest.ClusterProfile; p != "" { | |
| func LeasesForTest(test *TestStepConfiguration) (ret []StepLease) { | |
| if test == nil || test.MultiStageTestConfigurationLiteral == nil { | |
| return nil | |
| } | |
| multiStageTest := test.MultiStageTestConfigurationLiteral | |
| if p := multiStageTest.ClusterProfile; p != "" { |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/api/leases.go` around lines 11 - 13, LeasesForTest currently dereferences
test and test.MultiStageTestConfigurationLiteral (e.g., reading
MultiStageTestConfigurationLiteral.ClusterProfile) and can panic if either is
nil; add defensive nil checks at the top of LeasesForTest (function
LeasesForTest, type TestStepConfiguration and its field
MultiStageTestConfigurationLiteral) that return an empty []StepLease when test
== nil or test.MultiStageTestConfigurationLiteral == nil before accessing
ClusterProfile or other fields, then proceed as before.
danilo-gemoli
force-pushed
the
feat/ci-operator/cluster-profile-sets
branch
from
8985d58 to
e2447fd
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
pkg/api/leases.go (1)
11-13:⚠️ Potential issue | 🟡 MinorAdd defensive nil guards in
LeasesForTestto prevent panic.At Line 12,
test.MultiStageTestConfigurationLiteralis dereferenced without checking whethertestortest.MultiStageTestConfigurationLiteralis nil.💡 Proposed fix
func LeasesForTest(test *TestStepConfiguration) (ret []StepLease) { + if test == nil || test.MultiStageTestConfigurationLiteral == nil { + return nil + } multiStageTest := test.MultiStageTestConfigurationLiteral🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/api/leases.go` around lines 11 - 13, LeasesForTest currently dereferences test and test.MultiStageTestConfigurationLiteral without guards; add nil checks at the start of LeasesForTest to return empty slice if test is nil or test.MultiStageTestConfigurationLiteral is nil/zero value, then proceed to set multiStageTest := test.MultiStageTestConfigurationLiteral and use its ClusterProfile field as before (referencing LeasesForTest, TestStepConfiguration, and MultiStageTestConfigurationLiteral to locate the change).
🧹 Nitpick comments (3)
pkg/api/leases_test.go (1)
19-29: Add coverage forClusterProfileTargetpropagation.This case validates
ClusterProfilebut notClusterProfileTarget. Since the new flow relies on target propagation for secret import, this should be asserted explicitly.✅ Suggested test adjustment
name: "cluster profile, lease", tests: TestStepConfiguration{ + As: "e2e-aws", MultiStageTestConfigurationLiteral: &MultiStageTestConfigurationLiteral{ ClusterProfile: ClusterProfileAWS, }, }, expected: []StepLease{{ ResourceType: "aws-quota-slice", Env: DefaultLeaseEnv, Count: 1, ClusterProfile: string(ClusterProfileAWS), + ClusterProfileTarget: "e2e-aws", }},🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/api/leases_test.go` around lines 19 - 29, The test currently sets MultiStageTestConfigurationLiteral.ClusterProfile but does not set or assert ClusterProfileTarget propagation; update the test case (the TestStepConfiguration input using MultiStageTestConfigurationLiteral) to include a ClusterProfileTarget value and update the expected StepLease entry to assert that StepLease.ClusterProfileTarget (or the field name used in the lease struct) equals that value, ensuring the test covers propagation of ClusterProfileTarget from the input config to the produced StepLease.pkg/steps/lease_test.go (2)
265-266: Minor grammar issue: "two lease" → "two leases"✏️ Proposed fix
- name: "Acquire two lease of different types", + name: "Acquire two leases of different types",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/steps/lease_test.go` around lines 265 - 266, The test case name string "Acquire two lease of different types" has a grammar typo; update the test case's name value (the string literal used in the test case definition, e.g., in the test case struct/entry where name: "Acquire two lease of different types") to "Acquire two leases of different types" so the description is grammatically correct.
254-438: Consider adding error-path test cases.The current test cases cover happy-path scenarios well. Consider adding test cases for error conditions such as:
- Cluster profile getter returning an error (profile not found)
- Source secret missing from the "ci" namespace
- Malformed lease name that cannot be parsed for cluster profile extraction
This would improve test robustness and ensure error handling paths are exercised.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/steps/lease_test.go` around lines 254 - 438, Add negative test cases to the existing table-driven tests: insert entries where leases (the []api.StepLease row) reference a non-existent cluster profile (leave clusterProfiles map without that key to simulate "profile not found"), where the source secret in objects ([]ctrlruntimeclient.Object) is omitted to simulate "missing ci secret", and where resources (map[string]*common.Resource) contain a malformed Name value (e.g., not parseable like "bad-name") to exercise the lease-name parsing error path; for each new case assert the function under test returns an error (check err != nil) and that no successful provides/secrets are produced (wantProvides empty and wantSecrets.Items length 0 or wantCalls empty) so the test verifies the error-handling branches for ClusterProfile getter failures, missing source secret, and malformed lease names.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/steps/lease.go`:
- Around line 174-176: The comparator passed to sort.Slice is indexing s.leases
with the loop indices instead of using the permutation in sorted, so change the
comparator to compare s.leases[sorted[i]] and s.leases[sorted[j]] (e.g., use
sorted[i]/sorted[j] when accessing s.leases and compare their ResourceType) so
the sort orders the permutation slice correctly for deadlock avoidance; update
the anonymous func in the sort.Slice call that currently references
s.leases[i]/s.leases[j] to reference s.leases[sorted[i]]/s.leases[sorted[j]]
instead.
- Around line 191-195: The loop is overwriting s.clusterProfileName for every
lease even when l.ClusterProfile is empty; change the logic in the lease
iteration so s.clusterProfileName is only set when l.ClusterProfile (or the
resolved clusterProfileName from clusterProfileFromResources(names)) is
non-empty—i.e., if l.ClusterProfile != "" then set s.clusterProfileName =
l.ClusterProfile, otherwise preserve the existing s.clusterProfileName;
similarly only assign s.clusterProfileSetName when you have a non-empty
l.ClusterProfile and when clusterProfileFromResources(names) returns non-empty,
using the functions/fields clusterProfileFromResources(names), l.ClusterProfile,
s.clusterProfileName and s.clusterProfileSetName to locate the change.
In `@pkg/steps/multi_stage/multi_stage.go`:
- Around line 218-223: The code unconditionally assigns s.profile from
getClusterProfileFromParams(s.params) which can overwrite a previously
configured profile with an empty value; change the logic so that after calling
getClusterProfileFromParams (and handling err), only assign s.profile =
clusterProfile when clusterProfile is non-empty (e.g., check clusterProfile !=
""), otherwise leave the existing s.profile intact so missing/empty
CLUSTER_PROFILE does not clear the configured profile; the relevant symbols are
getClusterProfileFromParams, s.params, and s.profile.
---
Duplicate comments:
In `@pkg/api/leases.go`:
- Around line 11-13: LeasesForTest currently dereferences test and
test.MultiStageTestConfigurationLiteral without guards; add nil checks at the
start of LeasesForTest to return empty slice if test is nil or
test.MultiStageTestConfigurationLiteral is nil/zero value, then proceed to set
multiStageTest := test.MultiStageTestConfigurationLiteral and use its
ClusterProfile field as before (referencing LeasesForTest,
TestStepConfiguration, and MultiStageTestConfigurationLiteral to locate the
change).
---
Nitpick comments:
In `@pkg/api/leases_test.go`:
- Around line 19-29: The test currently sets
MultiStageTestConfigurationLiteral.ClusterProfile but does not set or assert
ClusterProfileTarget propagation; update the test case (the
TestStepConfiguration input using MultiStageTestConfigurationLiteral) to include
a ClusterProfileTarget value and update the expected StepLease entry to assert
that StepLease.ClusterProfileTarget (or the field name used in the lease struct)
equals that value, ensuring the test covers propagation of ClusterProfileTarget
from the input config to the produced StepLease.
In `@pkg/steps/lease_test.go`:
- Around line 265-266: The test case name string "Acquire two lease of different
types" has a grammar typo; update the test case's name value (the string literal
used in the test case definition, e.g., in the test case struct/entry where
name: "Acquire two lease of different types") to "Acquire two leases of
different types" so the description is grammatically correct.
- Around line 254-438: Add negative test cases to the existing table-driven
tests: insert entries where leases (the []api.StepLease row) reference a
non-existent cluster profile (leave clusterProfiles map without that key to
simulate "profile not found"), where the source secret in objects
([]ctrlruntimeclient.Object) is omitted to simulate "missing ci secret", and
where resources (map[string]*common.Resource) contain a malformed Name value
(e.g., not parseable like "bad-name") to exercise the lease-name parsing error
path; for each new case assert the function under test returns an error (check
err != nil) and that no successful provides/secrets are produced (wantProvides
empty and wantSecrets.Items length 0 or wantCalls empty) so the test verifies
the error-handling branches for ClusterProfile getter failures, missing source
secret, and malformed lease names.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 88b7b994-ec63-4358-b162-247fb954ca82
📒 Files selected for processing (13)
cmd/ci-operator/main.gopkg/api/constant.gopkg/api/leases.gopkg/api/leases_test.gopkg/api/types.gopkg/defaults/config.gopkg/defaults/defaults.gopkg/defaults/defaults_test.gopkg/prowgen/prowgen.gopkg/steps/lease.gopkg/steps/lease_test.gopkg/steps/multi_stage/multi_stage.gopkg/steps/multi_stage/multi_stage_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
- pkg/api/types.go
- pkg/steps/multi_stage/multi_stage_test.go
- pkg/defaults/config.go
- pkg/defaults/defaults_test.go
danilo-gemoli
force-pushed
the
feat/ci-operator/cluster-profile-sets
branch
from
e2447fd to
926ce2d
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
pkg/steps/lease_test.go (1)
241-556: Add a failure-path case for cluster-profile setup after successful acquire.
TestAcquireLeasescurrently validates happy paths, but it does not cover the case where acquire succeeds andhandleClusterProfilefails. Adding that case will lock in correct cleanup/release behavior and prevent lease-leak regressions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/steps/lease_test.go` around lines 241 - 556, TestAcquireLeases lacks a failure-path for cluster-profile setup after a successful lease acquire; add a new subtest case in TestAcquireLeases that includes a lease with ClusterProfile/ClusterProfileTarget, configure the fake clusterProfileGetter or kubeClient to make handleClusterProfile return an error (simulate failure during cluster-profile setup), and assert that the leaseClient was still called to release the acquired resource (check gotCalls contains releaseone for the acquired resource) and that no orphaned cluster-profile secret remains in kubeClient (verify secrets list and provides behave as expected); reference the test harness symbols TestAcquireLeases, LeaseStep, lease.NewFakeClient, clusterProfileGetter and ensure the new test mirrors existing cluster-profile cases but injects the failure to validate cleanup.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/steps/lease.go`:
- Around line 252-281: The code dereferences s.clusterProfileGetter and
s.kubeClient without nil checks in handleClusterProfile and
importClusterProfileSecret which can panic if wiring is missing; add guards that
validate s.clusterProfileGetter != nil before calling it in handleClusterProfile
(return a descriptive error like "clusterProfileGetter not configured for
cluster profile X") and validate s.kubeClient != nil at the start of
importClusterProfileSecret (return a descriptive error like "kubeClient not
configured when importing secret Y for test Z") so missing dependencies produce
controlled errors instead of panics.
- Around line 187-200: The lease acquisition code assigns l.resources after
calling s.handleClusterProfile, which can fail and trigger cleanup without the
acquired names; to fix, assign l.resources = names immediately after names are
computed (before invoking s.handleClusterProfile) so cleanup/rollback sees the
acquired lease names, then call s.handleClusterProfile(ctx, l, names) and handle
its error as before; update references to l.resources, names, and
s.handleClusterProfile in pkg/steps/lease.go accordingly.
---
Nitpick comments:
In `@pkg/steps/lease_test.go`:
- Around line 241-556: TestAcquireLeases lacks a failure-path for
cluster-profile setup after a successful lease acquire; add a new subtest case
in TestAcquireLeases that includes a lease with
ClusterProfile/ClusterProfileTarget, configure the fake clusterProfileGetter or
kubeClient to make handleClusterProfile return an error (simulate failure during
cluster-profile setup), and assert that the leaseClient was still called to
release the acquired resource (check gotCalls contains releaseone for the
acquired resource) and that no orphaned cluster-profile secret remains in
kubeClient (verify secrets list and provides behave as expected); reference the
test harness symbols TestAcquireLeases, LeaseStep, lease.NewFakeClient,
clusterProfileGetter and ensure the new test mirrors existing cluster-profile
cases but injects the failure to validate cleanup.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1ff93ed9-a74c-4610-9663-f1acce9a9119
📒 Files selected for processing (4)
pkg/steps/lease.gopkg/steps/lease_test.gopkg/steps/multi_stage/multi_stage.gopkg/steps/multi_stage/multi_stage_test.go
Comment on lines
+252
to
+281
| func (s *leaseStep) handleClusterProfile(ctx context.Context, l *stepLease, names []string) error { | ||
| s.clusterProfileName = l.ClusterProfile | ||
|
|
||
| if clusterProfileName := clusterProfileFromResources(names); clusterProfileName != "" { | ||
| s.clusterProfileSetName = l.ClusterProfile | ||
| s.clusterProfileName = clusterProfileName | ||
| } | ||
|
|
||
| if s.clusterProfileName == "" { | ||
| return nil | ||
| } | ||
|
|
||
| cpDetails, err := s.clusterProfileGetter(s.clusterProfileName) | ||
| if err != nil { | ||
| return fmt.Errorf("resolve cluster profile %s: %w", s.clusterProfileName, err) | ||
| } | ||
|
|
||
| if err := s.importClusterProfileSecret(ctx, cpDetails.Secret, l.ClusterProfileTarget); err != nil { | ||
| return fmt.Errorf("import secret %s for cluster profile %s: %w", cpDetails.Secret, s.clusterProfileName, err) | ||
| } | ||
|
|
||
| return nil | ||
| } | ||
|
|
||
| // importClusterProfileSecret retrieves the cluster profile secret name using config resolver, | ||
| // and gets the secret from the ci namespace | ||
| func (s *leaseStep) importClusterProfileSecret(ctx context.Context, secretName, testName string) error { | ||
| ciSecret := &coreapi.Secret{} | ||
| err := s.kubeClient.Get(ctx, ctrlruntimeclient.ObjectKey{Namespace: "ci", Name: secretName}, ciSecret) | ||
| if err != nil { |
There was a problem hiding this comment.
Guard cluster-profile dependencies before dereference.
At Line 264 and Line 280, s.clusterProfileGetter / s.kubeClient are used without nil checks. If a cluster-profile lease is configured but wiring is missing, this panics instead of returning a controlled error.
💡 Proposed fix
func (s *leaseStep) handleClusterProfile(ctx context.Context, l *stepLease, names []string) error {
+ if s.clusterProfileGetter == nil {
+ return errors.New("cluster profile getter is not configured")
+ }
+ if s.kubeClient == nil {
+ return errors.New("kube client is not configured")
+ }
+
s.clusterProfileName = l.ClusterProfile📝 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.
Suggested change
| func (s *leaseStep) handleClusterProfile(ctx context.Context, l *stepLease, names []string) error { | |
| s.clusterProfileName = l.ClusterProfile | |
| if clusterProfileName := clusterProfileFromResources(names); clusterProfileName != "" { | |
| s.clusterProfileSetName = l.ClusterProfile | |
| s.clusterProfileName = clusterProfileName | |
| } | |
| if s.clusterProfileName == "" { | |
| return nil | |
| } | |
| cpDetails, err := s.clusterProfileGetter(s.clusterProfileName) | |
| if err != nil { | |
| return fmt.Errorf("resolve cluster profile %s: %w", s.clusterProfileName, err) | |
| } | |
| if err := s.importClusterProfileSecret(ctx, cpDetails.Secret, l.ClusterProfileTarget); err != nil { | |
| return fmt.Errorf("import secret %s for cluster profile %s: %w", cpDetails.Secret, s.clusterProfileName, err) | |
| } | |
| return nil | |
| } | |
| // importClusterProfileSecret retrieves the cluster profile secret name using config resolver, | |
| // and gets the secret from the ci namespace | |
| func (s *leaseStep) importClusterProfileSecret(ctx context.Context, secretName, testName string) error { | |
| ciSecret := &coreapi.Secret{} | |
| err := s.kubeClient.Get(ctx, ctrlruntimeclient.ObjectKey{Namespace: "ci", Name: secretName}, ciSecret) | |
| if err != nil { | |
| func (s *leaseStep) handleClusterProfile(ctx context.Context, l *stepLease, names []string) error { | |
| if s.clusterProfileGetter == nil { | |
| return errors.New("cluster profile getter is not configured") | |
| } | |
| if s.kubeClient == nil { | |
| return errors.New("kube client is not configured") | |
| } | |
| s.clusterProfileName = l.ClusterProfile | |
| if clusterProfileName := clusterProfileFromResources(names); clusterProfileName != "" { | |
| s.clusterProfileSetName = l.ClusterProfile | |
| s.clusterProfileName = clusterProfileName | |
| } | |
| if s.clusterProfileName == "" { | |
| return nil | |
| } | |
| cpDetails, err := s.clusterProfileGetter(s.clusterProfileName) | |
| if err != nil { | |
| return fmt.Errorf("resolve cluster profile %s: %w", s.clusterProfileName, err) | |
| } | |
| if err := s.importClusterProfileSecret(ctx, cpDetails.Secret, l.ClusterProfileTarget); err != nil { | |
| return fmt.Errorf("import secret %s for cluster profile %s: %w", cpDetails.Secret, s.clusterProfileName, err) | |
| } | |
| return nil | |
| } | |
| // importClusterProfileSecret retrieves the cluster profile secret name using config resolver, | |
| // and gets the secret from the ci namespace | |
| func (s *leaseStep) importClusterProfileSecret(ctx context.Context, secretName, testName string) error { | |
| ciSecret := &coreapi.Secret{} | |
| err := s.kubeClient.Get(ctx, ctrlruntimeclient.ObjectKey{Namespace: "ci", Name: secretName}, ciSecret) | |
| if err != nil { |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/steps/lease.go` around lines 252 - 281, The code dereferences
s.clusterProfileGetter and s.kubeClient without nil checks in
handleClusterProfile and importClusterProfileSecret which can panic if wiring is
missing; add guards that validate s.clusterProfileGetter != nil before calling
it in handleClusterProfile (return a descriptive error like
"clusterProfileGetter not configured for cluster profile X") and validate
s.kubeClient != nil at the start of importClusterProfileSecret (return a
descriptive error like "kubeClient not configured when importing secret Y for
test Z") so missing dependencies produce controlled errors instead of panics.
926ce2d to
7d8ac45
Compare
Contributor
Author
|
/label tide/merge-method-squash |
openshift-ci
bot
added
the
tide/merge-method-squash
Contributor
Author
|
/test e2e |
liangxia
approved these changes
Mar 6, 2026
Contributor
|
/hold |
openshift-ci
bot
added
the
do-not-merge/hold
jmguzik
reviewed
Mar 6, 2026
| if s.clusterProfileName == "" { | ||
| return nil | ||
| } | ||
|
|
Contributor
There was a problem hiding this comment.
nit?
if s.clusterProfileGetter == nil {
return fmt.Errorf("cluster profile getter is not configured")
}
It's fixable in the followup though
openshift-ci
bot
removed
the
do-not-merge/hold
Contributor
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: danilo-gemoli, jmguzik, liangxia 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 |
Contributor
Author
|
/override ci/prow/integration |
Contributor
|
@danilo-gemoli: Overrode contexts on behalf of danilo-gemoli: ci/prow/integration 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. |
Contributor
|
@danilo-gemoli: 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. |
Contributor
Author
|
/override ci/prow/images |
Contributor
|
Scheduling required tests: |
Contributor
|
@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. |
Contributor
Author
|
/override ci/prow/e2e |
Contributor
|
@danilo-gemoli: Overrode contexts on behalf of danilo-gemoli: ci/prow/e2e 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. |
openshift-merge-bot
bot
merged commit 6a0a2b5
into
openshift:main
13 of 14 checks passed
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
4 participants
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.
Introducing a simple and naive cluster profile sets implementation by embedding the cluster profile name into the lease resource names.
This PR is much easier to explain with a real example.
As of today, in OpenShift, we have the following
awscluster profiles, and their associated lease types:Each one of them is defined as follow in boskos, (choosing
aws-3as an example, see here):We define a new
openshift-org-awsset by taking all the names from theaws-*-quota-slicelease types and put them all into a single entryopenshift-org-aws-quota-slice:The
namesmatch the pattern${CLUSTER_PROFILE}--${REGION}--${QUOTA_SLICE}.The
openshift-org-awshas to be added as usual, see https://docs.ci.openshift.org/how-tos/adding-a-cluster-profile/, such that it maps to theopenshift-org-aws-quota-slicelease type.At this stage a test, that is referencing the new cluster profile set, runs:
ci-operatorstarts and performs what follow:openshift-org-aws-quota-sliceat runtime, hence gettingaws-4--us-east-1--quota-slice-01as a lease (seepkg/steps/lease.go).aws-4cluster profile name in it (seepkg/steps/lease.go). The code now handles lease names matching the pattern${CLUSTER_PROFILE}--${REGION}--${QUOTA_SLICE}.cluster-secrets-aws-4from theciNS intoci-op-xxxx. This part was previously handled incmd/ci-operator/main.gobut it is now executed inpkg/steps/lease.go.CLUSTER_PROFILE_SET_NAME=openshift-org-awsto any step. The variable is exported as a parameter bypkg/steps/lease.goand then gets cunsumed inpkg/steps/multi_stage/multi_stage.go.Important note about (3): Since a cluster profile might not been know until the moment a lease is acquired,
ci-operatordoesn't copy the secret that belongs to a cluster profile at the beginning of its execution anymore.Summary by CodeRabbit
New Features
Improvements
Tests