prowgen: allow ci-operator config to override .config.prowgen#5119
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
WalkthroughAdds configuration to disable Prow rehearsals at the release level and per-test level, updates job generation to honor these overrides, and adds tests plus fixtures to validate the behavior. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 10 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (10 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
Bot
added
the
approved
| } | ||
|
|
||
| // ProwgenOverrides holds prowgen-related settings from the ci-operator config | ||
| // that override the corresponding .config.prowgen values. |
|
|
||
| // ProwgenOverrides holds prowgen-related settings from the ci-operator config | ||
| // that override the corresponding .config.prowgen values. | ||
| type ProwgenOverrides struct { |
There was a problem hiding this comment.
No need to introduce this as an override option.
| Metadata Metadata `json:"zz_generated_metadata"` | ||
|
|
||
| // Prowgen holds prowgen-related overrides from the ci-operator config that | ||
| // take precedence over .config.prowgen values. |
There was a problem hiding this comment.
The comment is irrelevant as well. This stanza should include the prowgen options either way. The config.prowgen is not related now, and also it will be deprecated.
| NodeArchitecture NodeArchitecture `json:"node_architecture,omitempty"` | ||
|
|
||
| // DisableRehearsal prevents this specific test from being picked up for rehearsals. | ||
| DisableRehearsal *bool `json:"disable_rehearsal,omitempty"` |
There was a problem hiding this comment.
Any particular reason why this is a pointer?
|
|
||
| // ApplyOverrides applies ci-operator config overrides on top of .config.prowgen values. | ||
| // Fields set in the ci-operator config take precedence. | ||
| func (p *Prowgen) ApplyOverrides(overrides *cioperatorapi.ProwgenOverrides) { |
There was a problem hiding this comment.
There are no overrides. The logic already exists, and it already overrides different layers. You should just mutate that logic and add the ci-op config in the equation.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/prowgen/prowgen_test.go (1)
689-732: Add an explicitdisable_rehearsals: falseprecedence case.The new cases are good, but they only validate the
truepath. Please add a table case where repo config hasrehearsals.disable_all: trueand ci-operator setsprowgen.disable_rehearsals: false, so precedence is verified in both directions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/prowgen/prowgen_test.go` around lines 689 - 732, Add a test table case that verifies ci-operator false overrides repo config true: create a new entry in the existing test cases (similar style to the surrounding cases) with id like "ci-operator config false takes precedence over prowgen config", set config to &ciop.ReleaseBuildConfiguration{Prowgen: &ciop.ProwgenOverrides{DisableRehearsals: utilpointer.Bool(false)}, Tests: [...] } and set repoInfo to &ProwgenInfo{Config: config.Prowgen{Rehearsals: config.Rehearsals{DisableAll: true}}, Metadata: ciop.Metadata{Org:"organization",Repo:"repository",Branch:"branch"}}; assert the resulting behavior matches ci-operator taking precedence (i.e., rehearsals not globally disabled). Ensure you use the same test field names (Prowgen.DisableRehearsals and ProwgenInfo.Config.Rehearsals.DisableAll) so the table covers both true→false precedence directions.
🤖 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/config/load.go`:
- Around line 107-113: The ApplyOverrides method currently only applies
overrides.DisableRehearsals when true, so an explicit false cannot clear
p.Rehearsals.DisableAll; change the logic in Prowgen.ApplyOverrides to check for
non-nil overrides.DisableRehearsals and set p.Rehearsals.DisableAll =
*overrides.DisableRehearsals (assign the dereferenced boolean directly) so the
repo-level value is properly overridden by both true and false from ci-operator.
---
Nitpick comments:
In `@pkg/prowgen/prowgen_test.go`:
- Around line 689-732: Add a test table case that verifies ci-operator false
overrides repo config true: create a new entry in the existing test cases
(similar style to the surrounding cases) with id like "ci-operator config false
takes precedence over prowgen config", set config to
&ciop.ReleaseBuildConfiguration{Prowgen:
&ciop.ProwgenOverrides{DisableRehearsals: utilpointer.Bool(false)}, Tests: [...]
} and set repoInfo to &ProwgenInfo{Config: config.Prowgen{Rehearsals:
config.Rehearsals{DisableAll: true}}, Metadata:
ciop.Metadata{Org:"organization",Repo:"repository",Branch:"branch"}}; assert the
resulting behavior matches ci-operator taking precedence (i.e., rehearsals not
globally disabled). Ensure you use the same test field names
(Prowgen.DisableRehearsals and ProwgenInfo.Config.Rehearsals.DisableAll) so the
table covers both true→false precedence directions.
🪄 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: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 14b94b5c-a84c-47b8-a871-28e3c2731288
📒 Files selected for processing (7)
pkg/api/types.gopkg/config/load.gopkg/prowgen/prowgen.gopkg/prowgen/prowgen_test.gopkg/prowgen/testdata/zz_fixture_TestGenerateJobs_ci_operator_config_overrides_prowgen_rehearsals.yamlpkg/prowgen/testdata/zz_fixture_TestGenerateJobs_ci_operator_config_takes_precedence_over_prowgen_config.yamlpkg/prowgen/testdata/zz_fixture_TestGenerateJobs_per_test_disable_rehearsal_from_ci_operator_config.yaml
| func (p *Prowgen) ApplyOverrides(overrides *cioperatorapi.ProwgenOverrides) { | ||
| if overrides == nil { | ||
| return | ||
| } | ||
| if overrides.DisableRehearsals != nil && *overrides.DisableRehearsals { | ||
| p.Rehearsals.DisableAll = true | ||
| } |
There was a problem hiding this comment.
DisableRehearsals precedence is currently one-way (true-only).
On Line 111, overrides only apply when the value is true. That means an explicit ci-operator disable_rehearsals: false cannot override repo-level rehearsals.disable_all: true, which breaks full precedence semantics.
💡 Proposed fix
func (p *Prowgen) ApplyOverrides(overrides *cioperatorapi.ProwgenOverrides) {
if overrides == nil {
return
}
- if overrides.DisableRehearsals != nil && *overrides.DisableRehearsals {
- p.Rehearsals.DisableAll = true
- }
+ if overrides.DisableRehearsals != nil {
+ p.Rehearsals.DisableAll = *overrides.DisableRehearsals
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (p *Prowgen) ApplyOverrides(overrides *cioperatorapi.ProwgenOverrides) { | |
| if overrides == nil { | |
| return | |
| } | |
| if overrides.DisableRehearsals != nil && *overrides.DisableRehearsals { | |
| p.Rehearsals.DisableAll = true | |
| } | |
| func (p *Prowgen) ApplyOverrides(overrides *cioperatorapi.ProwgenOverrides) { | |
| if overrides == nil { | |
| return | |
| } | |
| if overrides.DisableRehearsals != nil { | |
| p.Rehearsals.DisableAll = *overrides.DisableRehearsals | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/config/load.go` around lines 107 - 113, The ApplyOverrides method
currently only applies overrides.DisableRehearsals when true, so an explicit
false cannot clear p.Rehearsals.DisableAll; change the logic in
Prowgen.ApplyOverrides to check for non-nil overrides.DisableRehearsals and set
p.Rehearsals.DisableAll = *overrides.DisableRehearsals (assign the dereferenced
boolean directly) so the repo-level value is properly overridden by both true
and false from ci-operator.
Prucek
force-pushed
the
prowgen-overrides
branch
2 times, most recently
from
1b8b902 to
838c022
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/prowgen/prowgen_test.go (1)
703-718: Add the inverse precedence case (falseovertrue) for completeness.Line 704 validates
trueprecedence over repo config, but there is no paired assertion for explicitDisableRehearsals: falseoverriding repoDisableAll: true. Adding that case will lock in both branches of precedence semantics.🧪 Suggested test case addition
+ { + id: "ci-operator config false takes precedence over repo disable-all true", + config: &ciop.ReleaseBuildConfiguration{ + Prowgen: &ciop.ProwgenOverrides{DisableRehearsals: utilpointer.Bool(false)}, + Tests: []ciop.TestStepConfiguration{ + {As: "unit", ContainerTestConfiguration: &ciop.ContainerTestConfiguration{From: "bin"}}, + }, + }, + repoInfo: &ProwgenInfo{ + Config: config.Prowgen{Rehearsals: config.Rehearsals{DisableAll: true}}, + Metadata: ciop.Metadata{ + Org: "organization", + Repo: "repository", + Branch: "branch", + }}, + },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/prowgen/prowgen_test.go` around lines 703 - 718, Add a complementary test case to verify the inverse precedence: create a ciop.ReleaseBuildConfiguration with Prowgen: &ciop.ProwgenOverrides{DisableRehearsals: utilpointer.Bool(false)} and a ProwgenInfo whose Config.Prowgen.Rehearsals.DisableAll is true (and similar Metadata/Tests as the existing case) and assert that the explicit DisableRehearsals=false in the job config takes precedence over the repo-level DisableAll=true; update the test table in prowgen_test.go (the entries using ciop.ReleaseBuildConfiguration, ciop.ProwgenOverrides, ProwgenInfo and config.Prowgen.Rehearsals.DisableAll) by adding this case so both true->false and false->true precedence branches are covered.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/prowgen/prowgen_test.go`:
- Around line 703-718: Add a complementary test case to verify the inverse
precedence: create a ciop.ReleaseBuildConfiguration with Prowgen:
&ciop.ProwgenOverrides{DisableRehearsals: utilpointer.Bool(false)} and a
ProwgenInfo whose Config.Prowgen.Rehearsals.DisableAll is true (and similar
Metadata/Tests as the existing case) and assert that the explicit
DisableRehearsals=false in the job config takes precedence over the repo-level
DisableAll=true; update the test table in prowgen_test.go (the entries using
ciop.ReleaseBuildConfiguration, ciop.ProwgenOverrides, ProwgenInfo and
config.Prowgen.Rehearsals.DisableAll) by adding this case so both true->false
and false->true precedence branches are covered.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 6186bea1-e55d-4ea5-aaa6-0ccdb9ed9533
📒 Files selected for processing (6)
pkg/api/types.gopkg/prowgen/prowgen.gopkg/prowgen/prowgen_test.gopkg/prowgen/testdata/zz_fixture_TestGenerateJobs_ci_operator_config_overrides_prowgen_rehearsals.yamlpkg/prowgen/testdata/zz_fixture_TestGenerateJobs_ci_operator_config_takes_precedence_over_prowgen_config.yamlpkg/prowgen/testdata/zz_fixture_TestGenerateJobs_per_test_disable_rehearsal_from_ci_operator_config.yaml
✅ Files skipped from review due to trivial changes (3)
- pkg/prowgen/testdata/zz_fixture_TestGenerateJobs_ci_operator_config_takes_precedence_over_prowgen_config.yaml
- pkg/prowgen/testdata/zz_fixture_TestGenerateJobs_ci_operator_config_overrides_prowgen_rehearsals.yaml
- pkg/prowgen/testdata/zz_fixture_TestGenerateJobs_per_test_disable_rehearsal_from_ci_operator_config.yaml
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/prowgen/prowgen.go
- pkg/api/types.go
Add ProwgenOverrides to ci-operator config so that it takes precedence over all other prowgen configuration layers (org-level and repo-level .config.prowgen files). This is the first step toward deprecating .config.prowgen by allowing its features to be controlled directly from ci-operator config. - Add prowgen field to ReleaseBuildConfiguration with disable_rehearsals - Add per-test disable_rehearsal on TestStepConfiguration - Add ApplyOverrides method as the single merge point where ci-operator config overrides .config.prowgen values Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Prucek
force-pushed
the
prowgen-overrides
branch
from
838c022 to
16a106b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/prowgen/prowgen.go`:
- Around line 46-48: The current check only flips
prowgenConfig.Rehearsals.DisableAll when configSpec.Prowgen.DisableRehearsals is
true, preventing ci-operator from overriding a repo/org default back to false;
change the logic in the block that references configSpec.Prowgen and
prowgenConfig.Rehearsals.DisableAll so that when configSpec.Prowgen is non-nil
you assign prowgenConfig.Rehearsals.DisableAll =
configSpec.Prowgen.DisableRehearsals (i.e. honor true or false from
ci-operator), rather than only setting it on true, ensuring ci-operator takes
precedence.
🪄 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: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 73caf283-2215-4b06-9680-90ff00480781
⛔ 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 (6)
pkg/api/types.gopkg/prowgen/prowgen.gopkg/prowgen/prowgen_test.gopkg/prowgen/testdata/zz_fixture_TestGenerateJobs_ci_operator_config_overrides_prowgen_rehearsals.yamlpkg/prowgen/testdata/zz_fixture_TestGenerateJobs_ci_operator_config_takes_precedence_over_prowgen_config.yamlpkg/prowgen/testdata/zz_fixture_TestGenerateJobs_per_test_disable_rehearsal_from_ci_operator_config.yaml
✅ Files skipped from review due to trivial changes (3)
- pkg/prowgen/testdata/zz_fixture_TestGenerateJobs_ci_operator_config_overrides_prowgen_rehearsals.yaml
- pkg/prowgen/testdata/zz_fixture_TestGenerateJobs_per_test_disable_rehearsal_from_ci_operator_config.yaml
- pkg/prowgen/testdata/zz_fixture_TestGenerateJobs_ci_operator_config_takes_precedence_over_prowgen_config.yaml
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/prowgen/prowgen_test.go
- pkg/api/types.go
| if configSpec.Prowgen != nil && configSpec.Prowgen.DisableRehearsals { | ||
| prowgenConfig.Rehearsals.DisableAll = true | ||
| } |
There was a problem hiding this comment.
disable_rehearsals precedence is currently one-way
On Line 46, the override only applies when ci-operator sets disable_rehearsals: true. That means ci-operator config cannot override repo/org config back to false, which conflicts with “ci-operator takes precedence”.
Proposed fix
- if configSpec.Prowgen != nil && configSpec.Prowgen.DisableRehearsals {
- prowgenConfig.Rehearsals.DisableAll = true
- }
+ if configSpec.Prowgen != nil {
+ prowgenConfig.Rehearsals.DisableAll = configSpec.Prowgen.DisableRehearsals
+ }📝 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.
| if configSpec.Prowgen != nil && configSpec.Prowgen.DisableRehearsals { | |
| prowgenConfig.Rehearsals.DisableAll = true | |
| } | |
| if configSpec.Prowgen != nil { | |
| prowgenConfig.Rehearsals.DisableAll = configSpec.Prowgen.DisableRehearsals | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/prowgen/prowgen.go` around lines 46 - 48, The current check only flips
prowgenConfig.Rehearsals.DisableAll when configSpec.Prowgen.DisableRehearsals is
true, preventing ci-operator from overriding a repo/org default back to false;
change the logic in the block that references configSpec.Prowgen and
prowgenConfig.Rehearsals.DisableAll so that when configSpec.Prowgen is non-nil
you assign prowgenConfig.Rehearsals.DisableAll =
configSpec.Prowgen.DisableRehearsals (i.e. honor true or false from
ci-operator), rather than only setting it on true, ensuring ci-operator takes
precedence.
|
/override ci/prow/images |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: droslean, Prucek 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. |
|
@droslean: Overrode contexts on behalf of droslean: ci/prow/e2e, 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. |
|
@Prucek: 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. |
openshift-merge-bot
Bot
merged commit b6675f5
into
openshift:main
2 participants
Summary
ProwgenOverridesto ci-operator config (prowgenfield onReleaseBuildConfiguration) so that ci-operator config takes precedence over all other prowgen configuration layers (org-level and repo-level.config.prowgenfiles)disable_rehearsalonTestStepConfigurationfor fine-grained rehearsal controlApplyOverridesmethod as the single merge point where ci-operator config overrides.config.prowgenvalues.config.prowgenby allowing its features to be controlled directly from ci-operator configTest plan
.config.prowgendisable_rehearsalgo test ./pkg/prowgen/...)🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests