ROX-33465: Remove OCP3 from install and more reliable autosensing#19996
Merged
mclasmeier merged 20 commits intomasterfrom Apr 15, 2026
Merged
ROX-33465: Remove OCP3 from install and more reliable autosensing#19996mclasmeier merged 20 commits intomasterfrom
mclasmeier merged 20 commits intomasterfrom
Conversation
|
Skipping CI for Draft Pull Request. |
Contributor
There was a problem hiding this comment.
Hey - I've found 2 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="tests/roxctl/bats-tests/cluster/sensor-generate-bundle.bats" line_range="53-56" />
<code_context>
assert_failure
}
+
+@test "[openshift3] roxctl sensor generate rejects unsupported openshift version" {
+ generate_bundle openshift --name oc3-test-cluster --openshift-version 3
+ assert_failure
+ assert_output --partial "only '4' is currently supported"
+}
</code_context>
<issue_to_address>
**suggestion (testing):** Extend the negative test to verify no bundle artifacts are generated for unsupported OpenShift 3
Since this already verifies the failure and error message, please also assert that the bundle output directory (or key files within it) does not exist after the command fails. That will confirm we never leave partial bundles behind for unsupported versions.
Suggested implementation:
```shell
@test "[openshift3] roxctl sensor generate rejects unsupported openshift version" {
generate_bundle openshift --name oc3-test-cluster --openshift-version 3
assert_failure
assert_output --partial "only '4' is currently supported"
# ensure that no bundle artifacts are created for unsupported OpenShift versions
[ ! -d "${BUNDLE_OUTPUT_DIR}" ]
[ ! -e "${BUNDLE_OUTPUT_DIR}/sensor-deployment.yaml" ]
}
```
1. Make sure `BUNDLE_OUTPUT_DIR` is set consistently with where `generate_bundle` writes its bundle content (if the helper currently uses a different variable/path, adjust the assertions accordingly).
2. If the main bundle entry file has a different name than `sensor-deployment.yaml`, update that filename in the second assertion to match a key bundle artifact that would normally be produced for supported versions.
</issue_to_address>
### Comment 2
<location path="tests/roxctl/bats-tests/cluster/scanner-generate.bats" line_range="57-54" />
<code_context>
assert_number_of_k8s_resources 14
}
-@test "[openshift] roxctl scanner generate" {
- run_scanner_generate_and_check openshift
-
- assert_file_exist "${output_dir}/scanner/02-scanner-06-deployment.yaml"
- run -0 grep -q 'ROX_OPENSHIFT_API' "${output_dir}/scanner/02-scanner-06-deployment.yaml"
- run -1 grep -q 'trusted-ca-volume' "${output_dir}/scanner/02-scanner-06-deployment.yaml"
- assert_number_of_k8s_resources 14
-}
-
</code_context>
<issue_to_address>
**issue (testing):** Reintroduce a scanner generate test for OpenShift 4 to keep coverage for that supported platform
The OpenShift scanner generate test was removed along with OpenShift 3, but scanner generation for OpenShift 4 is still supported. Please add a new test (e.g., `[openshift4] roxctl scanner generate`) that invokes `run_scanner_generate_and_check openshift` with `--openshift-version 4` and verifies OpenShift-specific behavior such as `ROX_OPENSHIFT_API`, the trusted CA volume, and the expected resource count, so we maintain end-to-end coverage for OpenShift 4.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
added 2 commits
April 14, 2026 23:07
05bd5b7 to
1193636
Compare
Contributor
📝 WalkthroughWalkthroughThis pull request removes OpenShift version 3 support across the entire codebase. Changes span Helm templating, Go code for central and sensor generation, test fixtures, and integration tests. OpenShift 3 references are eliminated from installation logic, configuration templates, and validation rules, with error handling added to reject OpenShift 3 deployments. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
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
`@image/templates/helm/stackrox-secured-cluster/internal/compatibility-translation.yaml`:
- Line 15: The current translation emits env.openshift as a boolean which breaks
downstream numeric checks (e.g., templates/_init.tpl.htpl expecting numeric 4);
update the compatibility-translation so env.openshift yields the numeric value
used by consumers (emit integer 4 when .rawValue == "OPENSHIFT4_CLUSTER",
otherwise emit the prior numeric/default value) and ensure the output is an
unquoted integer so comparisons like eq ... 4 continue to work; adjust the
mapping that produces env.openshift in compatibility-translation.yaml
accordingly and verify related checks in templates/_init.tpl.htpl still evaluate
correctly.
In `@pkg/renderer/central_db_test.go`:
- Around line 189-211: Add a validation in postProcessConfig (or at start of
render) to detect when conf.ClusterType == storage.ClusterType_OPENSHIFT_CLUSTER
and return an error with the exact message "You have specified OpenShift version
3"; update postProcessConfig (the function that finalizes/validates Config) to
check conf.K8sConfig.EnableCentralDB and conf.ClusterType and return this error
immediately so the TestRenderCentralDBBundleFailsForOpenShift3 assertion
succeeds.
🪄 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: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 04644c96-ef36-414f-ab6f-c68b74aef49d
📒 Files selected for processing (42)
CHANGELOG.mdimage/embed_charts.goimage/templates/helm/shared/templates/02-scanner-v4-01-security.yamlimage/templates/helm/shared/templates/_openshift.tplimage/templates/helm/stackrox-secured-cluster/internal/cluster-config.yaml.tpl.htplimage/templates/helm/stackrox-secured-cluster/internal/compatibility-translation.yamlimage/templates/helm/stackrox-secured-cluster/internal/defaults/30-base-config.yaml.htplimage/templates/helm/stackrox-secured-cluster/sensor-chart-upgrade.md.htplimage/templates/helm/stackrox-secured-cluster/templates/_init.tpl.htplimage/templates/helm/stackrox-secured-cluster/templates/admission-controller.yamlimage/templates/helm/stackrox-secured-cluster/values.yaml.htplpkg/helm/charts/tests/centralservices/testdata/helmtest/central.test.yamlpkg/helm/charts/tests/centralservices/testdata/helmtest/injected-cabundle-cm.test.yamlpkg/helm/charts/tests/centralservices/testdata/helmtest/openshift-auth.test.yamlpkg/helm/charts/tests/centralservices/testdata/helmtest/openshift-autosense.test.yamlpkg/helm/charts/tests/centralservices/testdata/helmtest/openshift-monitoring.test.yamlpkg/helm/charts/tests/centralservices/testdata/helmtest/scanner-v4.test.yamlpkg/helm/charts/tests/centralservices/testdata/helmtest/scanner.test.yamlpkg/helm/charts/tests/securedclusterservices/feature-flags/testdata/helmtest/admission-controller-config-disabled/admission-control.test.yamlpkg/helm/charts/tests/securedclusterservices/feature-flags/testdata/helmtest/admission-controller-config/admission-control.test.yamlpkg/helm/charts/tests/securedclusterservices/flavor/testdata/helmtest/development_build-non-release/development_build.test.yamlpkg/helm/charts/tests/securedclusterservices/flavor/testdata/helmtest/development_build-release/development_build.test.yamlpkg/helm/charts/tests/securedclusterservices/flavor/testdata/helmtest/opensource-non-release/opensource.test.yamlpkg/helm/charts/tests/securedclusterservices/flavor/testdata/helmtest/opensource-release/opensource.test.yamlpkg/helm/charts/tests/securedclusterservices/flavor/testdata/helmtest/override/override.test.yamlpkg/helm/charts/tests/securedclusterservices/flavor/testdata/helmtest/rhacs/rhacs.test.yamlpkg/helm/charts/tests/securedclusterservices/flavor/testdata/helmtest/stackrox/stackrox.test.yamlpkg/helm/charts/tests/securedclusterservices/testdata/helmtest/admission-control.test.yamlpkg/helm/charts/tests/securedclusterservices/testdata/helmtest/audit-logs.test.yamlpkg/helm/charts/tests/securedclusterservices/testdata/helmtest/env.test.yamlpkg/helm/charts/tests/securedclusterservices/testdata/helmtest/injected-cabundle-cm.test.yamlpkg/helm/charts/tests/securedclusterservices/testdata/helmtest/legacy-settings.test.yamlpkg/helm/charts/tests/securedclusterservices/testdata/helmtest/openshift-monitoring.test.yamlpkg/helm/charts/tests/securedclusterservices/testdata/helmtest/scanner-slim.test.yamlpkg/helm/charts/tests/securedclusterservices/testdata/helmtest/scanner-v4.test.yamlpkg/renderer/central_db_test.gopkg/renderer/kubernetes_test.goroxctl/central/generate/generate_test.goroxctl/central/generate/k8s.goroxctl/sensor/generate/openshift.gotests/roxctl/bats-tests/cluster/scanner-generate.batstests/roxctl/bats-tests/cluster/sensor-generate-bundle.bats
💤 Files with no reviewable changes (6)
- image/templates/helm/stackrox-secured-cluster/templates/_init.tpl.htpl
- image/templates/helm/stackrox-secured-cluster/values.yaml.htpl
- pkg/helm/charts/tests/securedclusterservices/feature-flags/testdata/helmtest/admission-controller-config/admission-control.test.yaml
- roxctl/central/generate/generate_test.go
- image/templates/helm/stackrox-secured-cluster/internal/defaults/30-base-config.yaml.htpl
- tests/roxctl/bats-tests/cluster/scanner-generate.bats
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
image/embed_charts.go (1)
271-278: Make the OpenShift 3 rejection explicit and include the cluster type in the errorLine 277 currently reports a generic “invalid cluster type,” which now also catches
OPENSHIFT_CLUSTER(a known deprecated enum). Please add an explicitOPENSHIFT_CLUSTERbranch with a deprecation message, and includevalues.ClusterTypein the default error for better diagnostics.Suggested patch
func (i *Image) addScripts(values *charts.MetaValues) ([]*loader.BufferedFile, error) { if values.ClusterType == storage.ClusterType_KUBERNETES_CLUSTER.String() { return i.scripts(values, k8sScriptsFileMap) } else if values.ClusterType == storage.ClusterType_OPENSHIFT4_CLUSTER.String() { return i.scripts(values, osScriptsFileMap) + } else if values.ClusterType == storage.ClusterType_OPENSHIFT_CLUSTER.String() { + return nil, errors.Errorf("unable to create sensor bundle for cluster %q: OpenShift 3 is no longer supported", values.ClusterName) } - return nil, errors.Errorf("unable to create sensor bundle, invalid cluster type for cluster %s", - values.ClusterName) + return nil, errors.Errorf("unable to create sensor bundle for cluster %q: invalid cluster type %q", + values.ClusterName, values.ClusterType) }As per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@image/embed_charts.go` around lines 271 - 278, In addScripts, handle the deprecated OpenShift 3 enum explicitly: add an else-if branch checking if values.ClusterType == storage.ClusterType_OPENSHIFT_CLUSTER.String() and return an error that clearly marks OpenShift 3 as deprecated (e.g., "OpenShift 3 (OPENSHIFT_CLUSTER) is deprecated for cluster <ClusterName>"), then keep the existing branches that call i.scripts with k8sScriptsFileMap and osScriptsFileMap for KUBERNETES_CLUSTER and OPENSHIFT4_CLUSTER respectively; also change the final default error to include values.ClusterType (along with values.ClusterName) so the fallback error reports the actual cluster type string.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@image/embed_charts.go`:
- Around line 271-278: In addScripts, handle the deprecated OpenShift 3 enum
explicitly: add an else-if branch checking if values.ClusterType ==
storage.ClusterType_OPENSHIFT_CLUSTER.String() and return an error that clearly
marks OpenShift 3 as deprecated (e.g., "OpenShift 3 (OPENSHIFT_CLUSTER) is
deprecated for cluster <ClusterName>"), then keep the existing branches that
call i.scripts with k8sScriptsFileMap and osScriptsFileMap for
KUBERNETES_CLUSTER and OPENSHIFT4_CLUSTER respectively; also change the final
default error to include values.ClusterType (along with values.ClusterName) so
the fallback error reports the actual cluster type string.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 52121fd2-913f-4617-9d9b-10cc00dc88e7
📒 Files selected for processing (42)
CHANGELOG.mdimage/embed_charts.goimage/templates/helm/shared/templates/02-scanner-v4-01-security.yamlimage/templates/helm/shared/templates/_openshift.tplimage/templates/helm/stackrox-secured-cluster/internal/cluster-config.yaml.tpl.htplimage/templates/helm/stackrox-secured-cluster/internal/compatibility-translation.yamlimage/templates/helm/stackrox-secured-cluster/internal/defaults/30-base-config.yaml.htplimage/templates/helm/stackrox-secured-cluster/sensor-chart-upgrade.md.htplimage/templates/helm/stackrox-secured-cluster/templates/_init.tpl.htplimage/templates/helm/stackrox-secured-cluster/templates/admission-controller.yamlimage/templates/helm/stackrox-secured-cluster/values.yaml.htplpkg/helm/charts/tests/centralservices/testdata/helmtest/central.test.yamlpkg/helm/charts/tests/centralservices/testdata/helmtest/injected-cabundle-cm.test.yamlpkg/helm/charts/tests/centralservices/testdata/helmtest/openshift-auth.test.yamlpkg/helm/charts/tests/centralservices/testdata/helmtest/openshift-autosense.test.yamlpkg/helm/charts/tests/centralservices/testdata/helmtest/openshift-monitoring.test.yamlpkg/helm/charts/tests/centralservices/testdata/helmtest/scanner-v4.test.yamlpkg/helm/charts/tests/centralservices/testdata/helmtest/scanner.test.yamlpkg/helm/charts/tests/securedclusterservices/feature-flags/testdata/helmtest/admission-controller-config-disabled/admission-control.test.yamlpkg/helm/charts/tests/securedclusterservices/feature-flags/testdata/helmtest/admission-controller-config/admission-control.test.yamlpkg/helm/charts/tests/securedclusterservices/flavor/testdata/helmtest/development_build-non-release/development_build.test.yamlpkg/helm/charts/tests/securedclusterservices/flavor/testdata/helmtest/development_build-release/development_build.test.yamlpkg/helm/charts/tests/securedclusterservices/flavor/testdata/helmtest/opensource-non-release/opensource.test.yamlpkg/helm/charts/tests/securedclusterservices/flavor/testdata/helmtest/opensource-release/opensource.test.yamlpkg/helm/charts/tests/securedclusterservices/flavor/testdata/helmtest/override/override.test.yamlpkg/helm/charts/tests/securedclusterservices/flavor/testdata/helmtest/rhacs/rhacs.test.yamlpkg/helm/charts/tests/securedclusterservices/flavor/testdata/helmtest/stackrox/stackrox.test.yamlpkg/helm/charts/tests/securedclusterservices/testdata/helmtest/admission-control.test.yamlpkg/helm/charts/tests/securedclusterservices/testdata/helmtest/audit-logs.test.yamlpkg/helm/charts/tests/securedclusterservices/testdata/helmtest/env.test.yamlpkg/helm/charts/tests/securedclusterservices/testdata/helmtest/injected-cabundle-cm.test.yamlpkg/helm/charts/tests/securedclusterservices/testdata/helmtest/legacy-settings.test.yamlpkg/helm/charts/tests/securedclusterservices/testdata/helmtest/openshift-monitoring.test.yamlpkg/helm/charts/tests/securedclusterservices/testdata/helmtest/scanner-slim.test.yamlpkg/helm/charts/tests/securedclusterservices/testdata/helmtest/scanner-v4.test.yamlpkg/renderer/central_db_test.gopkg/renderer/kubernetes_test.goroxctl/central/generate/generate_test.goroxctl/central/generate/k8s.goroxctl/sensor/generate/openshift.gotests/roxctl/bats-tests/cluster/scanner-generate.batstests/roxctl/bats-tests/cluster/sensor-generate-bundle.bats
💤 Files with no reviewable changes (6)
- image/templates/helm/stackrox-secured-cluster/templates/_init.tpl.htpl
- roxctl/central/generate/generate_test.go
- image/templates/helm/stackrox-secured-cluster/values.yaml.htpl
- image/templates/helm/stackrox-secured-cluster/internal/defaults/30-base-config.yaml.htpl
- tests/roxctl/bats-tests/cluster/scanner-generate.bats
- pkg/helm/charts/tests/securedclusterservices/feature-flags/testdata/helmtest/admission-controller-config/admission-control.test.yaml
✅ Files skipped from review due to trivial changes (16)
- pkg/helm/charts/tests/securedclusterservices/flavor/testdata/helmtest/stackrox/stackrox.test.yaml
- pkg/helm/charts/tests/securedclusterservices/flavor/testdata/helmtest/override/override.test.yaml
- pkg/helm/charts/tests/securedclusterservices/flavor/testdata/helmtest/rhacs/rhacs.test.yaml
- pkg/helm/charts/tests/securedclusterservices/flavor/testdata/helmtest/opensource-release/opensource.test.yaml
- pkg/helm/charts/tests/centralservices/testdata/helmtest/central.test.yaml
- pkg/helm/charts/tests/securedclusterservices/flavor/testdata/helmtest/development_build-release/development_build.test.yaml
- image/templates/helm/stackrox-secured-cluster/sensor-chart-upgrade.md.htpl
- pkg/helm/charts/tests/securedclusterservices/flavor/testdata/helmtest/opensource-non-release/opensource.test.yaml
- pkg/helm/charts/tests/securedclusterservices/flavor/testdata/helmtest/development_build-non-release/development_build.test.yaml
- pkg/helm/charts/tests/securedclusterservices/feature-flags/testdata/helmtest/admission-controller-config-disabled/admission-control.test.yaml
- pkg/helm/charts/tests/centralservices/testdata/helmtest/scanner-v4.test.yaml
- pkg/helm/charts/tests/securedclusterservices/testdata/helmtest/admission-control.test.yaml
- pkg/helm/charts/tests/centralservices/testdata/helmtest/openshift-auth.test.yaml
- CHANGELOG.md
- pkg/helm/charts/tests/centralservices/testdata/helmtest/scanner.test.yaml
- pkg/helm/charts/tests/securedclusterservices/testdata/helmtest/env.test.yaml
🚧 Files skipped from review as they are similar to previous changes (14)
- image/templates/helm/stackrox-secured-cluster/internal/cluster-config.yaml.tpl.htpl
- pkg/helm/charts/tests/securedclusterservices/testdata/helmtest/injected-cabundle-cm.test.yaml
- image/templates/helm/shared/templates/02-scanner-v4-01-security.yaml
- pkg/helm/charts/tests/centralservices/testdata/helmtest/injected-cabundle-cm.test.yaml
- pkg/helm/charts/tests/securedclusterservices/testdata/helmtest/scanner-v4.test.yaml
- pkg/renderer/kubernetes_test.go
- pkg/helm/charts/tests/centralservices/testdata/helmtest/openshift-autosense.test.yaml
- pkg/helm/charts/tests/centralservices/testdata/helmtest/openshift-monitoring.test.yaml
- tests/roxctl/bats-tests/cluster/sensor-generate-bundle.bats
- image/templates/helm/stackrox-secured-cluster/internal/compatibility-translation.yaml
- image/templates/helm/shared/templates/_openshift.tpl
- pkg/helm/charts/tests/securedclusterservices/testdata/helmtest/openshift-monitoring.test.yaml
- pkg/renderer/central_db_test.go
- pkg/helm/charts/tests/securedclusterservices/testdata/helmtest/legacy-settings.test.yaml
Contributor
🚀 Build Images ReadyImages are ready for commit 4881eec. To use with deploy scripts: export MAIN_IMAGE_TAG=4.11.x-659-g4881eec00f |
added 16 commits
April 14, 2026 23:29
…openshift.io/v1 (openshift 4 only).
1193636 to
25e3bcb
Compare
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The various error messages for unsupported OpenShift versions (e.g.
sensor generate/central generateCLI, Helm rendering failure, and tests asserting on message substrings) are very similar but not identical; consider centralizing them in a shared helper/constant so we don’t accidentally diverge behavior or break tests on future wording tweaks. - In
srox.autoSenseOpenshiftVersion, the new logic that infers OpenShift 4 via theconfig.openshift.io/v1API and mapsenv.openshift=trueunconditionally to 4 would benefit from an explicit inline comment on what happens when no OpenShift-specific APIs are visible (especially forhelm templateuse), to clarify that this is now treated as non-OpenShift rather than defaulting to 3 as before.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The various error messages for unsupported OpenShift versions (e.g. `sensor generate`/`central generate` CLI, Helm rendering failure, and tests asserting on message substrings) are very similar but not identical; consider centralizing them in a shared helper/constant so we don’t accidentally diverge behavior or break tests on future wording tweaks.
- In `srox.autoSenseOpenshiftVersion`, the new logic that infers OpenShift 4 via the `config.openshift.io/v1` API and maps `env.openshift=true` unconditionally to 4 would benefit from an explicit inline comment on what happens when no OpenShift-specific APIs are visible (especially for `helm template` use), to clarify that this is now treated as non-OpenShift rather than defaulting to 3 as before.
## Individual Comments
### Comment 1
<location path="pkg/renderer/central_db_test.go" line_range="189-198" />
<code_context>
}
}
+
+func (suite *centralDBTestSuite) TestRenderCentralDBBundleFailsForOpenShift3() {
+ centralFileMap := make(map[string][]byte, 4)
+ centralFileMap["central-db-password"] = []byte("Apassword")
+ centralFileMap["central-db-cert.pem"] = suite.centralDBCert.CertPEM
+ centralFileMap["central-db-key.pem"] = suite.centralDBCert.KeyPEM
+ centralFileMap[mtls.CACertFileName] = suite.testCA.CertPEM()
+
+ conf := Config{
+ ClusterType: storage.ClusterType_KUBERNETES_CLUSTER,
+ K8sConfig: &K8sConfig{
+ CommonConfig: CommonConfig{
+ CentralDBImage: "stackrox/central-db:2.2.11.0-57-g392c0f5bed-dirty",
+ },
+ EnableCentralDB: true,
+ },
+ SecretsByteMap: centralFileMap,
+ }
+ conf.K8sConfig.DeploymentFormat = v1.DeploymentFormat_KUBECTL
+ conf.ClusterType = storage.ClusterType_OPENSHIFT_CLUSTER
+ _, err := render(conf, centralDBOnly, suite.testFlavor)
+ require.Error(suite.T(), err)
+ assert.Contains(suite.T(), err.Error(), "You have specified OpenShift version 3")
+}
</code_context>
<issue_to_address>
**suggestion (testing):** Strengthen the negative test by asserting the error type/cause, not only its message
Since this already verifies that OpenShift 3 is rejected and checks part of the message, please also assert on a specific error type or sentinel (e.g. `errox.InvalidArgs` or a dedicated error var, if available). That keeps the test resilient to message rewording while still validating the intended failure behavior.
Suggested implementation:
```golang
func (suite *centralDBTestSuite) TestRenderCentralDBBundleFailsForOpenShift3() {
centralFileMap := make(map[string][]byte, 4)
centralFileMap["central-db-password"] = []byte("Apassword")
centralFileMap["central-db-cert.pem"] = suite.centralDBCert.CertPEM
centralFileMap["central-db-key.pem"] = suite.centralDBCert.KeyPEM
centralFileMap[mtls.CACertFileName] = suite.testCA.CertPEM()
conf := Config{
ClusterType: storage.ClusterType_KUBERNETES_CLUSTER,
K8sConfig: &K8sConfig{
CommonConfig: CommonConfig{
CentralDBImage: "stackrox/central-db:2.2.11.0-57-g392c0f5bed-dirty",
},
EnableCentralDB: true,
},
SecretsByteMap: centralFileMap,
}
conf.K8sConfig.DeploymentFormat = v1.DeploymentFormat_KUBECTL
conf.ClusterType = storage.ClusterType_OPENSHIFT_CLUSTER
_, err := render(conf, centralDBOnly, suite.testFlavor)
require.Error(suite.T(), err)
assert.True(suite.T(), errox.Is(err, errox.InvalidArgs), "expected InvalidArgs error for OpenShift 3 central DB rendering")
assert.Contains(suite.T(), err.Error(), "You have specified OpenShift version 3")
}
```
```golang
import (
"github.com/stackrox/rox/pkg/errox"
```
I assumed the project uses the common StackRox `errox` helper `errox.Is(err, errox.InvalidArgs)` to classify errors. If the actual error helper differs (e.g. a dedicated sentinel like `ErrOpenShift3Unsupported` or a different `errox` API), adjust the assertion accordingly, for example:
- `assert.ErrorIs(suite.T(), err, ErrOpenShift3Unsupported)`
- or `assert.True(suite.T(), errox.Is(err, errox.SomeOtherKind))`
Also ensure the `import` block in `central_db_test.go` is deduplicated and sorted according to your existing import conventions.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
6 tasks
Contributor
Author
|
/retest |
3642756 to
f56ea2d
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@image/templates/helm/shared/templates/_openshift.tpl`:
- Around line 32-40: Reject non-numeric env.openshift strings before calling
int: before the line that does {{ $_ := set $env "openshift" (int
$env.openshift) }} add a guard that uses regexMatch (e.g., regexMatch "^\\d+$"
$env.openshift) to ensure the value is a numeric string; if it fails, call
include "srox.fail" with a clear error about invalid non-numeric OpenShift
version (referencing $env.openshift), otherwise proceed to set using int; keep
existing kindIs and boolean handling and the later include "srox.fail" for
unsupported numeric versions.
🪄 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: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: a7749df9-e229-4d66-b768-6d52970a2d11
📒 Files selected for processing (37)
central/clusters/zip/render_test.goimage/embed_charts.goimage/templates/helm/shared/templates/_openshift.tplimage/templates/helm/stackrox-secured-cluster/internal/cluster-config.yaml.tpl.htplimage/templates/helm/stackrox-secured-cluster/sensor-chart-upgrade.md.htplimage/templates/helm/stackrox-secured-cluster/values.yaml.htplpkg/helm/charts/tests/centralservices/testdata/helmtest/central.test.yamlpkg/helm/charts/tests/centralservices/testdata/helmtest/injected-cabundle-cm.test.yamlpkg/helm/charts/tests/centralservices/testdata/helmtest/openshift-auth.test.yamlpkg/helm/charts/tests/centralservices/testdata/helmtest/openshift-autosense.test.yamlpkg/helm/charts/tests/centralservices/testdata/helmtest/openshift-monitoring.test.yamlpkg/helm/charts/tests/centralservices/testdata/helmtest/scanner-v4.test.yamlpkg/helm/charts/tests/centralservices/testdata/helmtest/scanner.test.yamlpkg/helm/charts/tests/securedclusterservices/feature-flags/testdata/helmtest/admission-controller-config-disabled/admission-control.test.yamlpkg/helm/charts/tests/securedclusterservices/feature-flags/testdata/helmtest/admission-controller-config/admission-control.test.yamlpkg/helm/charts/tests/securedclusterservices/flavor/testdata/helmtest/development_build-non-release/development_build.test.yamlpkg/helm/charts/tests/securedclusterservices/flavor/testdata/helmtest/development_build-release/development_build.test.yamlpkg/helm/charts/tests/securedclusterservices/flavor/testdata/helmtest/opensource-non-release/opensource.test.yamlpkg/helm/charts/tests/securedclusterservices/flavor/testdata/helmtest/opensource-release/opensource.test.yamlpkg/helm/charts/tests/securedclusterservices/flavor/testdata/helmtest/override/override.test.yamlpkg/helm/charts/tests/securedclusterservices/flavor/testdata/helmtest/rhacs/rhacs.test.yamlpkg/helm/charts/tests/securedclusterservices/flavor/testdata/helmtest/stackrox/stackrox.test.yamlpkg/helm/charts/tests/securedclusterservices/testdata/helmtest/admission-control.test.yamlpkg/helm/charts/tests/securedclusterservices/testdata/helmtest/audit-logs.test.yamlpkg/helm/charts/tests/securedclusterservices/testdata/helmtest/env.test.yamlpkg/helm/charts/tests/securedclusterservices/testdata/helmtest/injected-cabundle-cm.test.yamlpkg/helm/charts/tests/securedclusterservices/testdata/helmtest/legacy-settings.test.yamlpkg/helm/charts/tests/securedclusterservices/testdata/helmtest/openshift-monitoring.test.yamlpkg/helm/charts/tests/securedclusterservices/testdata/helmtest/scanner-slim.test.yamlpkg/helm/charts/tests/securedclusterservices/testdata/helmtest/scanner-v4.test.yamlpkg/renderer/central_db_test.gopkg/renderer/kubernetes_test.goroxctl/central/generate/generate_test.goroxctl/central/generate/k8s.goroxctl/sensor/generate/openshift.gotests/roxctl/bats-tests/cluster/scanner-generate.batstests/roxctl/bats-tests/cluster/sensor-generate-bundle.bats
💤 Files with no reviewable changes (4)
- image/templates/helm/stackrox-secured-cluster/values.yaml.htpl
- tests/roxctl/bats-tests/cluster/scanner-generate.bats
- roxctl/central/generate/generate_test.go
- pkg/helm/charts/tests/securedclusterservices/feature-flags/testdata/helmtest/admission-controller-config/admission-control.test.yaml
✅ Files skipped from review due to trivial changes (13)
- pkg/helm/charts/tests/securedclusterservices/flavor/testdata/helmtest/stackrox/stackrox.test.yaml
- pkg/helm/charts/tests/securedclusterservices/flavor/testdata/helmtest/development_build-non-release/development_build.test.yaml
- pkg/helm/charts/tests/securedclusterservices/flavor/testdata/helmtest/opensource-non-release/opensource.test.yaml
- pkg/helm/charts/tests/securedclusterservices/flavor/testdata/helmtest/rhacs/rhacs.test.yaml
- pkg/helm/charts/tests/securedclusterservices/flavor/testdata/helmtest/override/override.test.yaml
- pkg/helm/charts/tests/securedclusterservices/flavor/testdata/helmtest/opensource-release/opensource.test.yaml
- pkg/helm/charts/tests/centralservices/testdata/helmtest/central.test.yaml
- pkg/helm/charts/tests/securedclusterservices/flavor/testdata/helmtest/development_build-release/development_build.test.yaml
- pkg/helm/charts/tests/centralservices/testdata/helmtest/scanner.test.yaml
- pkg/helm/charts/tests/centralservices/testdata/helmtest/injected-cabundle-cm.test.yaml
- pkg/helm/charts/tests/centralservices/testdata/helmtest/openshift-auth.test.yaml
- image/templates/helm/stackrox-secured-cluster/sensor-chart-upgrade.md.htpl
- pkg/helm/charts/tests/centralservices/testdata/helmtest/scanner-v4.test.yaml
🚧 Files skipped from review as they are similar to previous changes (11)
- image/embed_charts.go
- image/templates/helm/stackrox-secured-cluster/internal/cluster-config.yaml.tpl.htpl
- tests/roxctl/bats-tests/cluster/sensor-generate-bundle.bats
- pkg/helm/charts/tests/securedclusterservices/testdata/helmtest/scanner-v4.test.yaml
- pkg/helm/charts/tests/securedclusterservices/feature-flags/testdata/helmtest/admission-controller-config-disabled/admission-control.test.yaml
- pkg/renderer/central_db_test.go
- pkg/helm/charts/tests/securedclusterservices/testdata/helmtest/openshift-monitoring.test.yaml
- pkg/renderer/kubernetes_test.go
- pkg/helm/charts/tests/centralservices/testdata/helmtest/openshift-monitoring.test.yaml
- pkg/helm/charts/tests/securedclusterservices/testdata/helmtest/env.test.yaml
- pkg/helm/charts/tests/securedclusterservices/testdata/helmtest/audit-logs.test.yaml
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #19996 +/- ##
==========================================
- Coverage 49.62% 49.61% -0.01%
==========================================
Files 2765 2765
Lines 208599 208595 -4
==========================================
- Hits 103511 103498 -13
- Misses 97435 97440 +5
- Partials 7653 7657 +4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
vikin91
pushed a commit
that referenced
this pull request
Apr 16, 2026
…9996) Co-authored-by: Moritz Clasmeier <mclasmeier@redhat.com>
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.
Description
This is a reduced version of #19937, which tries harder to focus on install methods only.
@porridge, I don't think it would be a good idea to slice the PR to have the auto-sensing improvement separate, because that change is heavily intertwined with the removal of the OCP3 support from the Helm charts (reason being that the API we want to probe for is OCP4-only). Hence, removing OCP3 from the Helm templating depends on the new auto-sensing in the sense that it will deny OCP3 configurations provided by the user and the new auto-sensing depends on the OCP3 removal because it can only detect OCP4 and hence it doesn't make sense to carry around Helm templates capable of producing OCP3 manifests.
But I did remove several commits from #19937 which are not directly related to the install changes: proto deprecation, cluster validation and related tests.
User-facing documentation
Testing and quality
Automated testing
How I validated my change
Automated tests.