Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
WalkthroughThe changes introduce Slack reporting capabilities and per-test configuration options for Prow job generation. New API types encapsulate SlackReporter configuration, ProwgenExtras settings, and extend existing configuration types with optional fields for disabling rehearsals, setting max concurrency, and controlling operator presubmits. Prowgen job generation logic is refactored to apply these configurations with explicit precedence rules. 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: 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. |
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:
Summary by CodeRabbit