Repository navigation
prowgen: allow ci-operator config to enable CSI secrets store - #5120
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: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds an optional Prowgen override Changes
Sequence Diagram(s)sequenceDiagram
participant CI as ci-operator config (configSpec)
participant Info as runtime ProwgenInfo (info.Config)
participant Builder as NewProwJobBaseBuilderForTest
participant Pod as PodSpec
participant K8s as Kubernetes
CI->>Builder: provide configSpec (may include Prowgen.EnableSecretsStoreCSIDriver)
Info->>Builder: provide ProwgenInfo (may include EnableSecretsStoreCSIDriver)
Builder->>Builder: evaluate Info.EnableSecretsStoreCSIDriver OR configSpec.Prowgen.EnableSecretsStoreCSIDriver
alt CSI enabled
Builder->>Pod: add GSMConfig() and CSI volume/mounts
Pod->>K8s: rendered pod spec with CSI volume, secretProviderClass, mounts
else CSI disabled
Builder->>Pod: leave pod spec without GSMConfig()/CSI mounts
Pod->>K8s: rendered pod spec without CSI additions
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 11 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (11 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
pkg/prowgen/jobbase.go (1)
150-168: Extract CSI enablement predicate once to avoid branch drift.The same condition is duplicated in both multi-stage branches. Consider computing it once before the
switchand reusing it.♻️ Proposed refactor
+ enableSecretsStoreCSI := info.Config.EnableSecretsStoreCSIDriver || + (configSpec.Prowgen != nil && configSpec.Prowgen.EnableSecretsStoreCSIDriver) + switch { case test.MultiStageTestConfigurationLiteral != nil: p.PodSpec.Add(LeaseClient()) @@ - if info.Config.EnableSecretsStoreCSIDriver || (configSpec.Prowgen != nil && configSpec.Prowgen.EnableSecretsStoreCSIDriver) { + if enableSecretsStoreCSI { p.PodSpec.Add( GSMConfig(), ) } case test.MultiStageTestConfiguration != nil: @@ - if info.Config.EnableSecretsStoreCSIDriver || (configSpec.Prowgen != nil && configSpec.Prowgen.EnableSecretsStoreCSIDriver) { + if enableSecretsStoreCSI { p.PodSpec.Add( GSMConfig(), ) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/prowgen/jobbase.go` around lines 150 - 168, Extract the repeated CSI enablement predicate into a single boolean before the switch and reuse it in both branches: compute something like csiEnabled := info.Config.EnableSecretsStoreCSIDriver || (configSpec.Prowgen != nil && configSpec.Prowgen.EnableSecretsStoreCSIDriver) and replace both occurrences of the long condition used to decide p.PodSpec.Add(GSMConfig()) in the case handling test.MultiStageTestConfiguration and the other branch so the switch body uses the single csiEnabled variable.pkg/prowgen/jobbase_test.go (1)
357-369: Add a sibling test for literal multi-stage + ci-operator CSI override.This covers the non-literal multi-stage path well. Consider adding the same scenario for
MultiStageTestConfigurationLiteralto lock in behavior for both branches updated inNewProwJobBaseBuilderForTest.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/prowgen/jobbase_test.go` around lines 357 - 369, Add a sibling table-driven test case that mirrors the existing "multi-stage test with CSI enabled via ci-operator config" but uses MultiStageTestConfigurationLiteral instead of MultiStageTestConfiguration so the literal-path in NewProwJobBaseBuilderForTest is covered; create a case with name like "multi-stage literal test with CSI enabled via ci-operator config", cfg: ProwgenOverrides{EnableSecretsStoreCSIDriver: true}, test: ciop.TestStepConfiguration{As: "simple", MultiStageTestConfigurationLiteral: &ciop.MultiStageTestConfigurationLiteral{Workflow: pointer.StringPtr("workflow")}}, and info: defaultInfo, then run the same assertions as the non-literal case.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/prowgen/jobbase_test.go`:
- Around line 357-369: Add a sibling table-driven test case that mirrors the
existing "multi-stage test with CSI enabled via ci-operator config" but uses
MultiStageTestConfigurationLiteral instead of MultiStageTestConfiguration so the
literal-path in NewProwJobBaseBuilderForTest is covered; create a case with name
like "multi-stage literal test with CSI enabled via ci-operator config", cfg:
ProwgenOverrides{EnableSecretsStoreCSIDriver: true}, test:
ciop.TestStepConfiguration{As: "simple", MultiStageTestConfigurationLiteral:
&ciop.MultiStageTestConfigurationLiteral{Workflow:
pointer.StringPtr("workflow")}}, and info: defaultInfo, then run the same
assertions as the non-literal case.
In `@pkg/prowgen/jobbase.go`:
- Around line 150-168: Extract the repeated CSI enablement predicate into a
single boolean before the switch and reuse it in both branches: compute
something like csiEnabled := info.Config.EnableSecretsStoreCSIDriver ||
(configSpec.Prowgen != nil && configSpec.Prowgen.EnableSecretsStoreCSIDriver)
and replace both occurrences of the long condition used to decide
p.PodSpec.Add(GSMConfig()) in the case handling test.MultiStageTestConfiguration
and the other branch so the switch body uses the single csiEnabled variable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 96d2694c-c9af-41a5-9493-6f7b651be28e
⛔ Files ignored due to path filters (1)
pkg/webreg/zz_generated.ci_operator_reference.gois excluded by!**/zz_generated*
📒 Files selected for processing (4)
pkg/api/types.gopkg/prowgen/jobbase.gopkg/prowgen/jobbase_test.gopkg/prowgen/testdata/zz_fixture_TestNewProwJobBaseBuilderForTest_multi_stage_test_with_CSI_enabled_via_ci_operator_config.yaml
|
/lgtm |
|
/skip |
|
Tests from second stage were triggered manually. Pipeline can be controlled only manually, until HEAD changes. Use command to trigger second stage. |
|
@Prucek: Overrode contexts on behalf of Prucek: ci/prow/e2e, ci/prow/images DetailsIn response to this:
Instructions 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. |
|
Tests from second stage were triggered manually. Pipeline can be controlled only manually, until HEAD changes. Use command to trigger second stage. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/prowgen/jobbase.go (1)
150-150: De-duplicate CSI enablement predicate across both branches.The condition is correct, but it is repeated in two places. Extracting it into a helper keeps future changes safer and reduces drift risk.
♻️ Proposed refactor
+func enableSecretsStoreCSIDriver(info *ProwgenInfo, configSpec *cioperatorapi.ReleaseBuildConfiguration) bool { + return info.Config.EnableSecretsStoreCSIDriver || + (configSpec.Prowgen != nil && configSpec.Prowgen.EnableSecretsStoreCSIDriver) +} ... - if info.Config.EnableSecretsStoreCSIDriver || (configSpec.Prowgen != nil && configSpec.Prowgen.EnableSecretsStoreCSIDriver) { + if enableSecretsStoreCSIDriver(info, configSpec) { p.PodSpec.Add( GSMConfig(), ) } ... - if info.Config.EnableSecretsStoreCSIDriver || (configSpec.Prowgen != nil && configSpec.Prowgen.EnableSecretsStoreCSIDriver) { + if enableSecretsStoreCSIDriver(info, configSpec) { p.PodSpec.Add( GSMConfig(), ) }Also applies to: 164-164
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/prowgen/jobbase.go` at line 150, Extract the repeated predicate into a single helper to avoid duplication: create a function (e.g., enableSecretsStoreCSIDriver(info, configSpec) bool or a method on the enclosing type) that returns true if info.Config.EnableSecretsStoreCSIDriver || (configSpec.Prowgen != nil && configSpec.Prowgen.EnableSecretsStoreCSIDriver). Replace both occurrences of the inline condition in jobbase.go (the branches that currently check info.Config.EnableSecretsStoreCSIDriver || (configSpec.Prowgen != nil && configSpec.Prowgen.EnableSecretsStoreCSIDriver)) with a call to this new helper.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/prowgen/jobbase.go`:
- Line 150: Extract the repeated predicate into a single helper to avoid
duplication: create a function (e.g., enableSecretsStoreCSIDriver(info,
configSpec) bool or a method on the enclosing type) that returns true if
info.Config.EnableSecretsStoreCSIDriver || (configSpec.Prowgen != nil &&
configSpec.Prowgen.EnableSecretsStoreCSIDriver). Replace both occurrences of the
inline condition in jobbase.go (the branches that currently check
info.Config.EnableSecretsStoreCSIDriver || (configSpec.Prowgen != nil &&
configSpec.Prowgen.EnableSecretsStoreCSIDriver)) with a call to this new helper.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 789d6a55-c091-427f-b6ff-d5b0bb0b7109
⛔ Files ignored due to path filters (1)
pkg/webreg/zz_generated.ci_operator_reference.gois excluded by!**/zz_generated*
📒 Files selected for processing (4)
pkg/api/types.gopkg/prowgen/jobbase.gopkg/prowgen/jobbase_test.gopkg/prowgen/testdata/zz_fixture_TestNewProwJobBaseBuilderForTest_multi_stage_test_with_CSI_enabled_via_ci_operator_config.yaml
✅ Files skipped from review due to trivial changes (1)
- pkg/prowgen/testdata/zz_fixture_TestNewProwJobBaseBuilderForTest_multi_stage_test_with_CSI_enabled_via_ci_operator_config.yaml
Add enable_secrets_store_csi_driver to the prowgen section of ci-operator config, so it can be set directly instead of requiring a .config.prowgen file. This is the second step toward deprecating .config.prowgen. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: danilo-gemoli, 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 |
|
/skip |
|
Scheduling tests matching the |
|
@Prucek: Overrode contexts on behalf of Prucek: ci/prow/e2e, ci/prow/images DetailsIn response to this:
Instructions 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. |
|
@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. |
Move enable_secrets_store_csi_driver and skip_operator_presubmits settings from .config.prowgen files into ci-operator config's prowgen section. CSI migration (2 repos): - openshift/ci-tools - psalajova/test skip_operator_presubmits migration (3 repos): - opendatahub-io/opendatahub-operator (main) - openshift/csi-operator (4.17-4.20 periodics) - openshift/gcp-filestore-csi-driver-operator (4.17-4.20 periodics) Depends on openshift/ci-tools#5120 and openshift/ci-tools#5121. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ig (#78232) Move enable_secrets_store_csi_driver and skip_operator_presubmits settings from .config.prowgen files into ci-operator config's prowgen section. CSI migration (2 repos): - openshift/ci-tools - psalajova/test skip_operator_presubmits migration (3 repos): - opendatahub-io/opendatahub-operator (main) - openshift/csi-operator (4.17-4.20 periodics) - openshift/gcp-filestore-csi-driver-operator (4.17-4.20 periodics) Depends on openshift/ci-tools#5120 and openshift/ci-tools#5121. Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
…ig (openshift#78232) Move enable_secrets_store_csi_driver and skip_operator_presubmits settings from .config.prowgen files into ci-operator config's prowgen section. CSI migration (2 repos): - openshift/ci-tools - psalajova/test skip_operator_presubmits migration (3 repos): - opendatahub-io/opendatahub-operator (main) - openshift/csi-operator (4.17-4.20 periodics) - openshift/gcp-filestore-csi-driver-operator (4.17-4.20 periodics) Depends on openshift/ci-tools#5120 and openshift/ci-tools#5121. Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
…ig (openshift#78232) Move enable_secrets_store_csi_driver and skip_operator_presubmits settings from .config.prowgen files into ci-operator config's prowgen section. CSI migration (2 repos): - openshift/ci-tools - psalajova/test skip_operator_presubmits migration (3 repos): - opendatahub-io/opendatahub-operator (main) - openshift/csi-operator (4.17-4.20 periodics) - openshift/gcp-filestore-csi-driver-operator (4.17-4.20 periodics) Depends on openshift/ci-tools#5120 and openshift/ci-tools#5121. Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
…ig (openshift#78232) Move enable_secrets_store_csi_driver and skip_operator_presubmits settings from .config.prowgen files into ci-operator config's prowgen section. CSI migration (2 repos): - openshift/ci-tools - psalajova/test skip_operator_presubmits migration (3 repos): - opendatahub-io/opendatahub-operator (main) - openshift/csi-operator (4.17-4.20 periodics) - openshift/gcp-filestore-csi-driver-operator (4.17-4.20 periodics) Depends on openshift/ci-tools#5120 and openshift/ci-tools#5121. Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
…ig (openshift#78232) Move enable_secrets_store_csi_driver and skip_operator_presubmits settings from .config.prowgen files into ci-operator config's prowgen section. CSI migration (2 repos): - openshift/ci-tools - psalajova/test skip_operator_presubmits migration (3 repos): - opendatahub-io/opendatahub-operator (main) - openshift/csi-operator (4.17-4.20 periodics) - openshift/gcp-filestore-csi-driver-operator (4.17-4.20 periodics) Depends on openshift/ci-tools#5120 and openshift/ci-tools#5121. Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
…ig (openshift#78232) Move enable_secrets_store_csi_driver and skip_operator_presubmits settings from .config.prowgen files into ci-operator config's prowgen section. CSI migration (2 repos): - openshift/ci-tools - psalajova/test skip_operator_presubmits migration (3 repos): - opendatahub-io/opendatahub-operator (main) - openshift/csi-operator (4.17-4.20 periodics) - openshift/gcp-filestore-csi-driver-operator (4.17-4.20 periodics) Depends on openshift/ci-tools#5120 and openshift/ci-tools#5121. Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
…ig (openshift#78232) Move enable_secrets_store_csi_driver and skip_operator_presubmits settings from .config.prowgen files into ci-operator config's prowgen section. CSI migration (2 repos): - openshift/ci-tools - psalajova/test skip_operator_presubmits migration (3 repos): - opendatahub-io/opendatahub-operator (main) - openshift/csi-operator (4.17-4.20 periodics) - openshift/gcp-filestore-csi-driver-operator (4.17-4.20 periodics) Depends on openshift/ci-tools#5120 and openshift/ci-tools#5121. Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
…ig (openshift#78232) Move enable_secrets_store_csi_driver and skip_operator_presubmits settings from .config.prowgen files into ci-operator config's prowgen section. CSI migration (2 repos): - openshift/ci-tools - psalajova/test skip_operator_presubmits migration (3 repos): - opendatahub-io/opendatahub-operator (main) - openshift/csi-operator (4.17-4.20 periodics) - openshift/gcp-filestore-csi-driver-operator (4.17-4.20 periodics) Depends on openshift/ci-tools#5120 and openshift/ci-tools#5121. Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
…ig (openshift#78232) Move enable_secrets_store_csi_driver and skip_operator_presubmits settings from .config.prowgen files into ci-operator config's prowgen section. CSI migration (2 repos): - openshift/ci-tools - psalajova/test skip_operator_presubmits migration (3 repos): - opendatahub-io/opendatahub-operator (main) - openshift/csi-operator (4.17-4.20 periodics) - openshift/gcp-filestore-csi-driver-operator (4.17-4.20 periodics) Depends on openshift/ci-tools#5120 and openshift/ci-tools#5121. Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
…ig (openshift#78232) Move enable_secrets_store_csi_driver and skip_operator_presubmits settings from .config.prowgen files into ci-operator config's prowgen section. CSI migration (2 repos): - openshift/ci-tools - psalajova/test skip_operator_presubmits migration (3 repos): - opendatahub-io/opendatahub-operator (main) - openshift/csi-operator (4.17-4.20 periodics) - openshift/gcp-filestore-csi-driver-operator (4.17-4.20 periodics) Depends on openshift/ci-tools#5120 and openshift/ci-tools#5121. Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
Summary
enable_secrets_store_csi_driverto theprowgensection of ci-operator config.config.prowgenfile.config.prowgen(follows prowgen: allow ci-operator config to override .config.prowgen #5119 which added rehearsal control)Test plan
.config.prowgen-based CSI testgo test ./pkg/prowgen/...)🤖 Generated with Claude Code
Summary by CodeRabbit