Skip to content

ci-operator: Generic DeferredParameters #5241

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
openshift-merge-bot merged 1 commit into from
Jun 10, 2026
Merged

ci-operator: Generic DeferredParameters #5241

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion pkg/api/graph.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ func setMultiArchForChildren(node *StepNode) {
type InputDefinition []string

// +k8s:deepcopy-gen=false
type ParameterMap map[string]func() (string, error)
type ParameterMap map[string]func() (any, error)

@coderabbitai coderabbitai Bot Jun 10, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy lift

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


// StepLink abstracts the types of links that steps
// require and create.
Expand Down
70 changes: 25 additions & 45 deletions pkg/api/parameters.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,56 +16,26 @@ type Parameters interface {
Get(name string) (string, error)
}

type overrideParameters struct {
params Parameters
overrides map[string]string
}

func (p *overrideParameters) Has(name string) bool {
if _, ok := p.overrides[name]; ok {
return true
}
return p.params.Has(name)
}

func (p *overrideParameters) HasInput(name string) bool {
return p.params.HasInput(name)
}

func (p *overrideParameters) Get(name string) (string, error) {
if value, ok := p.overrides[name]; ok {
return value, nil
}
return p.params.Get(name)
}

func NewOverrideParameters(params Parameters, overrides map[string]string) Parameters {
return &overrideParameters{
params: params,
overrides: overrides,
}
}

// +k8s:deepcopy-gen=false
type DeferredParameters struct {
lock sync.Mutex
params Parameters
fns ParameterMap
values map[string]string
fns map[string]func() (any, error)
values map[string]any
}

func NewDeferredParameters(params Parameters) *DeferredParameters {
return &DeferredParameters{
params: params,
fns: make(ParameterMap),
values: make(map[string]string),
fns: make(map[string]func() (any, error)),
values: make(map[string]any),
}
}

func (p *DeferredParameters) Map() (map[string]string, error) {
func (p *DeferredParameters) Map() (map[string]any, error) {
p.lock.Lock()
defer p.lock.Unlock()
m := make(map[string]string, len(p.fns))
m := make(map[string]any, len(p.fns))
for k, fn := range p.fns {
if v, ok := p.values[k]; ok {
m[k] = v
Expand All @@ -81,7 +51,7 @@ func (p *DeferredParameters) Map() (map[string]string, error) {
return m, nil
}

func (p *DeferredParameters) Set(name, value string) {
func (p *DeferredParameters) Set(name string, value any) {
p.lock.Lock()
defer p.lock.Unlock()
if _, ok := p.fns[name]; ok {
Expand All @@ -95,7 +65,7 @@ func (p *DeferredParameters) Set(name, value string) {
p.values[name] = value
}

func (p *DeferredParameters) Add(name string, fn func() (string, error)) {
func (p *DeferredParameters) Add(name string, fn func() (any, error)) {
p.lock.Lock()
defer p.lock.Unlock()
if _, ok := p.fns[name]; ok {
Expand Down Expand Up @@ -141,34 +111,44 @@ func (p *DeferredParameters) has(name string) bool {
}

func (p *DeferredParameters) Get(name string) (string, error) {
if ret, err := p.get(name); ret != "" {
return ret, nil
} else if err != nil {
return ret, err
ret, err := p.get(name)
if err != nil {
return "", err
}

if retStr, ok := ret.(string); ok && retStr != "" {
return retStr, nil
}

if p.params != nil {
return p.params.Get(name)
}

return "", nil
}

func (p *DeferredParameters) get(name string) (string, error) {
func (p *DeferredParameters) get(name string) (any, error) {
p.lock.Lock()
defer p.lock.Unlock()

if value, ok := p.values[name]; ok {
return value, nil
}

if value, ok := os.LookupEnv(name); ok {
p.values[name] = value
return value, nil
}

if fn, ok := p.fns[name]; ok {
value, err := fn()
if err != nil {
return "", fmt.Errorf("could not lazily evaluate deferred parameter %q: %w", name, err)
return nil, fmt.Errorf("could not lazily evaluate deferred parameter %q: %w", name, err)
}

p.values[name] = value
return value, nil
}
return "", nil

return nil, nil
}
90 changes: 45 additions & 45 deletions pkg/api/parameters_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,51 +11,51 @@ func TestDeferredParametersMap(t *testing.T) {
var testCases = []struct {
purpose string
dp *DeferredParameters
expected map[string]string
expected map[string]any
}{{
purpose: "values[N]=V, fns[N] is not set, so returned map does not have key 'N'",
dp: &DeferredParameters{
values: map[string]string{"K1": "V1"},
fns: map[string]func() (string, error){},
values: map[string]any{"K1": "V1"},
fns: map[string]func() (any, error){},
},
expected: map[string]string{},
expected: map[string]any{},
}, {
purpose: "fns[N] is set, values[N] is not, so returned map has key 'N' set to fns[N]()",
dp: &DeferredParameters{
values: map[string]string{},
fns: map[string]func() (string, error){"K1": func() (string, error) { return "V1", nil }},
values: map[string]any{},
fns: map[string]func() (any, error){"K1": func() (any, error) { return "V1", nil }},
},
expected: map[string]string{"K1": "V1"},
expected: map[string]any{"K1": "V1"},
}, {
purpose: "fns[N] is set, values[N] is set, so returned map has key 'N' set to values[N]",
dp: &DeferredParameters{
values: map[string]string{"K1": "V1"},
fns: map[string]func() (string, error){"K1": func() (string, error) { return "F1", nil }},
values: map[string]any{"K1": "V1"},
fns: map[string]func() (any, error){"K1": func() (any, error) { return "F1", nil }},
},
expected: map[string]string{"K1": "V1"},
expected: map[string]any{"K1": "V1"},
}, {
purpose: "returned map contains all names",
dp: &DeferredParameters{
values: map[string]string{"K1": "V1", "K2": "V2"},
fns: map[string]func() (string, error){
"K1": func() (string, error) { return "should not be returned", nil },
"K2": func() (string, error) { return "should not be returned", nil },
"K3": func() (string, error) { return "F3", nil },
"K4": func() (string, error) { return "F4", nil },
values: map[string]any{"K1": "V1", "K2": "V2"},
fns: map[string]func() (any, error){
"K1": func() (any, error) { return "should not be returned", nil },
"K2": func() (any, error) { return "should not be returned", nil },
"K3": func() (any, error) { return "F3", nil },
"K4": func() (any, error) { return "F4", nil },
},
},
expected: map[string]string{"K1": "V1", "K2": "V2", "K3": "F3", "K4": "F4"},
expected: map[string]any{"K1": "V1", "K2": "V2", "K3": "F3", "K4": "F4"},
}, {
purpose: "parent values are not returned",
dp: &DeferredParameters{
params: &DeferredParameters{
values: map[string]string{"K1": "V1"},
fns: map[string]func() (string, error){
"K2": func() (string, error) { return "V2", nil },
values: map[string]any{"K1": "V1"},
fns: map[string]func() (any, error){
"K2": func() (any, error) { return "V2", nil },
},
},
},
expected: map[string]string{},
expected: map[string]any{},
}}

for _, tc := range testCases {
Expand Down Expand Up @@ -89,7 +89,7 @@ func TestDeferredParametersGetSet(t *testing.T) {
purpose: "Existing key is not overwritten",
dp: &DeferredParameters{
fns: make(ParameterMap),
values: map[string]string{"key": "oldValue"},
values: map[string]any{"key": "oldValue"},
},
name: "key",
callSet: true,
Expand All @@ -100,10 +100,10 @@ func TestDeferredParametersGetSet(t *testing.T) {
}, {
purpose: "Existing key is not set if lazy evaluation func is set",
dp: &DeferredParameters{
fns: map[string]func() (string, error){
"key": func() (string, error) { return "lazyValue", nil },
fns: map[string]func() (any, error){
"key": func() (any, error) { return "lazyValue", nil },
},
values: map[string]string{},
values: map[string]any{},
},
name: "key",
callSet: true,
Expand Down Expand Up @@ -139,55 +139,55 @@ func TestDeferredParametersParent(t *testing.T) {
}{{
name: "values, no parent",
params: &DeferredParameters{
values: map[string]string{"K": "expected"},
fns: map[string]func() (string, error){},
values: map[string]any{"K": "expected"},
fns: map[string]func() (any, error){},
},
}, {
name: "fns, no parent",
params: &DeferredParameters{
values: map[string]string{},
fns: map[string]func() (string, error){
"K": func() (string, error) { return "expected", nil },
values: map[string]any{},
fns: map[string]func() (any, error){
"K": func() (any, error) { return "expected", nil },
},
},
}, {
name: "values, parent",
params: &DeferredParameters{
values: map[string]string{"K": "expected"},
fns: map[string]func() (string, error){},
values: map[string]any{"K": "expected"},
fns: map[string]func() (any, error){},
params: &DeferredParameters{
values: map[string]string{"K": "unexpected"},
values: map[string]any{"K": "unexpected"},
},
},
}, {
name: "fns, parent",
params: &DeferredParameters{
values: map[string]string{},
fns: map[string]func() (string, error){
"K": func() (string, error) { return "expected", nil },
values: map[string]any{},
fns: map[string]func() (any, error){
"K": func() (any, error) { return "expected", nil },
},
params: &DeferredParameters{
values: map[string]string{"K": "unexpected"},
values: map[string]any{"K": "unexpected"},
},
},
}, {
name: "from parent's values",
params: &DeferredParameters{
values: map[string]string{},
fns: map[string]func() (string, error){},
values: map[string]any{},
fns: map[string]func() (any, error){},
params: &DeferredParameters{
values: map[string]string{"K": "expected"},
values: map[string]any{"K": "expected"},
},
},
}, {
name: "from parent's fns",
params: &DeferredParameters{
values: map[string]string{},
fns: map[string]func() (string, error){},
values: map[string]any{},
fns: map[string]func() (any, error){},
params: &DeferredParameters{
values: map[string]string{},
fns: map[string]func() (string, error){
"K": func() (string, error) { return "expected", nil },
values: map[string]any{},
fns: map[string]func() (any, error){
"K": func() (any, error) { return "expected", nil },
},
},
},
Expand Down
12 changes: 6 additions & 6 deletions pkg/defaults/defaults.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,11 +94,11 @@ func fromConfig(ctx context.Context, cfg *Config) ([]api.Step, []api.Step, error
for _, target := range cfg.RequiredTargets {
requiredNames.Insert(target)
}
cfg.params.Add("JOB_NAME", func() (string, error) { return cfg.JobSpec.Job, nil })
cfg.params.Add("JOB_NAME_HASH", func() (string, error) { return cfg.JobSpec.JobNameHash(), nil })
cfg.params.Add("JOB_NAME_SAFE", func() (string, error) { return strings.Replace(cfg.JobSpec.Job, "_", "-", -1), nil })
cfg.params.Add("UNIQUE_HASH", func() (string, error) { return cfg.JobSpec.UniqueHash(), nil })
cfg.params.Add("NAMESPACE", func() (string, error) { return cfg.JobSpec.Namespace(), nil })
cfg.params.Add("JOB_NAME", func() (any, error) { return cfg.JobSpec.Job, nil })
cfg.params.Add("JOB_NAME_HASH", func() (any, error) { return cfg.JobSpec.JobNameHash(), nil })
cfg.params.Add("JOB_NAME_SAFE", func() (any, error) { return strings.Replace(cfg.JobSpec.Job, "_", "-", -1), nil })
cfg.params.Add("UNIQUE_HASH", func() (any, error) { return cfg.JobSpec.UniqueHash(), nil })
cfg.params.Add("NAMESPACE", func() (any, error) { return cfg.JobSpec.Namespace(), nil })
inputImages := make(inputImageSet)
var overridableSteps []api.Step
var buildSteps []api.Step
Expand Down Expand Up @@ -1082,7 +1082,7 @@ func sourceStepForRef(ref *prowapi.Refs, primaryRef bool) api.StepConfiguration
}}
}

func paramsHasAllParametersAsInput(p api.Parameters, params map[string]func() (string, error)) (map[string]string, bool) {
func paramsHasAllParametersAsInput(p api.Parameters, params map[string]func() (any, error)) (map[string]string, bool) {
if len(params) == 0 {
return nil, false
}
Expand Down
Loading