prowgen: remove .config.prowgen support#5167
Open
Prucek wants to merge 3 commits into
Open
Conversation
Contributor
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
Contributor
|
Skipping CI for Draft Pull Request. |
|
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughMove Prowgen config into per-repo CI Operator YAML ( ChangesProwgen Configuration Type Removal
Job Generation Simplification
Job Base Builder Updates
Downstream and Tooling Updates
Integration Test Fixture Migration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 2 warnings)
✅ Passed checks (11 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/prowgen/prowgen.go (1)
43-65:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPropagate repo-level
disable_rehearsalsto non-test presubmits.
disableAllRehearsalsis only threaded through theconfigSpec.Testsloop. The generatedimagesand operator bundle presubmits below still callgeneratePresubmitForTest(...)withoutdisableRehearsal, so a repo-wideprowgen.disable_rehearsals: truewill stop suppressing rehearsals for those jobs after this migration.Proposed fix
presubmits[orgrepo] = append(presubmits[orgrepo], *generatePresubmitForTest(jobBaseGen, imagesTestName, info, func(options *generatePresubmitOptions) { + options.disableRehearsal = disableAllRehearsals options.optional = optional options.runIfChanged = configSpec.Images.RunIfChanged options.skipIfOnlyChanged = configSpec.Images.SkipIfOnlyChanged options.pipelineRunIfChanged = configSpec.Images.PipelineRunIfChanged options.pipelineSkipIfOnlyChanged = configSpec.Images.PipelineSkipIfOnlyChanged @@ presubmits[orgrepo] = append(presubmits[orgrepo], *generatePresubmitForTest(jobBaseGen, testName, info, func(options *generatePresubmitOptions) { + options.disableRehearsal = disableAllRehearsals options.optional = bundle.Optional options.Capabilities = bundle.Capabilities })) @@ - presubmits[orgrepo] = append(presubmits[orgrepo], *generatePresubmitForTest(jobBaseGen, name, info)) + presubmits[orgrepo] = append(presubmits[orgrepo], *generatePresubmitForTest(jobBaseGen, name, info, func(options *generatePresubmitOptions) { + options.disableRehearsal = disableAllRehearsals + }))Also applies to: 143-149, 181-208
🤖 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/prowgen/prowgen.go` around lines 43 - 65, The repo-level disableAllRehearsals flag is computed when iterating configSpec.Tests but never propagated into the other presubmit generators, so generatePresubmitForTest(...) calls for images and operator bundles ignore it; update the call sites that create non-test presubmits (calls to generatePresubmitForTest and any wrappers that build presubmits) to accept and pass a disableRehearsal boolean (derived from disableAllRehearsals || element.DisableRehearsal or appropriate repo-level value), and adjust the signature of generatePresubmitForTest (and any helper like NewProwJobBaseBuilderForTest usage) to honor that parameter so rehearsals are suppressed consistently. Ensure all places noted in the review (the additional presubmit generation blocks) pass the propagated disableRehearsal flag.
🧹 Nitpick comments (1)
cmd/ci-operator-prowgen/main.go (1)
123-136: ⚡ Quick winWrap
GenerateJobsfailures with repo context.The raw
return errhere drops whichorg/repowas being processed, so one bad config becomes hard to isolate during a full tree run.Proposed fix
generated, err := prowgen.GenerateJobs(configSpec, pInfo) if err != nil { - return err + return fmt.Errorf("generate jobs for %s: %w", orgRepo, err) }As per coding guidelines, "Wrap errors with fmt.Errorf("context: %w", err), lowercase messages, no trailing punctuation"
🤖 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-prowgen/main.go` around lines 123 - 136, The call to prowgen.GenerateJobs in generateJobs returns an error that is currently returned raw, losing repo context; update the error return to wrap the failure with the org/repo context (use orgRepo) using fmt.Errorf with the %w verb and a lowercase, no-trailing-punctuation message (e.g., "generate jobs for %s: %w", orgRepo, err) so failures from prowgen.GenerateJobs include which repository failed while keeping existing variables (configSpec, pInfo, generated) unchanged.
🤖 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/prowgen/prowgen.go`:
- Around line 262-275: slackReporterConfig currently only reads
tests[*].reporter_config and thus drops reporter settings for
synthesized/non-test jobs; update slackReporterConfig (and its callers) to
accept and merge a repo-level reporter config (e.g., a second parameter
repoSlackConfig *cioperatorapi.SlackReporterConfig) so that if testSlackConfig
is nil or missing fields you fall back to repoSlackConfig, then apply the
existing default-job-states logic and return the merged prowv1.ReporterConfig;
reference slackReporterConfig, cioperatorapi.SlackReporterConfig and
prowv1.ReporterConfig when locating where to implement the merge.
---
Outside diff comments:
In `@pkg/prowgen/prowgen.go`:
- Around line 43-65: The repo-level disableAllRehearsals flag is computed when
iterating configSpec.Tests but never propagated into the other presubmit
generators, so generatePresubmitForTest(...) calls for images and operator
bundles ignore it; update the call sites that create non-test presubmits (calls
to generatePresubmitForTest and any wrappers that build presubmits) to accept
and pass a disableRehearsal boolean (derived from disableAllRehearsals ||
element.DisableRehearsal or appropriate repo-level value), and adjust the
signature of generatePresubmitForTest (and any helper like
NewProwJobBaseBuilderForTest usage) to honor that parameter so rehearsals are
suppressed consistently. Ensure all places noted in the review (the additional
presubmit generation blocks) pass the propagated disableRehearsal flag.
---
Nitpick comments:
In `@cmd/ci-operator-prowgen/main.go`:
- Around line 123-136: The call to prowgen.GenerateJobs in generateJobs returns
an error that is currently returned raw, losing repo context; update the error
return to wrap the failure with the org/repo context (use orgRepo) using
fmt.Errorf with the %w verb and a lowercase, no-trailing-punctuation message
(e.g., "generate jobs for %s: %w", orgRepo, err) so failures from
prowgen.GenerateJobs include which repository failed while keeping existing
variables (configSpec, pInfo, generated) unchanged.
🪄 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: dc6a2fde-2869-489f-84b3-9bb7477a288f
📒 Files selected for processing (29)
cmd/check-gh-automation/main.gocmd/ci-operator-prowgen/main.gocmd/ci-operator-prowgen/main_test.gopkg/config/load.gopkg/config/load_test.gopkg/image-graph-generator/operator.gopkg/prowgen/jobbase.gopkg/prowgen/jobbase_test.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_images_job_is_configured_for_slack_reporting.yamlpkg/prowgen/testdata/zz_fixture_TestNewProwJobBaseBuilderForTest_job_excluded_by_patterns_should_not_have_slack_reporter_config.yamltest/integration/ci-operator-prowgen/input/config/norehearsals/duper/.config.prowgentest/integration/ci-operator-prowgen/input/config/norehearsals/duper/norehearsals-duper-master.yamltest/integration/ci-operator-prowgen/input/config/norehearsals/stuper/.config.prowgentest/integration/ci-operator-prowgen/input/config/norehearsals/stuper/norehearsals-stuper-master.yamltest/integration/ci-operator-prowgen/input/config/private-org/.config.prowgentest/integration/ci-operator-prowgen/input/config/private-org/duper/private-org-duper-master.yamltest/integration/ci-operator-prowgen/input/config/private-org/super/private-org-super-master.yamltest/integration/ci-operator-prowgen/input/config/private/duper/.config.prowgentest/integration/ci-operator-prowgen/input/config/private/duper/private-duper-master.yamltest/integration/ci-operator-prowgen/input/config/slack-report/duper/.config.prowgentest/integration/ci-operator-prowgen/input/config/slack-report/duper/slack-report-duper-master.yamltest/integration/ci-operator-prowgen/input/config/super/duper/.config.prowgentest/integration/ci-operator-prowgen/input/config/super/duper/super-duper-release-4.19__periodics.yamltest/integration/ci-operator-prowgen/output/jobs/slack-report-inline/duper/slack-report-inline-duper-master-periodics.yamltest/integration/ci-operator-prowgen/output/jobs/slack-report-inline/duper/slack-report-inline-duper-master-postsubmits.yamltest/integration/ci-operator-prowgen/output/jobs/slack-report-inline/duper/slack-report-inline-duper-master-presubmits.yaml
💤 Files with no reviewable changes (16)
- test/integration/ci-operator-prowgen/input/config/norehearsals/stuper/.config.prowgen
- test/integration/ci-operator-prowgen/input/config/private-org/.config.prowgen
- pkg/prowgen/testdata/zz_fixture_TestGenerateJobs_ci_operator_config_takes_precedence_over_prowgen_config.yaml
- test/integration/ci-operator-prowgen/input/config/norehearsals/duper/.config.prowgen
- pkg/prowgen/testdata/zz_fixture_TestNewProwJobBaseBuilderForTest_job_excluded_by_patterns_should_not_have_slack_reporter_config.yaml
- test/integration/ci-operator-prowgen/input/config/super/duper/.config.prowgen
- test/integration/ci-operator-prowgen/input/config/private/duper/.config.prowgen
- test/integration/ci-operator-prowgen/output/jobs/slack-report-inline/duper/slack-report-inline-duper-master-presubmits.yaml
- test/integration/ci-operator-prowgen/output/jobs/slack-report-inline/duper/slack-report-inline-duper-master-periodics.yaml
- test/integration/ci-operator-prowgen/input/config/slack-report/duper/.config.prowgen
- test/integration/ci-operator-prowgen/output/jobs/slack-report-inline/duper/slack-report-inline-duper-master-postsubmits.yaml
- pkg/image-graph-generator/operator.go
- pkg/prowgen/testdata/zz_fixture_TestGenerateJobs_images_job_is_configured_for_slack_reporting.yaml
- pkg/config/load_test.go
- cmd/check-gh-automation/main.go
- pkg/config/load.go
Comment on lines
+262
to
+275
| func slackReporterConfig(testSlackConfig *cioperatorapi.SlackReporterConfig) *prowv1.ReporterConfig { | ||
| if testSlackConfig == nil { | ||
| return nil | ||
| } | ||
| if slackReporter := info.Config.GetSlackReporterConfigForJobName(jobName, testName, info.Metadata.Variant); slackReporter != nil { | ||
| return &prowv1.ReporterConfig{ | ||
| Slack: &prowv1.SlackReporterConfig{ | ||
| Channel: slackReporter.Channel, | ||
| JobStatesToReport: slackReporter.JobStatesToReport, | ||
| ReportTemplate: slackReporter.ReportTemplate, | ||
| }, | ||
| } | ||
| jobStatesToReport := testSlackConfig.JobStatesToReport | ||
| if len(jobStatesToReport) == 0 { | ||
| jobStatesToReport = cioperatorapi.DefaultSlackReporterJobStatesToReport | ||
| } | ||
| return &prowv1.ReporterConfig{ | ||
| Slack: &prowv1.SlackReporterConfig{ | ||
| Channel: testSlackConfig.Channel, | ||
| JobStatesToReport: jobStatesToReport, | ||
| ReportTemplate: testSlackConfig.ReportTemplate, | ||
| }, |
There was a problem hiding this comment.
This migration drops reporter config for generated images jobs.
After narrowing slackReporterConfig to tests[*].reporter_config only, synthesized jobs like the images promotion postsubmit have no inline field that can ever reach this helper. Repos that previously configured Slack reporting for those jobs via .config.prowgen lose that behavior with no replacement.
If that deprecation is intentional, it should be called out explicitly; otherwise this needs a non-test reporter-config source before the old path is removed.
🤖 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/prowgen/prowgen.go` around lines 262 - 275, slackReporterConfig currently
only reads tests[*].reporter_config and thus drops reporter settings for
synthesized/non-test jobs; update slackReporterConfig (and its callers) to
accept and merge a repo-level reporter config (e.g., a second parameter
repoSlackConfig *cioperatorapi.SlackReporterConfig) so that if testSlackConfig
is nil or missing fields you fall back to repoSlackConfig, then apply the
existing default-job-states logic and return the merged prowv1.ReporterConfig;
reference slackReporterConfig, cioperatorapi.SlackReporterConfig and
prowv1.ReporterConfig when locating where to implement the merge.
All features previously configured via .config.prowgen files are now available inline in ci-operator configuration YAML via the `prowgen:` field (private, expose, disable_rehearsals, skip_operator_presubmits, enable_secrets_store_csi_driver) and per-test `reporter_config` / `disable_rehearsal` fields. This removes: - config.Prowgen struct and all associated types/functions from pkg/config/load.go (Rehearsals, SlackReporterConfig, SkipOperatorPresubmits, LoadProwgenConfig, validateProwgenConfig, MergeDefaults, GetSlackReporterConfigForJobName, SkipPresubmits) - ProwgenInfo.Config field — ProwgenInfo now only contains Metadata - .config.prowgen loading from ci-operator-prowgen and image-graph-generator - .config.prowgen skip from check-gh-automation Integration test fixtures migrated from .config.prowgen to inline ci-operator config equivalents. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Prucek
force-pushed
the
remove-prowgen-config-v2
branch
from
55deef5 to
a14f0f7
Compare
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
Contributor
|
Scheduling tests matching the |
Member
Author
|
/hold |
openshift-ci
Bot
added
the
do-not-merge/hold
After removing .config.prowgen support, ProwgenInfo was an empty wrapper around cioperatorapi.Metadata. Replace all usages with *cioperatorapi.Metadata. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Member
Author
|
/label tide/merge-method-squash |
openshift-ci
Bot
added
the
tide/merge-method-squash
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/prowgen/prowgen_test.go (1)
1-1105: ⚡ Quick winTest coverage gap: images job Slack reporting no longer tested.
The removed "images job is configured for slack reporting" test case validated Slack reporter config for images jobs via the old
ProwgenInfo.Config.SlackReporterConfigspath. That test should be replaced with a new one that verifies images jobs respectconfigSpec.Images.SlackReporterConfigin the inline approach. Without it, regressions in this feature could go undetected.📋 Add a test case for images job inline Slack config
Add a test case to
TestGenerateJobssimilar to:{ id: "images job with inline slack reporter config", config: &ciop.ReleaseBuildConfiguration{ Images: ciop.ImageConfiguration{ Items: []ciop.ProjectDirectoryImageBuildStepConfiguration{{}}, SlackReporterConfig: &ciop.SlackReporterConfig{ Channel: "test-channel", JobStatesToReport: []prowv1.ProwJobState{prowv1.FailureState}, }, }, PromotionConfiguration: &ciop.PromotionConfiguration{}, }, repoInfo: &ciop.Metadata{ Org: "organization", Repo: "repository", Branch: "branch", }, },Then verify the generated presubmit has
base.ReporterConfigpopulated correctly.🤖 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/prowgen/prowgen_test.go` around lines 1 - 1105, Add a new test case to TestGenerateJobs that supplies Images.SlackReporterConfig (ciop.SlackReporterConfig) in the ReleaseBuildConfiguration and a PromotionConfiguration so GenerateJobs produces an images presubmit; after calling GenerateJobs, locate the generated presubmit for the images job in jobConfig.PresubmitsStatic and assert its Base.ReporterConfig (or equivalent reporter field produced by GenerateJobs) is non-nil and populated with the Channel/JobStatesToReport you set. Update TestGenerateJobs (and the test vector slice) to include this case so GenerateJobs and the images-slack wiring from Images.SlackReporterConfig are covered.
🤖 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/prowgen/prowgen_test.go`:
- Around line 1-1105: Add a new test case to TestGenerateJobs that supplies
Images.SlackReporterConfig (ciop.SlackReporterConfig) in the
ReleaseBuildConfiguration and a PromotionConfiguration so GenerateJobs produces
an images presubmit; after calling GenerateJobs, locate the generated presubmit
for the images job in jobConfig.PresubmitsStatic and assert its
Base.ReporterConfig (or equivalent reporter field produced by GenerateJobs) is
non-nil and populated with the Channel/JobStatesToReport you set. Update
TestGenerateJobs (and the test vector slice) to include this case so
GenerateJobs and the images-slack wiring from Images.SlackReporterConfig are
covered.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 9a06a72d-a85e-410a-9216-a0f7b9cad5ff
📒 Files selected for processing (8)
cmd/ci-operator-prowgen/main.gocmd/multi-pr-prow-plugin/server.gopkg/controller/ephemeralcluster/reconciler.gopkg/controller/prpqr_reconciler/prpqr_reconciler.gopkg/prowgen/jobbase.gopkg/prowgen/jobbase_test.gopkg/prowgen/prowgen.gopkg/prowgen/prowgen_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/prowgen/jobbase.go
- cmd/ci-operator-prowgen/main.go
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Member
Author
|
/retest-required |
Member
Author
|
/retest-required |
Contributor
|
@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. |
Member
Author
|
/retest-required |
Contributor
|
/lgtm |
Contributor
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Prucek, psalajova 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 |
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
2 participants
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.
Summary
.config.prowgenfile support entirely from prowgen.config.prowgenare now available inline in ci-operator configuration YAML:prowgen: {private, expose, disable_rehearsals, skip_operator_presubmits, enable_secrets_store_csi_driver}reporter_configanddisable_rehearsalfieldsconfig.Prowgenstruct and all associated types/functions frompkg/config/load.goProwgenInfo.Configfield —ProwgenInfonow only containsMetadata.config.prowgenloading fromci-operator-prowgenandimage-graph-generator.config.prowgenskip fromcheck-gh-automation.config.prowgento inline ci-operator config equivalents🤖 Generated with Claude Code
prowgen: remove .config.prowgen support
This PR removes support for the legacy .config.prowgen file and consolidates all prowgen configuration into ci-operator YAML under a new prowgen: section (with per-test fields added on test entries). Repositories that still rely on .config.prowgen must migrate those settings into their ci-operator configs; the PR includes updated integration test fixtures as examples.
What this means for CI users and operators
Component-level changes and effects
Migration guidance
The PR removes legacy loader/types and simplifies prowgen codepaths to rely exclusively on ci-operator API fields.