Open
Conversation
Contributor
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
WalkthroughAdds Slack reporting and per-test concurrency/rehearsal controls to the API, threads those options through prowgen job generation with explicit precedence rules, refactors private/expose/CSI checks in job base logic, updates deepcopy and schema, and adds tests and fixtures. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 7 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (7 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.11.4)Command failed 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
![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
🤖 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/api/types.go`:
- Around line 52-64: The ProwgenExtras boolean toggles (Private, Expose,
DisableRehearsals, EnableSecretsStoreCSIDriver) and
OperatorStepConfiguration.SkipPresubmits must be made tri-state so omitted vs
explicit-false can be distinguished; change their types from bool to *bool
(e.g., Private *bool) and update downstream checks in isPrivate(), isExposed(),
isSecretsStoreCSIDriverEnabled() and any logic using SkipPresubmits to test for
nil before dereferencing (treat nil as “unset” and only honor explicit
true/false when pointer is non-nil), keeping existing fallback behavior to
legacy checks.
In
`@test/integration/ci-operator-prowgen/input/config/prowgen-config/duper/prowgen-config-duper-master.yaml`:
- Around line 16-19: Update the CPU quantity units for the YAML fixture: replace
limits.cpu value "500Mi" with "500m" and requests.cpu value "10Mi" with "10m" so
CPU uses millicores; locate the entries under the limits and requests keys
(limits.cpu, requests.cpu) and change the unit suffix from "Mi" to "m".
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8a423303-e44b-4b84-9d62-5200cef147ca
📒 Files selected for processing (19)
pkg/api/types.gopkg/prowgen/jobbase.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_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_takes_precedence_over_prowgen_config.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.yamltest/integration/ci-operator-prowgen/input/config/prowgen-config/duper/prowgen-config-duper-master.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
...ation/ci-operator-prowgen/input/config/prowgen-config/duper/prowgen-config-duper-master.yaml
Outdated
Show resolved
Hide resolved
Prucek
force-pushed
the
deprecate-prowgen-config
branch
from
d3f4082 to
6cbe1c3
Compare
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)
228-239:⚠️ Potential issue | 🟠 MajorPresubmits still ignore
tests[].max_concurrency.The periodic and postsubmit branches propagate
element.MaxConcurrency, but the presubmit path only forwardselement.SlackReporter. As a result, the new field has no effect for ordinary presubmit jobs. ThreadMaxConcurrencythroughgeneratePresubmitOptionsand apply it when constructing the presubmit as well.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/prowgen/prowgen.go` around lines 228 - 239, The presubmit path in handlePresubmit doesn't propagate element.MaxConcurrency into the generatePresubmitOptions, so presubmits ignore tests[].max_concurrency; update the call inside handlePresubmit (where generatePresubmitForTest is invoked and the closure sets options.*) to set the MaxConcurrency option from element (e.g., options.MaxConcurrency = element.MaxConcurrency) so generatePresubmitForTest and the resulting presubmit use the provided max concurrency value (follow the same symbol naming used in the periodic/postsubmit branches and in generatePresubmitOptions).
♻️ Duplicate comments (2)
pkg/api/types.go (2)
52-64:⚠️ Potential issue | 🟠 MajorMigrated prowgen booleans still need tri-state semantics.
These fields are supposed to override
.config.prowgen, but plainboolcollapses “unset” and explicitfalseduring unmarshal. That meansprowgen.private,prowgen.expose,prowgen.disable_rehearsals,prowgen.enable_secrets_store_csi_driver, andoperator.skip_presubmitsstill cannot honor precedence when the migrated value isfalse. Use*booland only fall back to legacy config when the pointer is nil.Also applies to: 2759-2760
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/api/types.go` around lines 52 - 64, The migrated prowgen boolean fields in the ProwgenExtras struct (Private, Expose, DisableRehearsals, EnableSecretsStoreCSIDriver) must be tri-state to distinguish "unset" from explicit false; change their types from bool to *bool and update any code that reads these fields to treat nil as "not set" and fall back to the legacy .config.prowgen value (i.e., only override legacy config when the pointer is non-nil), and apply the same change/handling for operator.skip_presubmits where applicable.
914-915:⚠️ Potential issue | 🟠 Major
disable_rehearsalhas the same precedence bug.With a plain
bool,disable_rehearsal: falseis indistinguishable from omission. In the current generation logic, that means a test still matched by legacydisabled_rehearsalscannot be re-enabled from ci-operator config. This field also needs to be*bool, with fallback to legacy config only when it is nil.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/api/types.go` around lines 914 - 915, The DisableRehearsal field in the struct should be changed from a plain bool to a pointer (*bool) so omission is distinguishable from explicit false; update the declaration of DisableRehearsal (the struct field named DisableRehearsal in pkg/api/types.go) to use *bool and leave the json tag as-is, and then update any code that reads this field (generation/validation/merge logic that currently falls back to legacy disabled_rehearsals) to only consult the legacy disabled_rehearsals when DisableRehearsal is nil, otherwise honor the explicit true/false value.
🤖 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/api/zz_generated.deepcopy.go`:
- Around line 1610-1623: Update the ProwgenExtras struct so the fields Private,
Expose, DisableRehearsals, and EnableSecretsStoreCSIDriver are pointers to bool
(*bool) instead of plain bool, then regenerate the deepcopy code so
DeepCopyInto/DeepCopy reflect those pointer types; locate the struct definition
for ProwgenExtras (where those fields are declared), change the types to *bool,
and run the repository's codegen/deepcopy generator to overwrite
pkg/api/zz_generated.deepcopy.go so the new pointer semantics are preserved.
---
Outside diff comments:
In `@pkg/prowgen/prowgen.go`:
- Around line 228-239: The presubmit path in handlePresubmit doesn't propagate
element.MaxConcurrency into the generatePresubmitOptions, so presubmits ignore
tests[].max_concurrency; update the call inside handlePresubmit (where
generatePresubmitForTest is invoked and the closure sets options.*) to set the
MaxConcurrency option from element (e.g., options.MaxConcurrency =
element.MaxConcurrency) so generatePresubmitForTest and the resulting presubmit
use the provided max concurrency value (follow the same symbol naming used in
the periodic/postsubmit branches and in generatePresubmitOptions).
---
Duplicate comments:
In `@pkg/api/types.go`:
- Around line 52-64: The migrated prowgen boolean fields in the ProwgenExtras
struct (Private, Expose, DisableRehearsals, EnableSecretsStoreCSIDriver) must be
tri-state to distinguish "unset" from explicit false; change their types from
bool to *bool and update any code that reads these fields to treat nil as "not
set" and fall back to the legacy .config.prowgen value (i.e., only override
legacy config when the pointer is non-nil), and apply the same change/handling
for operator.skip_presubmits where applicable.
- Around line 914-915: The DisableRehearsal field in the struct should be
changed from a plain bool to a pointer (*bool) so omission is distinguishable
from explicit false; update the declaration of DisableRehearsal (the struct
field named DisableRehearsal in pkg/api/types.go) to use *bool and leave the
json tag as-is, and then update any code that reads this field
(generation/validation/merge logic that currently falls back to legacy
disabled_rehearsals) to only consult the legacy disabled_rehearsals when
DisableRehearsal is nil, otherwise honor the explicit true/false value.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 588b2109-f9fa-4891-947c-2d638cead2b4
📒 Files selected for processing (21)
pkg/api/types.gopkg/api/zz_generated.deepcopy.gopkg/prowgen/jobbase.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_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_takes_precedence_over_prowgen_config.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/webreg/zz_generated.ci_operator_reference.gotest/integration/ci-operator-prowgen/input/config/prowgen-config/duper/prowgen-config-duper-master.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 skipped from review due to trivial changes (13)
- pkg/prowgen/testdata/zz_fixture_TestGenerateJobs_skip_operator_presubmits_from_ci_operator_config.yaml
- pkg/prowgen/testdata/zz_fixture_TestGenerateJobs_disable_all_rehearsals_from_ci_operator_prowgen_config.yaml
- pkg/prowgen/testdata/zz_fixture_TestGenerateJobs_disable_rehearsal_from_ci_operator_config_per_test.yaml
- pkg/prowgen/testdata/zz_fixture_TestGenerateJobs_postsubmit_with_custom_max_concurrency.yaml
- pkg/prowgen/testdata/zz_fixture_TestGenerateJobs_slack_reporter_from_ci_operator_config_with_explicit_values.yaml
- pkg/prowgen/testdata/zz_fixture_TestGenerateJobs_openshift_priv_org_defaults_to_private.yaml
- pkg/prowgen/testdata/zz_fixture_TestGenerateJobs_private_from_ci_operator_prowgen_config.yaml
- pkg/prowgen/jobbase.go
- pkg/prowgen/testdata/zz_fixture_TestGenerateJobs_slack_reporter_from_ci_operator_config_takes_precedence_over_prowgen_config.yaml
- test/integration/ci-operator-prowgen/output/jobs/prowgen-config/duper/prowgen-config-duper-master-presubmits.yaml
- test/integration/ci-operator-prowgen/input/config/prowgen-config/duper/prowgen-config-duper-master.yaml
- pkg/prowgen/testdata/zz_fixture_TestGenerateJobs_periodic_with_custom_max_concurrency.yaml
- pkg/prowgen/testdata/zz_fixture_TestGenerateJobs_slack_reporter_from_ci_operator_config_with_defaults.yaml
🚧 Files skipped from review as they are similar to previous changes (3)
- pkg/prowgen/testdata/zz_fixture_TestGenerateJobs_private_with_expose_from_ci_operator_prowgen_config.yaml
- pkg/prowgen/prowgen_test.go
- test/integration/ci-operator-prowgen/output/jobs/prowgen-config/duper/prowgen-config-duper-master-periodics.yaml
Member
Author
|
/test e2e |
Prucek
force-pushed
the
deprecate-prowgen-config
branch
from
6cbe1c3 to
7e141a9
Compare
Contributor
|
Scheduling tests matching the |
Contributor
|
Tests from second stage were triggered manually. Pipeline can be controlled only manually, until HEAD changes. Use command to trigger second stage. |
1 similar comment
Contributor
|
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
deprecate-prowgen-config
branch
from
7e141a9 to
9a13242
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
pkg/prowgen/prowgen_test.go (1)
913-999: Add an explicitfalse/ partial-migration case here.These cases mostly assert
truebranches. The risky part of this migration is distinguishing unset from explicitfalsewhile still falling back to legacy.config.prowgen, so one case likeopenshift-priv+ProwgenExtras{Private: ptr.To(false)}(or legacyprivate: truewith only one ci-operator toggle set) would make the new tri-state semantics much harder to regress.🧪 Example case
+ { + id: "openshift-priv can explicitly opt out of private jobs", + keep: true, + config: &ciop.ReleaseBuildConfiguration{ + Prowgen: &ciop.ProwgenExtras{Private: ptr.To(false)}, + Tests: []ciop.TestStepConfiguration{ + {As: "unit", ContainerTestConfiguration: &ciop.ContainerTestConfiguration{From: "bin"}}, + }, + }, + repoInfo: &ProwgenInfo{Metadata: ciop.Metadata{ + Org: "openshift-priv", + 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 913 - 999, Add a test case that exercises the explicit-false triage path for ProwgenExtras.Private to ensure unset vs explicit false is handled: insert a new table entry (similar structure to existing cases) with id like "openshift-priv explicit private false", repoInfo metadata Org: "openshift-priv", and config: &ciop.ReleaseBuildConfiguration{Prowgen: &ciop.ProwgenExtras{Private: ptr.To(false)}, Tests: []ciop.TestStepConfiguration{{As: "unit", ContainerTestConfiguration: &ciop.ContainerTestConfiguration{From: "bin"}}}}, and set keep to false/expected outcome accordingly so the test asserts the code paths in ProwgenExtras.Private (the tri-state behavior) treat explicit false differently than unset or legacy private:true; reference ProwgenExtras, Private, ptr.To(false), and "openshift-priv" from the existing table to locate where to add this case.
🤖 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 78-79: The per-test calculation of disableRehearsal is incorrectly
ORing in the legacy disabledRehearsals set (disabledRehearsals.Has(element.As)),
preventing an explicit per-test Enable (element.DisableRehearsal=false) from
re-enabling a test; change the expression that sets disableRehearsal to only
consider the global disableAllRehearsals and the per-test pointer
(element.DisableRehearsal) so it mirrors the top-level areRehearsalsDisabled
precedence (i.e., remove the disabledRehearsals.Has(element.As) term and keep
disableAllRehearsals and (element.DisableRehearsal != nil &&
*element.DisableRehearsal)).
- Around line 195-197: The current combined condition mixes legacy
SkipPresubmits with the new configSpec.Operator.SkipPresubmits so that a
nil/false operator field does not override legacy behavior; change the logic so
operator.skip_presubmits, when present, is authoritative: first check if
configSpec.Operator != nil && configSpec.Operator.SkipPresubmits != nil and use
the boolean value there (if true skip presubmits, if false do not skip)
otherwise fall back to calling
info.Config.SkipPresubmits(configSpec.Metadata.Branch,
configSpec.Metadata.Variant); update the conditional around presubmit generation
where symbols configSpec.Operator, configSpec.Operator.SkipPresubmits, and
info.Config.SkipPresubmits(...) are used to implement this precedence.
In `@pkg/validation/test.go`:
- Around line 220-222: Inside the SlackReporter validation block (the code
referencing test, validationErrors and fieldRootN), add three checks: ensure
test.SlackReporter.MaxConcurrency (or max_concurrency) is >= 0 and append an
error if negative; validate each entry in test.SlackReporter.JobStatesToReport
(job_states_to_report) against the known ProwJobState enum/allowed set (reject
unknown strings and append errors indicating the invalid value); and
syntactically validate test.SlackReporter.ReportTemplate (report_template) by
attempting to parse/compile it with the template engine used elsewhere (e.g.,
text/template) and append a parse error if it fails. Keep all errors appended to
validationErrors using the same fieldRootN prefix and messages consistent with
the existing channel check.
---
Nitpick comments:
In `@pkg/prowgen/prowgen_test.go`:
- Around line 913-999: Add a test case that exercises the explicit-false triage
path for ProwgenExtras.Private to ensure unset vs explicit false is handled:
insert a new table entry (similar structure to existing cases) with id like
"openshift-priv explicit private false", repoInfo metadata Org:
"openshift-priv", and config: &ciop.ReleaseBuildConfiguration{Prowgen:
&ciop.ProwgenExtras{Private: ptr.To(false)}, Tests:
[]ciop.TestStepConfiguration{{As: "unit", ContainerTestConfiguration:
&ciop.ContainerTestConfiguration{From: "bin"}}}}, and set keep to false/expected
outcome accordingly so the test asserts the code paths in ProwgenExtras.Private
(the tri-state behavior) treat explicit false differently than unset or legacy
private:true; reference ProwgenExtras, Private, ptr.To(false), and
"openshift-priv" from the existing table to locate where to add this case.
🪄 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: a620de8d-23c8-41ab-b7d1-c6b4c3350197
📒 Files selected for processing (22)
pkg/api/types.gopkg/api/zz_generated.deepcopy.gopkg/prowgen/jobbase.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_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_takes_precedence_over_prowgen_config.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/validation/test.gopkg/webreg/zz_generated.ci_operator_reference.gotest/integration/ci-operator-prowgen/input/config/prowgen-config/duper/prowgen-config-duper-master.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 skipped from review due to trivial changes (13)
- pkg/prowgen/testdata/zz_fixture_TestGenerateJobs_skip_operator_presubmits_from_ci_operator_config.yaml
- pkg/prowgen/testdata/zz_fixture_TestGenerateJobs_disable_all_rehearsals_from_ci_operator_prowgen_config.yaml
- pkg/prowgen/testdata/zz_fixture_TestGenerateJobs_disable_rehearsal_from_ci_operator_config_per_test.yaml
- test/integration/ci-operator-prowgen/output/jobs/prowgen-config/duper/prowgen-config-duper-master-postsubmits.yaml
- pkg/prowgen/testdata/zz_fixture_TestGenerateJobs_private_with_expose_from_ci_operator_prowgen_config.yaml
- pkg/prowgen/testdata/zz_fixture_TestGenerateJobs_private_from_ci_operator_prowgen_config.yaml
- test/integration/ci-operator-prowgen/output/jobs/prowgen-config/duper/prowgen-config-duper-master-periodics.yaml
- pkg/prowgen/testdata/zz_fixture_TestGenerateJobs_postsubmit_with_custom_max_concurrency.yaml
- pkg/prowgen/testdata/zz_fixture_TestGenerateJobs_slack_reporter_from_ci_operator_config_takes_precedence_over_prowgen_config.yaml
- pkg/prowgen/testdata/zz_fixture_TestGenerateJobs_openshift_priv_org_defaults_to_private.yaml
- test/integration/ci-operator-prowgen/output/jobs/prowgen-config/duper/prowgen-config-duper-master-presubmits.yaml
- pkg/prowgen/testdata/zz_fixture_TestGenerateJobs_periodic_with_custom_max_concurrency.yaml
- pkg/prowgen/testdata/zz_fixture_TestGenerateJobs_slack_reporter_from_ci_operator_config_with_explicit_values.yaml
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/prowgen/jobbase.go
- pkg/webreg/zz_generated.ci_operator_reference.go
Comment on lines
+78
to
+79
| // ci-operator per-test DisableRehearsal takes precedence, then .config.prowgen | ||
| disableRehearsal := disableAllRehearsals || disabledRehearsals.Has(element.As) || (element.DisableRehearsal != nil && *element.DisableRehearsal) |
There was a problem hiding this comment.
Honor explicit per-test rehearsal overrides.
Line 79 still ORs the legacy disabledRehearsals set into the result, so disable_rehearsal: false cannot re-enable a test that is disabled in .config.prowgen. The top-level path already handles precedence via areRehearsalsDisabled; this per-test path should mirror that behavior.
Suggested fix
- // ci-operator per-test DisableRehearsal takes precedence, then .config.prowgen
- disableRehearsal := disableAllRehearsals || disabledRehearsals.Has(element.As) || (element.DisableRehearsal != nil && *element.DisableRehearsal)
+ disableRehearsal := disableAllRehearsals
+ if !disableRehearsal {
+ if element.DisableRehearsal != nil {
+ disableRehearsal = *element.DisableRehearsal
+ } else {
+ disableRehearsal = disabledRehearsals.Has(element.As)
+ }
+ }📝 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
| // ci-operator per-test DisableRehearsal takes precedence, then .config.prowgen | |
| disableRehearsal := disableAllRehearsals || disabledRehearsals.Has(element.As) || (element.DisableRehearsal != nil && *element.DisableRehearsal) | |
| disableRehearsal := disableAllRehearsals | |
| if !disableRehearsal { | |
| if element.DisableRehearsal != nil { | |
| disableRehearsal = *element.DisableRehearsal | |
| } else { | |
| disableRehearsal = disabledRehearsals.Has(element.As) | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/prowgen/prowgen.go` around lines 78 - 79, The per-test calculation of
disableRehearsal is incorrectly ORing in the legacy disabledRehearsals set
(disabledRehearsals.Has(element.As)), preventing an explicit per-test Enable
(element.DisableRehearsal=false) from re-enabling a test; change the expression
that sets disableRehearsal to only consider the global disableAllRehearsals and
the per-test pointer (element.DisableRehearsal) so it mirrors the top-level
areRehearsalsDisabled precedence (i.e., remove the
disabledRehearsals.Has(element.As) term and keep disableAllRehearsals and
(element.DisableRehearsal != nil && *element.DisableRehearsal)).
Comment on lines
+195
to
+197
| if configSpec.Operator != nil && | ||
| !info.Config.SkipPresubmits(configSpec.Metadata.Branch, configSpec.Metadata.Variant) && | ||
| !(configSpec.Operator.SkipPresubmits != nil && *configSpec.Operator.SkipPresubmits) { |
There was a problem hiding this comment.
Let operator.skip_presubmits fully override legacy config.
Lines 195-197 only add another skip condition. If .config.prowgen says to skip and the new field is explicitly false, bundle presubmits are still suppressed, which breaks the documented precedence for migrated repos.
Suggested fix
+ skipOperatorPresubmits := false
+ if configSpec.Operator != nil {
+ skipOperatorPresubmits = info.Config.SkipPresubmits(configSpec.Metadata.Branch, configSpec.Metadata.Variant)
+ if configSpec.Operator.SkipPresubmits != nil {
+ skipOperatorPresubmits = *configSpec.Operator.SkipPresubmits
+ }
+ }
- if configSpec.Operator != nil &&
- !info.Config.SkipPresubmits(configSpec.Metadata.Branch, configSpec.Metadata.Variant) &&
- !(configSpec.Operator.SkipPresubmits != nil && *configSpec.Operator.SkipPresubmits) {
+ if configSpec.Operator != nil && !skipOperatorPresubmits {📝 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
| if configSpec.Operator != nil && | |
| !info.Config.SkipPresubmits(configSpec.Metadata.Branch, configSpec.Metadata.Variant) && | |
| !(configSpec.Operator.SkipPresubmits != nil && *configSpec.Operator.SkipPresubmits) { | |
| skipOperatorPresubmits := false | |
| if configSpec.Operator != nil { | |
| skipOperatorPresubmits = info.Config.SkipPresubmits(configSpec.Metadata.Branch, configSpec.Metadata.Variant) | |
| if configSpec.Operator.SkipPresubmits != nil { | |
| skipOperatorPresubmits = *configSpec.Operator.SkipPresubmits | |
| } | |
| } | |
| if configSpec.Operator != nil && !skipOperatorPresubmits { |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/prowgen/prowgen.go` around lines 195 - 197, The current combined
condition mixes legacy SkipPresubmits with the new
configSpec.Operator.SkipPresubmits so that a nil/false operator field does not
override legacy behavior; change the logic so operator.skip_presubmits, when
present, is authoritative: first check if configSpec.Operator != nil &&
configSpec.Operator.SkipPresubmits != nil and use the boolean value there (if
true skip presubmits, if false do not skip) otherwise fall back to calling
info.Config.SkipPresubmits(configSpec.Metadata.Branch,
configSpec.Metadata.Variant); update the conditional around presubmit generation
where symbols configSpec.Operator, configSpec.Operator.SkipPresubmits, and
info.Config.SkipPresubmits(...) are used to implement this precedence.
Comment on lines
+220
to
+222
| if test.SlackReporter != nil && test.SlackReporter.Channel == "" { | ||
| validationErrors = append(validationErrors, fmt.Errorf("%s.slack_reporter.channel: must be set when slack_reporter is configured", fieldRootN)) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate and examine the types.go file to confirm the new fields
fd -t f "types.go" | grep -E "(api|pkg)"Repository: openshift/ci-tools
Length of output: 10014
🏁 Script executed:
# Look for the TestStepConfiguration and SlackReporter definitions
rg "type TestStepConfiguration|type SlackReporter" -A 15Repository: openshift/ci-tools
Length of output: 7025
🏁 Script executed:
# Examine the current validation code at lines 220-222 and surrounding context
cat -n pkg/validation/test.go | sed -n '210,235p'Repository: openshift/ci-tools
Length of output: 1387
🏁 Script executed:
# Search for any existing validation of MaxConcurrency, JobStatesToReport, ReportTemplate
rg "MaxConcurrency|JobStatesToReport|ReportTemplate" pkg/validation/Repository: openshift/ci-tools
Length of output: 44
🏁 Script executed:
# Get the full TestStepConfiguration struct definition from pkg/api/types.go
rg "type TestStepConfiguration struct" -A 50 pkg/api/types.goRepository: openshift/ci-tools
Length of output: 2090
🏁 Script executed:
# Verify what fields exist in TestStepConfiguration around MaxConcurrency
rg "MaxConcurrency" pkg/api/types.go -B 5 -A 5Repository: openshift/ci-tools
Length of output: 929
🏁 Script executed:
# Check the ProwJobState type definition to understand valid values
rg "type ProwJobState" -A 10Repository: openshift/ci-tools
Length of output: 1157
🏁 Script executed:
# Look for any validation of SlackReporter fields elsewhere in the codebase
rg "JobStatesToReport|ReportTemplate" pkg/ -lRepository: openshift/ci-tools
Length of output: 282
🏁 Script executed:
# Check how SlackReporter is used in the codebase to understand the impact
rg "SlackReporter" pkg/ -B 2 -A 2 | head -50Repository: openshift/ci-tools
Length of output: 3242
🏁 Script executed:
# Check if there's any validation of MaxConcurrency in the codebase
rg "MaxConcurrency" pkg/validation/ -B 3 -A 3Repository: openshift/ci-tools
Length of output: 44
🏁 Script executed:
# Look for any template validation logic
rg "ReportTemplate|template.Parse" pkg/ -B 2 -A 2Repository: openshift/ci-tools
Length of output: 6791
🏁 Script executed:
# Check the prowgen code to see how SlackReporter is used when generating jobs
rg "SlackReporter" pkg/prowgen/prowgen.go -B 5 -A 5Repository: openshift/ci-tools
Length of output: 5708
🏁 Script executed:
# Verify the suggested diff syntax is correct and similar to existing validation patterns
rg "validationErrors = append.*MaxConcurrency|validationErrors = append.*negative|must be greater" pkg/validation/ -B 2 -A 2Repository: openshift/ci-tools
Length of output: 922
🏁 Script executed:
# Check how maxConcurrency function handles the value
rg "func maxConcurrency" pkg/prowgen/prowgen.go -A 10Repository: openshift/ci-tools
Length of output: 337
Add validation for max_concurrency, job_states_to_report, and report_template.
The new validation at line 220 only checks that slack_reporter.channel exists. The other newly added fields remain unvalidated: max_concurrency can be negative, job_states_to_report accepts arbitrary strings without checking against valid ProwJobState values, and report_template isn't syntactically checked. This allows malformed configs to pass validation and fail later at runtime or silently drop Slack notifications.
🔧 Suggested start
if test.SlackReporter != nil && test.SlackReporter.Channel == "" {
validationErrors = append(validationErrors, fmt.Errorf("%s.slack_reporter.channel: must be set when slack_reporter is configured", fieldRootN))
}
+ if test.MaxConcurrency != nil && *test.MaxConcurrency < 0 {
+ validationErrors = append(validationErrors, fmt.Errorf("%s.max_concurrency: must be greater than or equal to 0", fieldRootN))
+ }📝 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
| if test.SlackReporter != nil && test.SlackReporter.Channel == "" { | |
| validationErrors = append(validationErrors, fmt.Errorf("%s.slack_reporter.channel: must be set when slack_reporter is configured", fieldRootN)) | |
| } | |
| if test.SlackReporter != nil && test.SlackReporter.Channel == "" { | |
| validationErrors = append(validationErrors, fmt.Errorf("%s.slack_reporter.channel: must be set when slack_reporter is configured", fieldRootN)) | |
| } | |
| if test.MaxConcurrency != nil && *test.MaxConcurrency < 0 { | |
| validationErrors = append(validationErrors, fmt.Errorf("%s.max_concurrency: must be greater than or equal to 0", fieldRootN)) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/validation/test.go` around lines 220 - 222, Inside the SlackReporter
validation block (the code referencing test, validationErrors and fieldRootN),
add three checks: ensure test.SlackReporter.MaxConcurrency (or max_concurrency)
is >= 0 and append an error if negative; validate each entry in
test.SlackReporter.JobStatesToReport (job_states_to_report) against the known
ProwJobState enum/allowed set (reject unknown strings and append errors
indicating the invalid value); and syntactically validate
test.SlackReporter.ReportTemplate (report_template) by attempting to
parse/compile it with the template engine used elsewhere (e.g., text/template)
and append a parse error if it fails. Keep all errors appended to
validationErrors using the same fieldRootN prefix and messages consistent with
the existing channel check.
Member
Author
|
/retest-required |
Contributor
|
@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. |
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.
This PR adds new fields to the ci-operator configuration to replace the
.config.prowgenfile, as a first step toward deprecating it.New fields:
Else: