Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
WalkthroughAdds Slack reporting and per-test concurrency/rehearsal controls, introduces ProwgenExtras and SlackReporter API types, threads those options through prowgen job generation (with explicit precedence), refactors private/expose and CSI driver checks, updates deepcopy and webreg reference, and extends tests and fixtures to cover the new behaviors. 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
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
| 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.
| limits: | ||
| cpu: 500Mi | ||
| requests: | ||
| cpu: 10Mi |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Kubernetes resource requests/limits, is Mia valid unit for CPU, and should millicores be expressed withm instead?
💡 Result:
No—Mi should not be used for CPU requests/limits. In Kubernetes, CPU is expressed in CPU units (cores), and fractional CPU is typically written as millicpu / millicores using the m suffix (e.g., 100m = 0.1 CPU). Kubernetes’ own docs describe CPU this way and use m for milli-CPU. [1]
Mi is an IEC binary prefix (“mebi”, 2²⁰) that belongs to the generic Kubernetes Quantity syntax (used commonly for bytes like memory), alongside Ki, Gi, etc. [2] For CPU, the correct milli-unit is m, not Mi. [1]
Examples
- CPU:
cpu: "250m"orcpu: "0.25"[1] - Memory:
memory: "128Mi"[1]
Sources: [1], [2]
Fix the CPU quantity units in this fixture.
cpu: 500Mi and cpu: 10Mi use a binary memory suffix (Mi = mebi, 2²⁰), which is invalid for CPU. CPU quantities should use the m suffix for millicores. Change these to cpu: 500m and cpu: 10m respectively.
Suggested fix
resources:
'*':
limits:
- cpu: 500Mi
+ cpu: 500m
requests:
- cpu: 10Mi
+ cpu: 10m📝 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.
| limits: | |
| cpu: 500Mi | |
| requests: | |
| cpu: 10Mi | |
| limits: | |
| cpu: 500m | |
| requests: | |
| cpu: 10m |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@test/integration/ci-operator-prowgen/input/config/prowgen-config/duper/prowgen-config-duper-master.yaml`
around lines 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".
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
| // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. | ||
| func (in *ProwgenExtras) DeepCopyInto(out *ProwgenExtras) { | ||
| *out = *in | ||
| } | ||
|
|
||
| // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new ProwgenExtras. | ||
| func (in *ProwgenExtras) DeepCopy() *ProwgenExtras { | ||
| if in == nil { | ||
| return nil | ||
| } | ||
| out := new(ProwgenExtras) | ||
| in.DeepCopyInto(out) | ||
| return out | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== New ci-operator prowgen fields =="
sed -n '/type ProwgenExtras struct {/,/}/p' pkg/api/types.go
echo
echo "== Legacy .config.prowgen fields =="
sed -n '/type Prowgen struct {/,/}/p' pkg/config/load.go
echo
echo "== Other migrated knobs worth checking in the same pass =="
sed -n '/type TestStepConfiguration struct {/,/}/p' pkg/api/types.go | rg -n 'DisableRehearsal|MaxConcurrency|SlackReporter'
sed -n '/type OperatorStepConfiguration struct {/,/}/p' pkg/api/types.go | rg -n 'SkipPresubmits'
echo
echo "== Call sites that merge or read these settings =="
rg -n -C4 'Prowgen|DisableRehearsals|EnableSecretsStoreCSIDriver|SkipPresubmits|DisableRehearsal|private|expose|rehears' \
pkg/prowgen/prowgen.go pkg/config/load.go pkg/api/types.goRepository: openshift/ci-tools
Length of output: 2139
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Prowgen merge/fallback logic in prowgen.go =="
# Look for how Prowgen and ProwgenExtras are merged or used
rg -A 10 -B 2 'func.*[Pp]rowgen|Private|Expose|DisableRehear' pkg/prowgen/prowgen.go | head -100
echo
echo "== Config loading/merge in config/load.go =="
# Look for how legacy Prowgen is loaded and merged
rg -A 5 -B 2 'Prowgen|Private|Expose' pkg/config/load.go | head -80
echo
echo "== Check if any pointer usage in ProwgenExtras =="
grep -A 20 'type ProwgenExtras struct' pkg/api/types.goRepository: openshift/ci-tools
Length of output: 8356
🏁 Script executed:
#!/bin/bash
# Check if there's explicit field presence tracking or optional bool patterns
echo "== Search for optional/pointer patterns in related types =="
rg '\*bool|\*Prowgen' pkg/api/types.go pkg/config/load.go pkg/prowgen/prowgen.go
echo
echo "== Check DeepCopy behavior and merge points =="
# Look for where DeepCopyInto is called with Prowgen
rg -B 5 -A 5 'DeepCopyInto.*[Pp]rowgen|Prowgen.*DeepCopyInto' pkg/prowgen/prowgen.goRepository: openshift/ci-tools
Length of output: 2680
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== How Private/Expose are applied to jobs =="
rg -B 3 -A 3 'Private|Expose' pkg/prowgen/prowgen.go | grep -A 3 -B 3 'Private\|Expose' | head -50
echo
echo "== Check if fallback reads both ci-operator and .config.prowgen =="
rg -B 5 -A 5 'info.Config.*Private|info.Config.*Expose|configSpec.Prowgen.*Private' pkg/prowgen/prowgen.go
echo
echo "== Verify how ProwgenInfo.Config is populated =="
rg -B 2 -A 10 'type ProwgenInfo struct' pkg/prowgen/prowgen.go
echo
echo "== Check actual merge between ci-operator and .config.prowgen =="
rg -B 3 -A 8 'func.*Merge|ApplyDefaults' pkg/prowgen/prowgen.go pkg/config/load.go | head -80Repository: openshift/ci-tools
Length of output: 106
🏁 Script executed:
#!/bin/bash
# Check the ProwJobBaseBuilder to see how job properties are set
rg -B 2 -A 10 'private|expose|Private|Expose' pkg/prowgen/jobs.go 2>/dev/null | head -100Repository: openshift/ci-tools
Length of output: 44
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Where info.Config is populated and used =="
rg -B 5 -A 8 'info\.Config\s*=' pkg/prowgen/prowgen.go | head -60
echo
echo "== Search for where Private and Expose are actually read =="
rg 'Private|Expose' pkg/prowgen/jobs.go pkg/prowgen/prowgen.go | grep -v '//' | head -40
echo
echo "== Check the ProwgenInfo struct definition =="
rg -A 15 'type ProwgenInfo struct' pkg/prowgen/
echo
echo "== Check how .config.prowgen settings are read vs ci-operator Prowgen =="
rg -B 2 -A 5 'info\.Config\.Private|info\.Config\.Expose|configSpec\.Prowgen\.Private' pkg/prowgen/prowgen.goRepository: openshift/ci-tools
Length of output: 107
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Find all files that reference Private or Expose =="
fd . pkg/prowgen/ pkg/config/ -type f -name '*.go' | head -20
echo
echo "== Search broadly for Private/Expose usage =="
rg 'Private|Expose' pkg/prowgen/ --type go -l
echo
echo "== Check ProwgenInfo struct definition =="
find pkg/prowgen -name '*.go' -exec grep -l 'ProwgenInfo' {} \; | head -5
rg -A 10 'type ProwgenInfo' pkg/
echo
echo "== See the full context of how DisableRehearsals is used =="
rg -B 3 -A 3 'DisableRehearsals' pkg/prowgen/prowgen.goRepository: openshift/ci-tools
Length of output: 303
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== ProwgenInfo struct definition =="
rg -A 15 'type ProwgenInfo struct' pkg/
echo
echo "== Check full context of DisableRehearsals precedence =="
rg -B 5 -A 8 'disableAllRehearsals :=' pkg/prowgen/prowgen.go
echo
echo "== Look for where job properties like Hidden are set =="
rg -n 'Hidden|Private|\.Private' pkg/prowgen/ -A 2 -B 2
echo
echo "== Check how prowgen config is merged into job generation =="
rg -B 3 -A 5 'info\.Config\.' pkg/prowgen/prowgen.go | head -60Repository: openshift/ci-tools
Length of output: 6479
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Check the isExposed function for same pattern =="
rg -A 10 'func isExposed' pkg/prowgen/jobbase.go
echo
echo "== Verify the precedence is all-or-nothing =="
rg -B 2 -A 8 'if configSpec.Prowgen != nil' pkg/prowgen/prowgen.go | head -40
echo
echo "== Check if there's any merging logic for ProwgenExtras fields =="
rg -B 3 -A 10 'Prowgen.*Merge|MergeProwgen' pkg/prowgen/ pkg/config/Repository: openshift/ci-tools
Length of output: 642
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Full isSecretsStoreCSIDriverEnabled function =="
rg -A 10 'func isSecretsStoreCSIDriverEnabled' pkg/prowgen/jobbase.go
echo
echo "== Test cases showing the migration risk =="
rg -B 5 -A 8 'Prowgen.*Private|Prowgen.*Expose' pkg/prowgen/prowgen_test.go | head -80Repository: openshift/ci-tools
Length of output: 1549
Use pointers for bool fields in ProwgenExtras to preserve fallback semantics during partial migrations.
The precedence logic in pkg/prowgen/jobbase.go checks if configSpec.Prowgen != nil to decide between ci-operator and .config.prowgen sources. Once a repo adds any prowgen: block in the ci-operator config, all plain bool fields default to false if not explicitly set—making it impossible to preserve legacy .config.prowgen settings like private: true. For example, a repo with .config.prowgen: {private: true} that migrates with prowgen: {expose: true} will silently lose the private flag.
Change Private, Expose, DisableRehearsals, and EnableSecretsStoreCSIDriver to *bool in ProwgenExtras so the merge logic can distinguish between "not set" (nil), "explicit false", and "explicit true". Regenerate the deepcopy methods afterward.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/api/zz_generated.deepcopy.go` around lines 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.
|
@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. |
|
/test e2e |
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: