ci-operator: Generic DeferredParameters#5241
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
danilo-gemoli
changed the title
|
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 (25)
🔗 Linked repositories identifiedCodeRabbit considers these linked repositories for cross-repo context during reviews:
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (20)
📝 WalkthroughWalkthroughThis PR widens parameter providers from returning string to any, refactors DeferredParameters to cache any-typed values and evaluators, adapts all step providers and tests to the new shape, and updates the parameter writer to only serialize string values. ChangesParameter System Type Widening
🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 15 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (15 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)Command failed 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
🧹 Nitpick comments (3)
pkg/api/parameters.go (1)
113-125: ⚡ Quick win
Get()silently ignores non-string parameter values.The condition
if ret, err := p.get(name); ret != ""(line 114) compares anany-typed value to"". For non-string values (e.g.,int,bool), this comparison returnsfalse, causing the code to skip the type assertion block and fall through to the parent parameters check or return empty string. Non-string parameters registered viaAdd()are effectively unretrievable throughGet().This appears intentional—
Get()maintains its(string, error)contract for backward compatibility whileMap()provides access to all value types. However, the silent failure mode could confuse callers who register non-string parameters but attempt retrieval viaGet(). Consider documenting this behavior in the method's godoc comment.🤖 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 113 - 125, Update the godoc for DeferredParameters.Get to explicitly state that Get returns only string-typed parameter values and will ignore non-string values registered via Add (i.e., values stored via DeferredParameters.get are type-asserted to string), and recommend using DeferredParameters.Map or calling Get on the underlying params (DeferredParameters.params) for non-string retrieval; reference DeferredParameters.Get, DeferredParameters.Add, DeferredParameters.get, DeferredParameters.Map and the params field in the comment so callers understand why non-string values are not returned and what to use instead.Source: Coding guidelines
pkg/api/parameters_test.go (1)
70-132: ⚡ Quick winConsider adding test coverage for
Get()with non-string parameter values.The test suite covers
Get()andSet()with string values and lazy evaluation, but doesn't verify the behavior whenGet()is called on a parameter registered with a non-string value (e.g., an integer or boolean). Adding a test case that registersAdd("int_param", func() (any, error) { return 42, nil })and verifiesGet("int_param")returns""would document the silent-ignore behavior and prevent future regressions.🤖 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_test.go` around lines 70 - 132, Add a test case to TestDeferredParametersGetSet that registers a non-string lazy value and asserts Get returns the empty string and nil error: create a DeferredParameters via NewDeferredParameters(nil), call dp.Add("int_param", func() (any, error) { return 42, nil }) (or populate fns with "int_param": func() (any,error){return 42,nil}), set name to "int_param", leave callSet false, expect getValue "" and getError nil, and include a descriptive purpose like "Non-string lazy value returns empty string" so the test exercises DeferredParameters.Get and guards against regressions for non-string results.pkg/steps/write_params_test.go (1)
14-15: ⚡ Quick winAdd test coverage for non-string parameter skipping.
The test verifies string parameters are written correctly, but doesn't verify that non-string parameters are skipped as intended by the type assertion logic in
write_params.go:45-48.🧪 Proposed test addition
Add a non-string parameter and verify it's not written:
params.Add("K1", func() (any, error) { return "V1", nil }) params.Add("K2", func() (any, error) { return "V:2", nil }) + params.Add("K3", func() (any, error) { return 42, nil })Then verify the output only contains K1 and K2 (K3 skipped).
🤖 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/steps/write_params_test.go` around lines 14 - 15, Add a test case in pkg/steps/write_params_test.go that registers a non-string parameter using params.Add (e.g., params.Add("K3", func() (any, error) { return 123, nil })) then call the same code path that writes params (the test already exercising params.Add for "K1"/"K2") and assert the resulting output contains only K1 and K2 and does not contain K3; this verifies the type-assertion logic in write_params.go (lines ~45-48) correctly skips non-string values.
🤖 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/graph.go`:
- Line 108: The change widened ParameterMap's value signature from func()
(string, error) to func() (any, error), which breaks the public API; restore
backward compatibility by reverting ParameterMap to map[string]func() (string,
error) and, if generic behavior is needed, introduce a new distinct type (e.g.,
ParameterMapAny map[string]func() (any, error)) and update internal callers to
use the new type where safe; ensure all functions, methods, and tests that
referenced ParameterMap (search for ParameterMap, NewParameterMap,
ParseParameters, etc.) are updated to use the new name when they expect
any-typed values so existing vendored consumers keep the original string-typed
contract.
In `@pkg/steps/write_params.go`:
- Around line 45-48: The loop currently does vStr, ok := v.(string) and silently
continues on !ok; update that branch to log a warning or debug message (e.g.,
using logrus.Warnf or Debugf) that includes the parameter key and the actual
type/value so operators can see why a parameter was skipped; ensure the package
imports logrus if missing and place the log inside the same loop where vStr and
ok are checked (referencing vStr, ok and the loop's parameter key variable,
e.g., k) before continuing.
---
Nitpick comments:
In `@pkg/api/parameters_test.go`:
- Around line 70-132: Add a test case to TestDeferredParametersGetSet that
registers a non-string lazy value and asserts Get returns the empty string and
nil error: create a DeferredParameters via NewDeferredParameters(nil), call
dp.Add("int_param", func() (any, error) { return 42, nil }) (or populate fns
with "int_param": func() (any,error){return 42,nil}), set name to "int_param",
leave callSet false, expect getValue "" and getError nil, and include a
descriptive purpose like "Non-string lazy value returns empty string" so the
test exercises DeferredParameters.Get and guards against regressions for
non-string results.
In `@pkg/api/parameters.go`:
- Around line 113-125: Update the godoc for DeferredParameters.Get to explicitly
state that Get returns only string-typed parameter values and will ignore
non-string values registered via Add (i.e., values stored via
DeferredParameters.get are type-asserted to string), and recommend using
DeferredParameters.Map or calling Get on the underlying params
(DeferredParameters.params) for non-string retrieval; reference
DeferredParameters.Get, DeferredParameters.Add, DeferredParameters.get,
DeferredParameters.Map and the params field in the comment so callers understand
why non-string values are not returned and what to use instead.
In `@pkg/steps/write_params_test.go`:
- Around line 14-15: Add a test case in pkg/steps/write_params_test.go that
registers a non-string parameter using params.Add (e.g., params.Add("K3", func()
(any, error) { return 123, nil })) then call the same code path that writes
params (the test already exercising params.Add for "K1"/"K2") and assert the
resulting output contains only K1 and K2 and does not contain K3; this verifies
the type-assertion logic in write_params.go (lines ~45-48) correctly skips
non-string values.
🪄 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: 47a6e0d5-7a99-4784-a989-3e03fbeab4b3
📒 Files selected for processing (25)
pkg/api/graph.gopkg/api/parameters.gopkg/api/parameters_test.gopkg/defaults/defaults.gopkg/defaults/defaults_test.gopkg/steps/input_image_tag.gopkg/steps/ip_pool.gopkg/steps/ip_pool_test.gopkg/steps/lease.gopkg/steps/lease_proxy.gopkg/steps/lease_proxy_test.gopkg/steps/lease_test.gopkg/steps/multi_stage/gen_test.gopkg/steps/multi_stage/multi_stage_test.gopkg/steps/output_image_tag.gopkg/steps/pipeline_image_cache.gopkg/steps/project_image.gopkg/steps/release/create_release.gopkg/steps/release/import_release.gopkg/steps/release/release_images.gopkg/steps/rpm_server.gopkg/steps/rpm_server_test.gopkg/steps/source.gopkg/steps/write_params.gopkg/steps/write_params_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)
|
|
||
| // +k8s:deepcopy-gen=false | ||
| type ParameterMap map[string]func() (string, error) | ||
| type ParameterMap map[string]func() (any, error) |
There was a problem hiding this comment.
Breaking API change impacts vendored consumers in linked repositories.
Widening ParameterMap value functions from (string, error) to (any, error) breaks the API contract. Linked repositories research shows openshift/release-controller and openshift/ci-chat-bot vendor the old ParameterMap type with string-typed signatures in their vendor/github.com/openshift/ci-tools/pkg/api/graph.go and parameters.go. Those repos will need to re-vendor and adapt their code to the new any-typed contract.
Per coding guidelines for pkg/api/**: "Backward compatibility matters — new fields should have zero-value defaults that preserve existing behavior." While this technically changes an existing type rather than adding a field, it still violates the backward compatibility requirement for the ci-operator configuration API.
🤖 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/graph.go` at line 108, The change widened ParameterMap's value
signature from func() (string, error) to func() (any, error), which breaks the
public API; restore backward compatibility by reverting ParameterMap to
map[string]func() (string, error) and, if generic behavior is needed, introduce
a new distinct type (e.g., ParameterMapAny map[string]func() (any, error)) and
update internal callers to use the new type where safe; ensure all functions,
methods, and tests that referenced ParameterMap (search for ParameterMap,
NewParameterMap, ParseParameters, etc.) are updated to use the new name when
they expect any-typed values so existing vendored consumers keep the original
string-typed contract.
Sources: Coding guidelines, MCP tools
danilo-gemoli
force-pushed
the
chore/ci-operator/generic-params
branch
from
83ff744 to
b7f867b
Compare
|
/test e2e |
danilo-gemoli
force-pushed
the
chore/ci-operator/generic-params
branch
from
b7f867b to
0e5caee
Compare
|
/test e2e |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: danilo-gemoli, jmguzik 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 |
|
Tests from second stage were triggered manually. Pipeline can be controlled only manually, until HEAD changes. Use command to trigger second stage. |
|
@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
Allow
DeferredParametersto store generic objectsanyrather thanstringsolely.The semantic of the main get method:
doesn't change: if it exists and is a
stringtype, return a parameter stored with namename.This is part of some preparatory work to move cluster profiles definition out of the code base.
Overview
This PR generalizes ci-operator's parameter system so deferred parameters can hold arbitrary values (any) instead of only strings. It's preparatory work to move cluster profile definitions (and other richer parameter data) out of the codebase and into configurable inputs.
What changed in practice
Components affected and behavioral impact
Why this matters to CI users and operators
Backward compatibility and migration notes
Tests & risk