Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (21)
✅ Files skipped from review due to trivial changes (13)
🚧 Files skipped from review as they are similar to previous changes (3)
WalkthroughAdds new Slack reporting and per-test concurrency/rehearsal controls via new API types ( 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)
📝 Coding Plan
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.3)Command failed Comment |
|
[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
pkg/api/types.go
Outdated
| type ProwgenExtras struct { | ||
| // Private indicates that generated jobs should be marked as hidden | ||
| // from display in deck and that they should mount appropriate git credentials | ||
| // to clone the repository under test. | ||
| Private bool `json:"private,omitempty"` | ||
| // Expose declares that jobs should not be hidden from view in deck if they | ||
| // are private. This field has no effect if private is not set. | ||
| Expose bool `json:"expose,omitempty"` | ||
| // DisableRehearsals prevents all tests in this config from being rehearsed. | ||
| DisableRehearsals bool `json:"disable_rehearsals,omitempty"` | ||
| // EnableSecretsStoreCSIDriver indicates that jobs should use the new CSI Secrets Store | ||
| // mechanism to handle multi-stage credentials secrets. | ||
| EnableSecretsStoreCSIDriver bool `json:"enable_secrets_store_csi_driver,omitempty"` |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Go, when unmarshalling JSON/YAML into a bool field tagged with omitempty, can code distinguish an omitted field from an explicit falsewithout using*bool or a custom type?
💡 Result:
No—not when you unmarshal directly into a struct field of type bool.
omitemptyaffects marshaling only; it “has no effect when unmarshaling.” [1] (Same idea ingopkg.in/yaml.v3: theomitemptyflag is described as a marshaling control.) [2]- When a field is omitted in the input, Go’s unmarshaler simply doesn’t assign it, so it stays at the Go zero value; for
boolthat’sfalse, which is indistinguishable from an explicitly providedfalse. [3]
If you can’t use *bool or a custom type, the remaining option is an out-of-band presence check, e.g. unmarshal into a map[string]json.RawMessage / map[string]any (or yaml.Node) to see whether the key exists, then decode into your struct.
Sources: [1] encoding/json/v2 docs on struct tags (omitempty has no effect when unmarshaling) https://pkg.go.dev/encoding/json/v2, [2] yaml.v3 docs (omitempty flag described under Marshal/tag behavior) https://pkg.go.dev/gopkg.in/yaml.v3, [3] Eli Bendersky on missing fields keeping Go zero values during json.Unmarshal https://eli.thegreenplace.net/2020/optional-json-fields-in-go/
🏁 Script executed:
# Locate and inspect the exact field locations mentioned
echo "=== Checking ProwgenExtras in pkg/api/types.go ==="
rg -n "type ProwgenExtras" pkg/api/types.go -A 15
echo -e "\n=== Checking lines 2759-2760 in pkg/api/types.go ==="
sed -n '2755,2765p' pkg/api/types.go
echo -e "\n=== Checking if *bool is already used in types.go ==="
rg "\*bool" pkg/api/types.go | head -20
echo -e "\n=== Checking downstream helpers in jobbase.go ==="
rg -n "isPrivate|isExposed|isSecretsStoreCSIDriverEnabled" pkg/prowgen/jobbase.go -A 3Repository: openshift/ci-tools
Length of output: 3903
🏁 Script executed:
# Check how ProwgenExtras fields are accessed in pkg/prowgen/prowgen.go
echo "=== Checking GenerateJobs usage of Prowgen fields ==="
rg -n "\.Private|\.Expose|\.DisableRehearsals|\.EnableSecretsStoreCSIDriver" pkg/prowgen/ -B 2 -A 2Repository: openshift/ci-tools
Length of output: 1751
🏁 Script executed:
# Verify the OperatorStepConfiguration struct location
echo "=== Checking OperatorStepConfiguration and SkipPresubmits ==="
rg -n "type OperatorStepConfiguration" pkg/api/types.go -A 30 | grep -i "skip"Repository: openshift/ci-tools
Length of output: 317
Use tri-state booleans for the migrated prowgen toggles.
These fields need "set vs unset" semantics. Plain bool cannot distinguish between omitted and explicit false during JSON/YAML unmarshal—both result in false. This breaks fallback behavior: an openshift-priv repo migrating only prowgen.expose: true will unmarshal private as false, and the helper logic in pkg/prowgen/jobbase.go will return false instead of falling back to the legacy check. The same issue affects all four ProwgenExtras fields and OperatorStepConfiguration.SkipPresubmits.
Use *bool instead. The codebase already follows this pattern for optional booleans (AlwaysRun, RestrictNetworkAccess, etc.). Downstream code in isPrivate(), isExposed(), and isSecretsStoreCSIDriverEnabled() will need to check for nil before dereferencing, which enables correct fallback semantics.
Suggested direction
type ProwgenExtras struct {
- Private bool `json:"private,omitempty"`
- Expose bool `json:"expose,omitempty"`
- DisableRehearsals bool `json:"disable_rehearsals,omitempty"`
- EnableSecretsStoreCSIDriver bool `json:"enable_secrets_store_csi_driver,omitempty"`
+ Private *bool `json:"private,omitempty"`
+ Expose *bool `json:"expose,omitempty"`
+ DisableRehearsals *bool `json:"disable_rehearsals,omitempty"`
+ EnableSecretsStoreCSIDriver *bool `json:"enable_secrets_store_csi_driver,omitempty"`
}
...
- SkipPresubmits bool `json:"skip_presubmits,omitempty"`
+ SkipPresubmits *bool `json:"skip_presubmits,omitempty"`Also applies to: 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 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.
...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
|
/test e2e |
Prucek
force-pushed
the
deprecate-prowgen-config
branch
from
6cbe1c3 to
7e141a9
Compare
|
Scheduling tests matching the |
|
@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. |
|
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
|
Tests from second stage were triggered manually. Pipeline can be controlled only manually, until HEAD changes. Use command to trigger second stage. |
2 participants
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: