prowgen: remove .config.prowgen support#5079
Draft
Prucek wants to merge 2 commits intoopenshift:mainfrom
Draft
Conversation
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
WalkthroughThis pull request migrates Prowgen configuration from an external Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
|
[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
Member
Author
|
/hold |
openshift-ci
bot
added
the
do-not-merge/hold
openshift-ci
bot
added
the
do-not-merge/work-in-progress
![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
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)
444-453:⚠️ Potential issue | 🟠 MajorDefault periodic
max_concurrencyto 1 when unset.Line 116 applies the new default to postsubmits, but Lines 451-453 only copy the periodic value when the pointer is non-nil. That diverges from Lines 915-918 in
pkg/api/types.goand leaves test periodics at zero/unset concurrency whenmax_concurrencyis omitted, so overlapping runs become possible.🛠️ Suggested fix
pj := &prowconfig.Periodic{ JobBase: base, Cron: cron, Interval: opts.Interval, MinimumInterval: opts.MinimumInterval, Retry: opts.Retry, + MaxConcurrency: maxConcurrency(opts.MaxConcurrency, 1), } - if opts.MaxConcurrency != nil { - pj.MaxConcurrency = *opts.MaxConcurrency - }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/prowgen/prowgen.go` around lines 444 - 453, The periodic job creation sets pj.MaxConcurrency only when opts.MaxConcurrency is non-nil, leaving it unset (zero) and allowing overlapping runs; update the logic in the block that builds prowconfig.Periodic (pj) so that if opts.MaxConcurrency is nil you set pj.MaxConcurrency = 1, otherwise set it to *opts.MaxConcurrency (i.e., ensure pj.MaxConcurrency defaults to 1 when opts.MaxConcurrency is omitted, mirroring the postsubmit/default behavior).
🧹 Nitpick comments (1)
pkg/webreg/zz_generated.ci_operator_reference.go (1)
641-644: Documentation references removed feature.The comment mentions ".config.prowgen's disable_all setting" but this PR removes .config.prowgen support. Consider updating the documentation to remove this reference since users can no longer use that configuration method.
Suggested documentation update
- " # DisableRehearsal prevents this specific test from being picked up for rehearsals.\n" + - " # Note: this cannot re-enable rehearsals if they are globally disabled via\n" + - " # prowgen.disable_rehearsals or .config.prowgen's disable_all setting.\n" + + " # DisableRehearsal prevents this specific test from being picked up for rehearsals.\n" + + " # Note: this cannot re-enable rehearsals if they are globally disabled via\n" + + " # prowgen.disable_rehearsals.\n" +🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/webreg/zz_generated.ci_operator_reference.go` around lines 641 - 644, Update the generated comment string that mentions ".config.prowgen's disable_all setting" to remove that reference and rephrase the sentence in the string literal so it no longer suggests .config.prowgen can re-enable rehearsals; locate the multi-line string in pkg/webreg/zz_generated.ci_operator_reference.go (the block containing "DisableRehearsal prevents this specific test..." and "disable_rehearsal: false") and edit the text to instead note that rehearsals may be globally disabled via prowgen settings without referencing .config.prowgen, keeping the rest of the explanatory text intact.
🤖 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/jobbase.go`:
- Around line 23-30: The isPrivate function currently lets
configSpec.Prowgen.Private override the org check; change it so that org ==
"openshift-priv" always yields true regardless of prowgen.private. Concretely,
in isPrivate ensure you return true if info.Org == "openshift-priv" before
honoring configSpec.Prowgen.Private (or compute the result as configPrivate ||
info.Org == "openshift-priv"), referencing the existing symbols isPrivate,
configSpec.Prowgen.Private, and info.Org with the literal "openshift-priv".
In
`@pkg/prowgen/testdata/zz_fixture_TestGenerateJobs_private_with_expose_from_ci_operator_prowgen_config.yaml`:
- Around line 57-69: The volumes list is missing a definition for the referenced
volume "gcs-credentials" used by the container; add a volume entry named
"gcs-credentials" under the same volumes block (similar to existing secret
volumes like "pull-secret" and "result-aggregator") that mounts the appropriate
Kubernetes Secret (e.g., secretName: gcs-credentials) so the pod spec validates
and the container can access the GCS credentials.
---
Outside diff comments:
In `@pkg/prowgen/prowgen.go`:
- Around line 444-453: The periodic job creation sets pj.MaxConcurrency only
when opts.MaxConcurrency is non-nil, leaving it unset (zero) and allowing
overlapping runs; update the logic in the block that builds prowconfig.Periodic
(pj) so that if opts.MaxConcurrency is nil you set pj.MaxConcurrency = 1,
otherwise set it to *opts.MaxConcurrency (i.e., ensure pj.MaxConcurrency
defaults to 1 when opts.MaxConcurrency is omitted, mirroring the
postsubmit/default behavior).
---
Nitpick comments:
In `@pkg/webreg/zz_generated.ci_operator_reference.go`:
- Around line 641-644: Update the generated comment string that mentions
".config.prowgen's disable_all setting" to remove that reference and rephrase
the sentence in the string literal so it no longer suggests .config.prowgen can
re-enable rehearsals; locate the multi-line string in
pkg/webreg/zz_generated.ci_operator_reference.go (the block containing
"DisableRehearsal prevents this specific test..." and "disable_rehearsal:
false") and edit the text to instead note that rehearsals may be globally
disabled via prowgen settings without referencing .config.prowgen, keeping the
rest of the explanatory text intact.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1dedbeb5-c213-49d5-926a-9b56efa92654
📒 Files selected for processing (45)
cmd/check-gh-automation/main.gocmd/ci-operator-prowgen/main.gocmd/ci-operator-prowgen/main_test.gopkg/api/types.gopkg/api/zz_generated.deepcopy.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_disable_all_rehearsals_from_ci_operator_prowgen_config.yamlpkg/prowgen/testdata/zz_fixture_TestGenerateJobs_disable_rehearsal_from_ci_operator_config_per_test.yamlpkg/prowgen/testdata/zz_fixture_TestGenerateJobs_images_job_is_configured_for_slack_reporting.yamlpkg/prowgen/testdata/zz_fixture_TestGenerateJobs_openshift_priv_org_defaults_to_private.yamlpkg/prowgen/testdata/zz_fixture_TestGenerateJobs_periodic_with_custom_max_concurrency.yamlpkg/prowgen/testdata/zz_fixture_TestGenerateJobs_postsubmit_with_custom_max_concurrency.yamlpkg/prowgen/testdata/zz_fixture_TestGenerateJobs_private_from_ci_operator_prowgen_config.yamlpkg/prowgen/testdata/zz_fixture_TestGenerateJobs_private_with_expose_from_ci_operator_prowgen_config.yamlpkg/prowgen/testdata/zz_fixture_TestGenerateJobs_skip_operator_presubmits_from_ci_operator_config.yamlpkg/prowgen/testdata/zz_fixture_TestGenerateJobs_slack_reporter_from_ci_operator_config_per_test.yamlpkg/prowgen/testdata/zz_fixture_TestGenerateJobs_slack_reporter_from_ci_operator_config_with_defaults.yamlpkg/prowgen/testdata/zz_fixture_TestGenerateJobs_slack_reporter_from_ci_operator_config_with_explicit_values.yamlpkg/prowgen/testdata/zz_fixture_TestNewProwJobBaseBuilderForTest_job_excluded_by_patterns_should_not_have_slack_reporter_config.yamlpkg/prowgen/testdata/zz_fixture_TestNewProwJobBaseBuilderForTest_simple_with_slack_reporter_config.yamlpkg/validation/test.gopkg/webreg/zz_generated.ci_operator_reference.gotest/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/prowgen-config/duper/prowgen-config-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/prowgen-config/duper/prowgen-config-duper-master-periodics.yamltest/integration/ci-operator-prowgen/output/jobs/prowgen-config/duper/prowgen-config-duper-master-postsubmits.yamltest/integration/ci-operator-prowgen/output/jobs/prowgen-config/duper/prowgen-config-duper-master-presubmits.yaml
💤 Files with no reviewable changes (13)
- test/integration/ci-operator-prowgen/input/config/private-org/.config.prowgen
- test/integration/ci-operator-prowgen/input/config/norehearsals/stuper/.config.prowgen
- test/integration/ci-operator-prowgen/input/config/norehearsals/duper/.config.prowgen
- test/integration/ci-operator-prowgen/input/config/private/duper/.config.prowgen
- cmd/check-gh-automation/main.go
- pkg/image-graph-generator/operator.go
- test/integration/ci-operator-prowgen/input/config/super/duper/.config.prowgen
- pkg/prowgen/testdata/zz_fixture_TestGenerateJobs_images_job_is_configured_for_slack_reporting.yaml
- test/integration/ci-operator-prowgen/input/config/slack-report/duper/.config.prowgen
- pkg/prowgen/testdata/zz_fixture_TestNewProwJobBaseBuilderForTest_job_excluded_by_patterns_should_not_have_slack_reporter_config.yaml
- pkg/prowgen/testdata/zz_fixture_TestNewProwJobBaseBuilderForTest_simple_with_slack_reporter_config.yaml
- pkg/config/load_test.go
- pkg/config/load.go
Comment on lines
+23
to
+30
| // isPrivate returns true if the repo is private, checking ci-operator config | ||
| // and the org name (openshift-priv is always private). | ||
| func isPrivate(configSpec *cioperatorapi.ReleaseBuildConfiguration, info *ProwgenInfo) bool { | ||
| if configSpec.Prowgen != nil && configSpec.Prowgen.Private != nil { | ||
| return *configSpec.Prowgen.Private | ||
| } | ||
| return info.Org == "openshift-priv" | ||
| } |
There was a problem hiding this comment.
Enforce openshift-priv as unconditionally private
Line 26 currently allows prowgen.private: false to override openshift-priv, which conflicts with the function contract and can unintentionally disable private-job protections (hidden jobs and private token paths at Line 109/Line 128/Line 137).
Suggested fix
func isPrivate(configSpec *cioperatorapi.ReleaseBuildConfiguration, info *ProwgenInfo) bool {
- if configSpec.Prowgen != nil && configSpec.Prowgen.Private != nil {
- return *configSpec.Prowgen.Private
- }
- return info.Org == "openshift-priv"
+ if info != nil && info.Org == "openshift-priv" {
+ return true
+ }
+ if configSpec != nil && configSpec.Prowgen != nil && configSpec.Prowgen.Private != nil {
+ return *configSpec.Prowgen.Private
+ }
+ return false
}📝 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.
Suggested change
| // isPrivate returns true if the repo is private, checking ci-operator config | |
| // and the org name (openshift-priv is always private). | |
| func isPrivate(configSpec *cioperatorapi.ReleaseBuildConfiguration, info *ProwgenInfo) bool { | |
| if configSpec.Prowgen != nil && configSpec.Prowgen.Private != nil { | |
| return *configSpec.Prowgen.Private | |
| } | |
| return info.Org == "openshift-priv" | |
| } | |
| // isPrivate returns true if the repo is private, checking ci-operator config | |
| // and the org name (openshift-priv is always private). | |
| func isPrivate(configSpec *cioperatorapi.ReleaseBuildConfiguration, info *ProwgenInfo) bool { | |
| if info != nil && info.Org == "openshift-priv" { | |
| return true | |
| } | |
| if configSpec != nil && configSpec.Prowgen != nil && configSpec.Prowgen.Private != nil { | |
| return *configSpec.Prowgen.Private | |
| } | |
| return false | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/prowgen/jobbase.go` around lines 23 - 30, The isPrivate function
currently lets configSpec.Prowgen.Private override the org check; change it so
that org == "openshift-priv" always yields true regardless of prowgen.private.
Concretely, in isPrivate ensure you return true if info.Org == "openshift-priv"
before honoring configSpec.Prowgen.Private (or compute the result as
configPrivate || info.Org == "openshift-priv"), referencing the existing symbols
isPrivate, configSpec.Prowgen.Private, and info.Org with the literal
"openshift-priv".
Comment on lines
+57
to
+69
| volumes: | ||
| - name: github-credentials-openshift-ci-robot-private-git-cloner | ||
| secret: | ||
| secretName: github-credentials-openshift-ci-robot-private-git-cloner | ||
| - name: manifest-tool-local-pusher | ||
| secret: | ||
| secretName: manifest-tool-local-pusher | ||
| - name: pull-secret | ||
| secret: | ||
| secretName: registry-pull-credentials | ||
| - name: result-aggregator | ||
| secret: | ||
| secretName: result-aggregator |
There was a problem hiding this comment.
Critical: Missing volume definition for gcs-credentials.
The container references a gcs-credentials volume mount at lines 41-43, but the volumes section does not define this volume. This results in an invalid Kubernetes pod spec that would fail validation.
🔧 Proposed fix: Add the missing volume definition
serviceAccountName: ci-operator
volumes:
+ - name: gcs-credentials
+ secret:
+ secretName: gcs-credentials
- name: github-credentials-openshift-ci-robot-private-git-cloner
secret:
secretName: github-credentials-openshift-ci-robot-private-git-cloner🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@pkg/prowgen/testdata/zz_fixture_TestGenerateJobs_private_with_expose_from_ci_operator_prowgen_config.yaml`
around lines 57 - 69, The volumes list is missing a definition for the
referenced volume "gcs-credentials" used by the container; add a volume entry
named "gcs-credentials" under the same volumes block (similar to existing secret
volumes like "pull-secret" and "result-aggregator") that mounts the appropriate
Kubernetes Secret (e.g., secretName: gcs-credentials) so the pod spec validates
and the container can access the GCS credentials.
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
Follow-up to #5012 (deprecation). This PR removes
.config.prowgensupport entirely:config.Prowgenstruct,SlackReporterConfig,Rehearsals,SkipOperatorPresubmitstypes and all related methodsProwgenInfo.Configfield —ProwgenInfonow only embedsMetadataaddSlackReporterConfigto only use per-testSlackReporter(no.config.prowgenfallback).config.prowgenloading/caching fromci-operator-prowgenand dead code fromimage-graph-generator.config.prowgenskip fromcheck-gh-automation.config.prowgenfiles (migrated to inline ci-operator config)All configuration previously in
.config.prowgenis now handled via inline ci-operator config fields introduced in #5012.Depends on #5012.
🤖 Generated with Claude Code