ci-operator: Introduce cluster_profile_literal#5247
Open
danilo-gemoli wants to merge 5 commits into
Open
Conversation
Contributor
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
📝 WalkthroughWalkthroughCluster-profile handling is migrated from string enums to literal/details structs; Parameters API returns any with typed errors; registry loads and validates cluster profiles and passes them into resolver; lease/multi-stage/ip-pool steps, defaults, validation, commands, and tests updated to use new shapes. ChangesCluster profile object model and Parameters API
Registry loading, validation, and resolver
Lease step and STS integration
Multi-stage step and pod generation
IP pool step
Prowgen, validation, rehearse, and command tests
Test fixtures and integration artifacts
🎯 4 (Complex) | ⏱️ ~60 minutes ✨ Finishing Touches🧪 Generate unit tests (beta)
|
Contributor
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: danilo-gemoli 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
Contributor
Author
|
@coderabbitai help |
ChatThere are 3 ways to chat with CodeRabbit:
CodeRabbit commands
Other keywords and placeholders
Status, support, documentation and community
|
Contributor
Author
|
/test e2e |
Contributor
Author
|
@coderabbitai full review |
✅ Action performedFull review finished. |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmd/ci-operator/main_test.go (1)
1819-1823:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAssert the comparison result in this test.
Line 1822 calls
reflect.DeepEqual(...)but ignores the result, so the test always passes even if output is wrong.Suggested fix
for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { tc.options.getClusterProfileNamesFromTargets() - reflect.DeepEqual(tc.expectedProfileNames, tc.options.clusterProfiles) + if diff := cmp.Diff(tc.expectedProfileNames, tc.options.clusterProfiles); diff != "" { + t.Fatalf("unexpected cluster profile names (-want,+got): %s", diff) + } }) }🤖 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 `@cmd/ci-operator/main_test.go` around lines 1819 - 1823, The test currently calls reflect.DeepEqual(tc.expectedProfileNames, tc.options.clusterProfiles) but ignores the boolean result; update the subtest in the t.Run closure to assert the result by checking if !reflect.DeepEqual(...) and calling t.Fatalf or t.Errorf with a descriptive message including the expected and actual values. Locate the loop over testCases and the tc.options.getClusterProfileNamesFromTargets() call and add the assertion immediately after that call to fail the test when the comparison returns false.
🧹 Nitpick comments (2)
pkg/api/parameters.go (1)
13-39: ⚡ Quick winAdd GoDoc for newly exported API symbols.
ErrParamNotFound,ErrParamTypeMismatch, andGetParamTypedare exported but undocumented. Please add proper GoDoc comments describing behavior and expected usage.As per coding guidelines, "
**/*.go: Go documentation on Classes/Functions/Fields should be written properly" and "Comment important exported functions with their purpose, parameters, and return values."Also applies to: 210-233
🤖 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/parameters.go` around lines 13 - 39, Add GoDoc comments for the exported types and function: document ErrParamNotFound (what it represents, the meaning of its param field and when it is returned), ErrParamTypeMismatch (describe want/got fields and when this error is used), and GetParamTyped (describe the purpose, input parameters, expected return values, and error cases including when ErrParamNotFound or ErrParamTypeMismatch are returned). Also add short comments for the Error() and Is() methods to state their behavior (Error returns the formatted message; Is implements error comparison semantics). Keep comments concise and follow GoDoc style placed immediately above each exported symbol.Source: Coding guidelines
pkg/api/clusterprofile.go (1)
1210-1212: ⚡ Quick winAdd GoDoc for exported
ClusterProfileFromParams.Line 1210 changes an exported API but still lacks a leading doc comment. Please document return semantics (especially nil/error behavior) for callers.
As per coding guidelines,
**/*.go: “Go documentation on Classes/Functions/Fields should be written properly”.🤖 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 1210 - 1212, Add a GoDoc comment above the exported function ClusterProfileFromParams that succinctly describes its purpose (convert Parameters into a *ClusterProfileDetails), its return semantics (returns a non-nil *ClusterProfileDetails and nil error on success; returns nil and a non-nil error if the parameter is missing or cannot be decoded), and mention that it delegates to GetParamTyped with ClusterProfileParam; include any relevant expectations about the params input (e.g., that ClusterProfileParam must be present).Source: Coding guidelines
🤖 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/load/load.go`:
- Around line 200-205: The code unconditionally calls
ClusterProfilesConfig(clusterProfilesConfigPath) which fails with an opaque
error when clusterProfilesConfigPath is empty; modify the load logic in the
function that currently invokes ClusterProfilesConfig to first check if
clusterProfilesConfigPath is empty/blank and, if so, set profiles to an
empty/default value (or skip loading) instead of calling ClusterProfilesConfig;
when the path is non-empty, call ClusterProfilesConfig and wrap any returned
error with the resolved clusterProfilesConfigPath (include the path in the error
message) so diagnostics show the file that failed to open; keep the rest of the
flow (including the subsequent registry.Validate(references, chains, workflows,
observers, profiles)) unchanged.
In `@pkg/prowgen/jobbase.go`:
- Around line 174-176: The literal branch is using the raw field
clusterProfile.ClusterType which can be empty/mismatched; change it to derive
the cloud label from the profile's resolved ClusterType (use
clusterProfile.ClusterType() or look up ClusterProfile.Name to get its
ClusterType) so the call p.WithLabel(cioperatorapi.CloudLabel, ...) uses the
canonical/mapped value; alternatively validate/mutate clusterProfile.ClusterType
to match ClusterProfile.Name before calling p.WithLabel.
In `@pkg/validation/test.go`:
- Around line 452-462: When v.validClusterProfiles is configured (non-nil) you
must treat absence of the profile in that map as a validation failure instead of
falling back to the static list; change the logic around v.validClusterProfiles
and api.ClusterProfile(profileName) so that if v.validClusterProfiles != nil and
the key is missing you return an error (use the same message pattern with
fieldRoot and profileName), otherwise if the key exists call
verifyClusterProfileOwnership(...) as currently done; keep the existing fallback
to slices.Contains(api.ClusterProfiles(), api.ClusterProfile(profileName)) only
when v.validClusterProfiles is nil.
---
Outside diff comments:
In `@cmd/ci-operator/main_test.go`:
- Around line 1819-1823: The test currently calls
reflect.DeepEqual(tc.expectedProfileNames, tc.options.clusterProfiles) but
ignores the boolean result; update the subtest in the t.Run closure to assert
the result by checking if !reflect.DeepEqual(...) and calling t.Fatalf or
t.Errorf with a descriptive message including the expected and actual values.
Locate the loop over testCases and the
tc.options.getClusterProfileNamesFromTargets() call and add the assertion
immediately after that call to fail the test when the comparison returns false.
---
Nitpick comments:
In `@pkg/api/clusterprofile.go`:
- Around line 1210-1212: Add a GoDoc comment above the exported function
ClusterProfileFromParams that succinctly describes its purpose (convert
Parameters into a *ClusterProfileDetails), its return semantics (returns a
non-nil *ClusterProfileDetails and nil error on success; returns nil and a
non-nil error if the parameter is missing or cannot be decoded), and mention
that it delegates to GetParamTyped with ClusterProfileParam; include any
relevant expectations about the params input (e.g., that ClusterProfileParam
must be present).
In `@pkg/api/parameters.go`:
- Around line 13-39: Add GoDoc comments for the exported types and function:
document ErrParamNotFound (what it represents, the meaning of its param field
and when it is returned), ErrParamTypeMismatch (describe want/got fields and
when this error is used), and GetParamTyped (describe the purpose, input
parameters, expected return values, and error cases including when
ErrParamNotFound or ErrParamTypeMismatch are returned). Also add short comments
for the Error() and Is() methods to state their behavior (Error returns the
formatted message; Is implements error comparison semantics). Keep comments
concise and follow GoDoc style placed immediately above each exported symbol.
🪄 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: 203881ef-54b7-4fd4-90f4-533024d2bf1d
📒 Files selected for processing (37)
cmd/ci-operator-checkconfig/main.gocmd/ci-operator-prowgen/main.gocmd/ci-operator/main.gocmd/ci-operator/main_test.gocmd/registry-replacer/main.gocmd/registry-replacer/testdata/zz_fixture_TestReplacer_Unused_base_images_are_pruned.yamlpkg/api/clusterprofile.gopkg/api/constant.gopkg/api/leases.gopkg/api/leases_test.gopkg/api/parameters.gopkg/api/parameters_test.gopkg/api/types.gopkg/cmd/release/util.gopkg/defaults/defaults.gopkg/defaults/defaults_test.gopkg/load/agents/registryAgent.gopkg/load/load.gopkg/prowgen/jobbase.gopkg/registry/resolver.gopkg/registry/resolver_test.gopkg/rehearse/jobs_test.gopkg/rehearse/rehearse.gopkg/steps/ip_pool.gopkg/steps/ip_pool_test.gopkg/steps/lease.gopkg/steps/lease_test.gopkg/steps/multi_stage/gen.gopkg/steps/multi_stage/gen_test.gopkg/steps/multi_stage/multi_stage.gopkg/steps/multi_stage/multi_stage_test.gopkg/steps/multi_stage/run_test.gopkg/steps/multi_stage/testdata/zz_fixture_TestGenerateObservers.yamlpkg/steps/multi_stage/testdata/zz_fixture_TestGeneratePods_STS_volumes.yamlpkg/steps/multi_stage/testdata/zz_fixture_TestGeneratePods_generate_pods.yamlpkg/validation/test.gopkg/validation/test_test.go
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
openshift/release(manual)openshift/ci-docs(manual)openshift/release-controller(manual)openshift/ci-chat-bot(manual)
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/types.go`:
- Around line 1327-1329: You renamed the serialized field to
cluster_profile_literal which breaks existing consumers; to fix, preserve
compatibility by keeping the new ClusterProfileLiteral *ClusterProfileLiteral
`json:"cluster_profile_literal"` field and add a backward-compatible alias field
(e.g., ClusterProfile *ClusterProfileLiteral `json:"cluster_profile,omitempty"`)
or add struct tags that accept both names, and ensure code that reads/serializes
(constructors/unmarshal helpers) prefers ClusterProfileLiteral when present but
falls back to ClusterProfile; reference the MultiStageTestConfigurationLiteral
type and ClusterProfileLiteral/ClusterProfile fields when making this change.
- Around line 952-965: FromClusterProfileDetails currently dereferences
profileDetails unconditionally and ClusterProfileLiteral omits fields like
owners and config_map; make the conversion nil-safe (if profileDetails == nil
return nil) and lossless by adding Owners and ConfigMap (and any other missing
ClusterProfileDetails fields you need preserved) to ClusterProfileLiteral and
copying those fields in FromClusterProfileDetails; ensure you copy slices/maps
defensively (nil -> nil, deep-copy if necessary) so configresolved can
round-trip externally defined profiles without data loss.
🪄 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: 563f7c79-b64c-4ba6-b54a-3e5bd33a8e72
⛔ Files ignored due to path filters (2)
pkg/api/zz_generated.deepcopy.gois excluded by!**/zz_generated*pkg/webreg/zz_generated.ci_operator_reference.gois excluded by!**/zz_generated*
📒 Files selected for processing (25)
cmd/ci-operator/main_test.gopkg/api/leases.gopkg/api/leases_test.gopkg/api/parameters.gopkg/api/types.gopkg/defaults/defaults_test.gopkg/prowgen/jobbase.gopkg/registry/resolver.gopkg/registry/resolver_test.gopkg/steps/ip_pool.gopkg/steps/ip_pool_test.gopkg/steps/lease.gopkg/steps/lease_test.gopkg/steps/multi_stage/gen.gopkg/steps/multi_stage/gen_test.gopkg/steps/multi_stage/multi_stage.gopkg/steps/multi_stage/multi_stage_test.gopkg/validation/test.gopkg/validation/test_test.gotest/integration/ci-operator-configresolver/expected/openshift-installer-console-merged-release-4.2-injected.jsontest/integration/ci-operator-configresolver/expected/openshift-installer-console-merged-release-4.2-installer-as-base.jsontest/integration/ci-operator-configresolver/expected/openshift-installer-release-4.2-golang111.jsontest/integration/ci-operator-configresolver/expected/openshift-installer-release-4.2-regChange.jsontest/integration/ci-operator-configresolver/expected/openshift-installer-release-4.2.jsontest/integration/ci-operator-configresolver/expected/resolved-config.json
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
openshift/release(manual)openshift/ci-docs(manual)openshift/release-controller(manual)openshift/ci-chat-bot(manual)
✅ Files skipped from review due to trivial changes (1)
- test/integration/ci-operator-configresolver/expected/resolved-config.json
🚧 Files skipped from review as they are similar to previous changes (17)
- pkg/prowgen/jobbase.go
- cmd/ci-operator/main_test.go
- pkg/steps/ip_pool_test.go
- pkg/api/leases.go
- pkg/steps/multi_stage/gen_test.go
- pkg/steps/ip_pool.go
- pkg/steps/multi_stage/gen.go
- pkg/steps/lease_test.go
- pkg/api/parameters.go
- pkg/defaults/defaults_test.go
- pkg/registry/resolver.go
- pkg/steps/lease.go
- pkg/steps/multi_stage/multi_stage_test.go
- pkg/api/leases_test.go
- pkg/validation/test.go
- pkg/registry/resolver_test.go
- pkg/steps/multi_stage/multi_stage.go
Comment on lines
+952
to
+965
| type ClusterProfileLiteral struct { | ||
| Name string `yaml:"name,omitempty" json:"name,omitempty"` | ||
| LeaseType string `yaml:"lease_type,omitempty" json:"lease_type,omitempty"` | ||
| ClusterType string `yaml:"cluster_type,omitempty" json:"cluster_type,omitempty"` | ||
| Secret string `yaml:"secret,omitempty" json:"secret,omitempty"` | ||
| } | ||
|
|
||
| func FromClusterProfileDetails(profileDetails *ClusterProfileDetails) *ClusterProfileLiteral { | ||
| return &ClusterProfileLiteral{ | ||
| Name: string(profileDetails.Name), | ||
| LeaseType: profileDetails.LeaseType, | ||
| ClusterType: profileDetails.ClusterType, | ||
| Secret: profileDetails.Secret, | ||
| } |
There was a problem hiding this comment.
Preserve the full cluster profile payload in the literal converter.
ClusterProfileLiteral only carries name, lease_type, cluster_type, and secret, and FromClusterProfileDetails() dereferences profileDetails unconditionally. That makes the resolved form lossy for profile data this PR says should survive expansion (owners, config_map) and turns a nil profile into a panic instead of a nil literal. Please make this conversion lossless and nil-safe so configresolved can round-trip externally defined profiles.
🤖 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/types.go` around lines 952 - 965, FromClusterProfileDetails currently
dereferences profileDetails unconditionally and ClusterProfileLiteral omits
fields like owners and config_map; make the conversion nil-safe (if
profileDetails == nil return nil) and lossless by adding Owners and ConfigMap
(and any other missing ClusterProfileDetails fields you need preserved) to
ClusterProfileLiteral and copying those fields in FromClusterProfileDetails;
ensure you copy slices/maps defensively (nil -> nil, deep-copy if necessary) so
configresolved can round-trip externally defined profiles without data loss.
Comment on lines
+1327
to
+1329
| type MultiStageTestConfigurationLiteral struct { | ||
| // ClusterProfile defines the profile/cloud provider for end-to-end test steps. | ||
| ClusterProfile ClusterProfile `json:"cluster_profile"` | ||
| // ClusterProfileLiteral defines the profile/cloud provider for end-to-end test steps. | ||
| ClusterProfileLiteral *ClusterProfileLiteral `json:"cluster_profile_literal"` |
There was a problem hiding this comment.
This serialized field rename is a breaking contract.
Changing resolved configs from steps.cluster_profile to steps.cluster_profile_literal without a compatibility path will break linked consumers that still read the scalar field. One concrete example is openshift/release::ci-operator/platform-balance/step-jobs-by-platform.py, which indexes _test['steps']['cluster_profile']. Please either land those consumer updates in the same stack or preserve the old field until they do.
🤖 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/types.go` around lines 1327 - 1329, You renamed the serialized field
to cluster_profile_literal which breaks existing consumers; to fix, preserve
compatibility by keeping the new ClusterProfileLiteral *ClusterProfileLiteral
`json:"cluster_profile_literal"` field and add a backward-compatible alias field
(e.g., ClusterProfile *ClusterProfileLiteral `json:"cluster_profile,omitempty"`)
or add struct tags that accept both names, and ensure code that reads/serializes
(constructors/unmarshal helpers) prefers ClusterProfileLiteral when present but
falls back to ClusterProfile; reference the MultiStageTestConfigurationLiteral
type and ClusterProfileLiteral/ClusterProfile fields when making this change.
Source: Linked repositories
Contributor
Author
|
/test e2e |
Contributor
Author
|
/retest-required |
danilo-gemoli
force-pushed
the
feat/ci-operator/literal-cluster-profile
branch
from
f34bf40 to
adbe543
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/api/parameters.go (1)
13-16: ⚡ Quick winAdd proper Go doc comments for the exported API.
ErrParamNotFound,ErrParamTypeMismatch,DeferredParameters,NewDeferredParameters,Map,Set,Add, andGetParamTypedare exported without doc comments, and theGetStringcomment still starts withGet. That breaks godoc and the repo's Go-doc requirement.As per coding guidelines, "Go documentation on Classes/Functions/Fields should be written properly" and "Comment important exported functions with their purpose, parameters, and return values."
Also applies to: 28-31, 51-57, 59-107, 145-165, 212-234
🤖 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/parameters.go` around lines 13 - 16, Add proper Go doc comments for all exported types and functions so godoc requirements are satisfied: add a comment starting with the exact exported name for ErrParamNotFound and ErrParamTypeMismatch describing the error and fields, document the DeferredParameters type explaining its purpose, describe NewDeferredParameters (what it constructs and returns), and document Map, Set, Add, GetParamTyped methods (their behavior, parameters and return values); also update the GetString comment to start with "GetString" (not "Get") and describe what it returns. Ensure each comment is a single or multi-line Go comment directly above the declaration and begins with the exported identifier.Source: Coding guidelines
🤖 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.
Nitpick comments:
In `@pkg/api/parameters.go`:
- Around line 13-16: Add proper Go doc comments for all exported types and
functions so godoc requirements are satisfied: add a comment starting with the
exact exported name for ErrParamNotFound and ErrParamTypeMismatch describing the
error and fields, document the DeferredParameters type explaining its purpose,
describe NewDeferredParameters (what it constructs and returns), and document
Map, Set, Add, GetParamTyped methods (their behavior, parameters and return
values); also update the GetString comment to start with "GetString" (not "Get")
and describe what it returns. Ensure each comment is a single or multi-line Go
comment directly above the declaration and begins with the exported identifier.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 39f4fff9-ecad-452b-a724-287f6e6b443f
⛔ Files ignored due to path filters (2)
pkg/api/zz_generated.deepcopy.gois excluded by!**/zz_generated*pkg/webreg/zz_generated.ci_operator_reference.gois excluded by!**/zz_generated*
📒 Files selected for processing (27)
cmd/ci-operator/main_test.gocmd/registry-replacer/testdata/zz_fixture_TestReplacer_Unused_base_images_are_pruned.yamlpkg/api/leases.gopkg/api/leases_test.gopkg/api/parameters.gopkg/api/types.gopkg/defaults/defaults_test.gopkg/prowgen/jobbase.gopkg/registry/resolver.gopkg/registry/resolver_test.gopkg/steps/ip_pool.gopkg/steps/ip_pool_test.gopkg/steps/lease.gopkg/steps/lease_test.gopkg/steps/multi_stage/gen.gopkg/steps/multi_stage/gen_test.gopkg/steps/multi_stage/multi_stage.gopkg/steps/multi_stage/multi_stage_test.gopkg/validation/test.gopkg/validation/test_test.gotest/integration/ci-operator-configresolver/expected/openshift-installer-console-merged-release-4.2-injected.jsontest/integration/ci-operator-configresolver/expected/openshift-installer-console-merged-release-4.2-installer-as-base.jsontest/integration/ci-operator-configresolver/expected/openshift-installer-release-4.2-golang111.jsontest/integration/ci-operator-configresolver/expected/openshift-installer-release-4.2-regChange.jsontest/integration/ci-operator-configresolver/expected/openshift-installer-release-4.2.jsontest/integration/ci-operator-configresolver/expected/resolved-config.jsontest/integration/pj-rehearse/expected.yaml
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
openshift/release(manual)openshift/ci-docs(manual)openshift/release-controller(manual)openshift/ci-chat-bot(manual)
💤 Files with no reviewable changes (1)
- cmd/registry-replacer/testdata/zz_fixture_TestReplacer_Unused_base_images_are_pruned.yaml
🚧 Files skipped from review as they are similar to previous changes (24)
- test/integration/ci-operator-configresolver/expected/openshift-installer-release-4.2-golang111.json
- test/integration/ci-operator-configresolver/expected/resolved-config.json
- test/integration/ci-operator-configresolver/expected/openshift-installer-console-merged-release-4.2-injected.json
- pkg/api/leases.go
- pkg/prowgen/jobbase.go
- pkg/validation/test_test.go
- test/integration/ci-operator-configresolver/expected/openshift-installer-release-4.2-regChange.json
- pkg/steps/lease_test.go
- pkg/steps/ip_pool_test.go
- pkg/registry/resolver.go
- pkg/steps/lease.go
- test/integration/ci-operator-configresolver/expected/openshift-installer-console-merged-release-4.2-installer-as-base.json
- cmd/ci-operator/main_test.go
- pkg/steps/multi_stage/gen_test.go
- pkg/defaults/defaults_test.go
- pkg/validation/test.go
- test/integration/ci-operator-configresolver/expected/openshift-installer-release-4.2.json
- pkg/steps/multi_stage/gen.go
- pkg/steps/ip_pool.go
- pkg/steps/multi_stage/multi_stage_test.go
- pkg/steps/multi_stage/multi_stage.go
- pkg/api/types.go
- pkg/registry/resolver_test.go
- pkg/api/leases_test.go
Contributor
Author
|
/test e2e |
danilo-gemoli
force-pushed
the
feat/ci-operator/literal-cluster-profile
branch
from
adbe543 to
398d3cd
Compare
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)
cmd/ci-operator/main_test.go (1)
1750-1825:⚠️ Potential issue | 🟠 Major | ⚡ Quick winThis test never asserts the migrated behavior.
Line 1822 drops the
reflect.DeepEqualresult, so the test always passes. On top of that, it compares[]stringagainsto.clusterProfiles, which is populated as[]metrics.ClusterProfileForTargetincmd/ci-operator/main.go, so the assertion would still be wrong if wired in. Please assert on the extractedProfileNamevalues (or change the expected type) so this migration is actually covered.Suggested fix
for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { tc.options.getClusterProfileNamesFromTargets() - reflect.DeepEqual(tc.expectedProfileNames, tc.options.clusterProfiles) + actualProfileNames := make([]string, 0, len(tc.options.clusterProfiles)) + for _, profile := range tc.options.clusterProfiles { + actualProfileNames = append(actualProfileNames, profile.ProfileName) + } + if diff := cmp.Diff(tc.expectedProfileNames, actualProfileNames); diff != "" { + t.Fatalf("unexpected cluster profile names (-want +got):\n%s", diff) + } }) } }🤖 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 `@cmd/ci-operator/main_test.go` around lines 1750 - 1825, The test TestGetClusterProfileNamesFromTargets never asserts because the reflect.DeepEqual result is discarded and also compares []string to options.clusterProfiles which is a []metrics.ClusterProfileForTarget; update the test to call tc.options.getClusterProfileNamesFromTargets(), extract the ProfileName values from tc.options.clusterProfiles (the slice of metrics.ClusterProfileForTarget) into a []string and then assert equality with tc.expectedProfileNames (use t.Fatalf or t.Errorf on mismatch or a test helper like require.Equal). Ensure you reference the getClusterProfileNamesFromTargets method and the options.clusterProfiles field when locating and fixing the assertion.
🤖 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.
Outside diff comments:
In `@cmd/ci-operator/main_test.go`:
- Around line 1750-1825: The test TestGetClusterProfileNamesFromTargets never
asserts because the reflect.DeepEqual result is discarded and also compares
[]string to options.clusterProfiles which is a
[]metrics.ClusterProfileForTarget; update the test to call
tc.options.getClusterProfileNamesFromTargets(), extract the ProfileName values
from tc.options.clusterProfiles (the slice of metrics.ClusterProfileForTarget)
into a []string and then assert equality with tc.expectedProfileNames (use
t.Fatalf or t.Errorf on mismatch or a test helper like require.Equal). Ensure
you reference the getClusterProfileNamesFromTargets method and the
options.clusterProfiles field when locating and fixing the assertion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: ffc4f19d-dbed-4e5b-8f57-24ab63907bf3
⛔ Files ignored due to path filters (2)
pkg/api/zz_generated.deepcopy.gois excluded by!**/zz_generated*pkg/webreg/zz_generated.ci_operator_reference.gois excluded by!**/zz_generated*
📒 Files selected for processing (27)
cmd/ci-operator/main_test.gocmd/registry-replacer/testdata/zz_fixture_TestReplacer_Unused_base_images_are_pruned.yamlpkg/api/leases.gopkg/api/leases_test.gopkg/api/parameters.gopkg/api/types.gopkg/defaults/defaults_test.gopkg/prowgen/jobbase.gopkg/registry/resolver.gopkg/registry/resolver_test.gopkg/steps/ip_pool.gopkg/steps/ip_pool_test.gopkg/steps/lease.gopkg/steps/lease_test.gopkg/steps/multi_stage/gen.gopkg/steps/multi_stage/gen_test.gopkg/steps/multi_stage/multi_stage.gopkg/steps/multi_stage/multi_stage_test.gopkg/validation/test.gopkg/validation/test_test.gotest/integration/ci-operator-configresolver/expected/openshift-installer-console-merged-release-4.2-injected.jsontest/integration/ci-operator-configresolver/expected/openshift-installer-console-merged-release-4.2-installer-as-base.jsontest/integration/ci-operator-configresolver/expected/openshift-installer-release-4.2-golang111.jsontest/integration/ci-operator-configresolver/expected/openshift-installer-release-4.2-regChange.jsontest/integration/ci-operator-configresolver/expected/openshift-installer-release-4.2.jsontest/integration/ci-operator-configresolver/expected/resolved-config.jsontest/integration/pj-rehearse/expected.yaml
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
openshift/release(manual)openshift/ci-docs(manual)openshift/release-controller(manual)openshift/ci-chat-bot(manual)
💤 Files with no reviewable changes (2)
- cmd/registry-replacer/testdata/zz_fixture_TestReplacer_Unused_base_images_are_pruned.yaml
- test/integration/ci-operator-configresolver/expected/resolved-config.json
✅ Files skipped from review due to trivial changes (1)
- test/integration/ci-operator-configresolver/expected/openshift-installer-console-merged-release-4.2-injected.json
🚧 Files skipped from review as they are similar to previous changes (21)
- test/integration/ci-operator-configresolver/expected/openshift-installer-release-4.2-regChange.json
- test/integration/ci-operator-configresolver/expected/openshift-installer-release-4.2-golang111.json
- pkg/validation/test.go
- test/integration/ci-operator-configresolver/expected/openshift-installer-console-merged-release-4.2-installer-as-base.json
- test/integration/ci-operator-configresolver/expected/openshift-installer-release-4.2.json
- pkg/prowgen/jobbase.go
- pkg/validation/test_test.go
- pkg/steps/multi_stage/multi_stage_test.go
- pkg/steps/ip_pool.go
- pkg/steps/lease.go
- pkg/registry/resolver.go
- pkg/steps/multi_stage/gen_test.go
- pkg/api/leases_test.go
- test/integration/pj-rehearse/expected.yaml
- pkg/steps/ip_pool_test.go
- pkg/defaults/defaults_test.go
- pkg/steps/lease_test.go
- pkg/steps/multi_stage/multi_stage.go
- pkg/api/types.go
- pkg/api/parameters.go
- pkg/registry/resolver_test.go
Contributor
|
@danilo-gemoli: 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. |
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.
This is part of some preparatory work to move cluster profiles definition out of the code base.
The idea of this PR is to let
ci-operator-configresolvedresolving thecluster_profilestanza into the newcluster_profile_literalone.The following test:
will be expanded into:
This allow tests to use cluster profiles defined solely into the configuration /ci-operator/step-registry/cluster-profiles/cluster-profiles-config.yaml rather than into the code.
Another noticeable change concerns
type Parameters interfacethat has now gained three new functions:(1) is a drop-in replacement of the old func (p *Parameters) Get(name string) (string, error).
(2) allows client to retrieve parameters in a type unsafe manner.
(3) leverages (2) to retrieve parameters in a type safe (this time) manner.
Note that functions (2) and (3) shall be merged together if the Golang team will ever accept the "Generic Methods" proposal golang/go#77273
Summary
This PR makes ci-operator's config resolution expand a test's cluster_profile string reference into a structured cluster_profile_literal object populated from cluster-profile configuration loaded from the registry. It also changes the Parameters API to support untyped retrieval, an explicit string accessor, generic typed retrieval, and distinct parameter error types so structured profile details can be passed through parameters safely.
What this means for CI users / operators
Components affected and behavioral changes
Registry resolver (pkg/registry)
Loading & CLI tools (cmd/*, pkg/load, pkg/rehearse, pkg/defaults, release tooling)
API, types & validation (pkg/api, pkg/validation)
Steps, leases & IP-pool logic (pkg/steps, pkg/api/leases)
Parameters API (pkg/api/parameters.go and callers)
Tests & fixtures
Practical impact
Review guidance