prowgen: add rehearsal control to ci-operator config#5106
prowgen: add rehearsal control to ci-operator config#5106Prucek wants to merge 2 commits intoopenshift:mainfrom
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: 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 |
openshift-ci
bot
added
the
approved
|
Scheduling tests matching the |
|
Tests from second stage were triggered manually. Pipeline can be controlled only manually, until HEAD changes. Use command to trigger second stage. |
3 similar comments
|
Tests from second stage were triggered manually. Pipeline can be controlled only manually, until HEAD changes. Use command to trigger second stage. |
|
Tests from second stage were triggered manually. Pipeline can be controlled only manually, until HEAD changes. Use command to trigger second stage. |
|
Tests from second stage were triggered manually. Pipeline can be controlled only manually, until HEAD changes. Use command to trigger second stage. |
Prucek
force-pushed
the
prowgen-rehearsals
branch
from
80aa6cf to
68332b0
Compare
WalkthroughThis pull request consolidates Prowgen configuration models by moving type definitions from the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 9 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (9 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![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)
703-718: Consider adding a test for the edge case where global disable cannot be overridden by per-test enable.The documentation in
types.go(lines 872-874) states thatDisableRehearsalcannot re-enable rehearsals if globally disabled. Consider adding a test case to verify this behavior explicitly.💡 Suggested test case
{ id: "per-test enable cannot override global disable", config: &ciop.ReleaseBuildConfiguration{ Prowgen: &ciop.ProwgenExtras{DisableRehearsals: ptr.To(true)}, Tests: []ciop.TestStepConfiguration{ // DisableRehearsal: false should NOT re-enable since global is disabled {As: "unit", DisableRehearsal: ptr.To(false), ContainerTestConfiguration: &ciop.ContainerTestConfiguration{From: "bin"}}, }, }, repoInfo: &ProwgenInfo{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 new test case that verifies a per-test DisableRehearsal=false does not re-enable rehearsals when the global flag is set: create an entry similar to the suggested snippet using ReleaseBuildConfiguration with ProwgenExtras{DisableRehearsals: ptr.To(true)} and a TestStepConfiguration having DisableRehearsal: ptr.To(false) (e.g., As: "unit"), include the same ProwgenInfo Metadata used by other cases, and assert the generated job respects the global disable (i.e., rehearsal remains disabled); update the test table alongside the existing cases that use TestStepConfiguration, ProwgenInfo, Metadata, DisableRehearsal and DisableRehearsals.
🤖 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/testdata/zz_fixture_TestNewProwJobBaseBuilderForTest_multi_stage_test_with_CSI_enabled_from_ci_operator_config.yaml`:
- Around line 38-40: The container mounts a secret named "gcs-credentials" at
mountPath "/secrets/gcs" but there is no corresponding volume in the "volumes"
section; add a volume entry named "gcs-credentials" of type secret (matching
other fixtures) under the top-level volumes so the mount has a defined source
and uses the correct secret name.
---
Nitpick comments:
In `@pkg/prowgen/prowgen_test.go`:
- Around line 703-718: Add a new test case that verifies a per-test
DisableRehearsal=false does not re-enable rehearsals when the global flag is
set: create an entry similar to the suggested snippet using
ReleaseBuildConfiguration with ProwgenExtras{DisableRehearsals: ptr.To(true)}
and a TestStepConfiguration having DisableRehearsal: ptr.To(false) (e.g., As:
"unit"), include the same ProwgenInfo Metadata used by other cases, and assert
the generated job respects the global disable (i.e., rehearsal remains
disabled); update the test table alongside the existing cases that use
TestStepConfiguration, ProwgenInfo, Metadata, DisableRehearsal and
DisableRehearsals.
🪄 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: ed5d27a3-4885-4979-988c-82c74c5007c5
⛔ 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 (10)
cmd/ci-operator-prowgen/main.gopkg/api/types.gopkg/config/load.gopkg/prowgen/jobbase.gopkg/prowgen/prowgen.gopkg/prowgen/prowgen_test.gopkg/prowgen/testdata/zz_fixture_TestGenerateJobs_ci_operator_config_takes_precedence_over_prowgen_config.yamlpkg/prowgen/testdata/zz_fixture_TestGenerateJobs_disabled_rehearsals_from_ci_operator_prowgen_config.yamlpkg/prowgen/testdata/zz_fixture_TestGenerateJobs_per_test_disable_rehearsal_from_ci_operator_config.yamlpkg/prowgen/testdata/zz_fixture_TestNewProwJobBaseBuilderForTest_multi_stage_test_with_CSI_enabled_from_ci_operator_config.yaml
| - mountPath: /secrets/gcs | ||
| name: gcs-credentials | ||
| readOnly: true |
There was a problem hiding this comment.
Missing gcs-credentials volume definition.
The container mounts gcs-credentials at /secrets/gcs (lines 38-40), but the corresponding volume is not defined in the volumes section (lines 57-81). Other fixtures typically include this volume as a secret.
🔧 Proposed fix to add the missing volume
volumes:
- name: boskos
secret:
items:
- key: credentials
path: credentials
secretName: boskos-credentials
+ - name: gcs-credentials
+ secret:
+ secretName: gce-sa-credentials-gcs-publisher
- configMap:
name: gsm-config
name: gsm-configAlso applies to: 57-81
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@pkg/prowgen/testdata/zz_fixture_TestNewProwJobBaseBuilderForTest_multi_stage_test_with_CSI_enabled_from_ci_operator_config.yaml`
around lines 38 - 40, The container mounts a secret named "gcs-credentials" at
mountPath "/secrets/gcs" but there is no corresponding volume in the "volumes"
section; add a volume entry named "gcs-credentials" of type secret (matching
other fixtures) under the top-level volumes so the mount has a defined source
and uses the correct secret name.
| RestrictNetworkAccess *bool `json:"restrict_network_access,omitempty"` | ||
|
|
||
| // DisableRehearsal prevents this specific test from being picked up for rehearsals. | ||
| // Note: this cannot re-enable rehearsals if they are globally disabled via |
There was a problem hiding this comment.
Well, the note here is irrelevant since you will depreciate that. Also, we need to override it. So if its globaly disabled, then enabling it here should override the global setting. So the comment is wrong too.
| } | ||
|
|
||
| // ApplyOverrides applies overrides from ci-operator config's prowgen stanza. | ||
| // Fields set in the ci-operator config take precedence over .config.prowgen values. |
There was a problem hiding this comment.
The comments are irrelevant. Same for the other. Just remove them all.
| "time" | ||
|
|
||
| utilpointer "k8s.io/utils/pointer" | ||
| "k8s.io/utils/ptr" |
There was a problem hiding this comment.
all changes in this file are not related to what you are introducing in this PR.
| var periodics []prowconfig.Periodic | ||
| rehearsals := info.Config.Rehearsals | ||
| disabledRehearsals := sets.New[string](rehearsals.DisabledRehearsals...) | ||
| disabledRehearsals := sets.New(rehearsals.DisabledRehearsals...) |
There was a problem hiding this comment.
same here. Its not that important but it would be better to open another PR that fixes that in the whole ci-tools repo.
|
|
||
| for _, tc := range tests { | ||
| t.Run(tc.id, func(t *testing.T) { | ||
| // Mimic the behavior of ci-operator config loading where the repo config is overridden |
| t.Run(tc.id, func(t *testing.T) { | ||
| // Mimic the behavior of ci-operator config loading where the repo config is overridden | ||
| // by prowgen config and then generate jobs based on the merged config | ||
| tc.repoInfo.Config.ApplyOverrides(tc.config.Prowgen) |
There was a problem hiding this comment.
This is wrong here. This is proof that the whole test cases are wrong now. You shouldn't need to apply the override before running the function you want to test. It seems that you are trying to hack the test to work instead.
Prucek
force-pushed
the
prowgen-rehearsals
branch
from
68332b0 to
52aa218
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)
pkg/config/load.go (1)
50-95:⚠️ Potential issue | 🟠 MajorValidate regex syntax even when there is only one
slack_reporterentry.Line 52 currently skips all validation unless there are multiple configs. That lets a single invalid
job_name_patternsorexcluded_job_patternsvalue through and only fails later by being silently ignored at match time. Keep the duplicate checks conditional if you want, but regex compilation should run for every config.Suggested fix
func validateProwgenConfig(pConfig *cioperatorapi.Prowgen) error { var errs []error - if len(pConfig.SlackReporterConfigs) > 1 { // There is no reason to validate if we only have one slack_reporter_config - jobsSeen := sets.NewString() - patternsSeen := sets.NewString() + jobsSeen := sets.NewString() + patternsSeen := sets.NewString() + checkDuplicates := len(pConfig.SlackReporterConfigs) > 1 - for _, sc := range pConfig.SlackReporterConfigs { - // Validate exact job names - for _, job := range sc.JobNames { - if jobsSeen.Has(job) { - errs = append(errs, fmt.Errorf("job: %s exists in multiple slack_reporter_configs, it should only be in one", job)) - continue - } - jobsSeen.Insert(job) + for _, sc := range pConfig.SlackReporterConfigs { + for _, job := range sc.JobNames { + if checkDuplicates && jobsSeen.Has(job) { + errs = append(errs, fmt.Errorf("job: %s exists in multiple slack_reporter_configs, it should only be in one", job)) + continue } + jobsSeen.Insert(job) + } - // Validate regex patterns - for _, pattern := range sc.JobNamePatterns { - // Check if regex pattern is valid - if _, err := regexp.Compile(pattern); err != nil { - errs = append(errs, fmt.Errorf("invalid regex pattern: %s, error: %w", pattern, err)) - continue - } - - // Check for duplicate patterns - if patternsSeen.Has(pattern) { - errs = append(errs, fmt.Errorf("regex pattern: %s exists in multiple slack_reporter_configs, it should only be in one", pattern)) - continue - } - patternsSeen.Insert(pattern) + for _, pattern := range sc.JobNamePatterns { + if _, err := regexp.Compile(pattern); err != nil { + errs = append(errs, fmt.Errorf("invalid regex pattern: %s, error: %w", pattern, err)) + continue } - - // Validate excluded job patterns - for _, pattern := range sc.ExcludedJobPatterns { - // Check if regex pattern is valid - if _, err := regexp.Compile(pattern); err != nil { - errs = append(errs, fmt.Errorf("invalid excluded job pattern: %s, error: %w", pattern, err)) - continue - } - - // Note: We don't check for duplicates in excluded patterns as it's reasonable - // to have the same exclusion in multiple configs + if checkDuplicates && patternsSeen.Has(pattern) { + errs = append(errs, fmt.Errorf("regex pattern: %s exists in multiple slack_reporter_configs, it should only be in one", pattern)) + continue } + patternsSeen.Insert(pattern) } + + for _, pattern := range sc.ExcludedJobPatterns { + if _, err := regexp.Compile(pattern); err != nil { + errs = append(errs, fmt.Errorf("invalid excluded job pattern: %s, error: %w", pattern, err)) + } + } } return utilerrors.NewAggregate(errs) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/config/load.go` around lines 50 - 95, The current validateProwgenConfig function only runs validation when len(pConfig.SlackReporterConfigs) > 1, which skips regex compilation for JobNamePatterns and ExcludedJobPatterns when there is a single SlackReporterConfig; update validateProwgenConfig so that you always iterate through pConfig.SlackReporterConfigs to compile each pattern in sc.JobNamePatterns and sc.ExcludedJobPatterns and append any regexp.Compile errors to errs (preserving the existing error messages), but keep the duplicate-detection logic that uses jobsSeen/patternsSeen conditional on having more than one config (i.e., perform the duplicate job/pattern checks only when len(pConfig.SlackReporterConfigs) > 1); return utilerrors.NewAggregate(errs) as before.
♻️ Duplicate comments (1)
pkg/prowgen/prowgen_test.go (1)
714-720:⚠️ Potential issue | 🟠 MajorUse the actual ci-operator field in this scenario.
This case is labeled as ci-operator config, but it still sets the legacy
Prowgen.Rehearsals.DisableAllpath. That means a regression inprowgen.disable_rehearsalsparsing or ci-operator-over-file precedence would still pass here. Switch this setup toDisableRehearsals: ptr.To(true)and add one mixed-source case if you want to lock in the precedence contract.Suggested test setup
- Prowgen: &ciop.ProwgenExtras{Prowgen: ciop.Prowgen{Rehearsals: ciop.Rehearsals{DisableAll: true}}}, + Prowgen: &ciop.ProwgenExtras{DisableRehearsals: ptr.To(true)},🤖 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 714 - 720, The test case is using the legacy field Prowgen.Rehearsals.DisableAll but should exercise the actual ci-operator field; update the test vector that builds a ReleaseBuildConfiguration (symbol: ReleaseBuildConfiguration) so its ProwgenExtras.Prowgen.Rehearsals uses DisableRehearsals: ptr.To(true) instead of DisableAll, and add an additional mixed-source test case combining a ci-operator DisableRehearsals and a Prowgen.Rehearsals.DisableAll to assert ci-operator-over-file precedence (check the test names and assertions around prowgen.disable_rehearsals parsing to validate the behavior).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@pkg/config/load.go`:
- Around line 50-95: The current validateProwgenConfig function only runs
validation when len(pConfig.SlackReporterConfigs) > 1, which skips regex
compilation for JobNamePatterns and ExcludedJobPatterns when there is a single
SlackReporterConfig; update validateProwgenConfig so that you always iterate
through pConfig.SlackReporterConfigs to compile each pattern in
sc.JobNamePatterns and sc.ExcludedJobPatterns and append any regexp.Compile
errors to errs (preserving the existing error messages), but keep the
duplicate-detection logic that uses jobsSeen/patternsSeen conditional on having
more than one config (i.e., perform the duplicate job/pattern checks only when
len(pConfig.SlackReporterConfigs) > 1); return utilerrors.NewAggregate(errs) as
before.
---
Duplicate comments:
In `@pkg/prowgen/prowgen_test.go`:
- Around line 714-720: The test case is using the legacy field
Prowgen.Rehearsals.DisableAll but should exercise the actual ci-operator field;
update the test vector that builds a ReleaseBuildConfiguration (symbol:
ReleaseBuildConfiguration) so its ProwgenExtras.Prowgen.Rehearsals uses
DisableRehearsals: ptr.To(true) instead of DisableAll, and add an additional
mixed-source test case combining a ci-operator DisableRehearsals and a
Prowgen.Rehearsals.DisableAll to assert ci-operator-over-file precedence (check
the test names and assertions around prowgen.disable_rehearsals parsing to
validate the behavior).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: fa168e00-6623-4165-825c-985e24fa9167
⛔ 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 (13)
cmd/ci-operator-prowgen/main.gocmd/ci-operator-prowgen/main_test.gopkg/api/types.gopkg/config/load.gopkg/config/load_test.gopkg/image-graph-generator/operator.gopkg/prowgen/jobbase_test.gopkg/prowgen/prowgen.gopkg/prowgen/prowgen_test.gopkg/prowgen/testdata/zz_fixture_TestGenerateJobs_disabled_all_rehearsals_ci_operator_config_.yamlpkg/prowgen/testdata/zz_fixture_TestGenerateJobs_disabled_all_rehearsals_prowgen_file_.yamlpkg/prowgen/testdata/zz_fixture_TestGenerateJobs_disabled_rehearsals_at_job_level_ci_operator_config_.yamlpkg/prowgen/testdata/zz_fixture_TestGenerateJobs_disabled_rehearsals_at_job_level_prowgen_file_.yaml
✅ Files skipped from review due to trivial changes (3)
- pkg/prowgen/testdata/zz_fixture_TestGenerateJobs_disabled_all_rehearsals_prowgen_file_.yaml
- pkg/prowgen/testdata/zz_fixture_TestGenerateJobs_disabled_all_rehearsals_ci_operator_config_.yaml
- pkg/prowgen/testdata/zz_fixture_TestGenerateJobs_disabled_rehearsals_at_job_level_prowgen_file_.yaml
🚧 Files skipped from review as they are similar to previous changes (2)
- cmd/ci-operator-prowgen/main.go
- pkg/prowgen/prowgen.go
|
@Prucek: 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. |
2 participants
Summary
ProwgenExtrasstruct withDisableRehearsalsfield to ci-operator config (prowgen.disable_rehearsals)DisableRehearsalfield toTestStepConfiguration(disable_rehearsal).config.prowgensettings.config.prowgenDetails
Previously, rehearsal control was only possible via
.config.prowgen:rehearsals.disable_all: true— disables all rehearsals for a reporehearsals.disabled_rehearsals: [test1, test2]— disables specific testsThis PR adds equivalent fields directly in ci-operator config:
prowgen.disable_rehearsals: true— global disable (replacesdisable_all)disable_rehearsal: true— per-test disable (replacesdisabled_rehearsalslist)Both
.config.prowgenand ci-operator config are supported simultaneously, with ci-operator config taking precedence.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Refactor
Tests