deprecate all Openshift templates related code#4952
Open
droslean wants to merge 1 commit intoopenshift:mainfrom
Open
deprecate all Openshift templates related code#4952droslean wants to merge 1 commit intoopenshift:mainfrom
droslean wants to merge 1 commit intoopenshift:mainfrom
Conversation
Contributor
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
WalkthroughThis pull request removes template processing support and OpenShift installer-based test configurations from the CI operator. Changes include deletion of template clientset wiring, removal of multiple OpenShiftAnsible* and OpenShiftInstaller* configuration types from the API, elimination of template migration functionality, and cleanup of related test fixtures and validation logic. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
openshift-ci
bot
added
the
approved
Member
Author
|
/hold Holding to babyshit this after merge. |
openshift-ci
bot
added
the
do-not-merge/hold
droslean
force-pushed
the
templates-code-cleanup
branch
2 times, most recently
from
6ae5712 to
fb8181c
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/defaults/defaults.go (1)
817-826:⚠️ Potential issue | 🟠 MajorInclude unresolved
MultiStageTestConfigurationtests in the build graph.The condition at line 819 only processes literal test configurations. However, registry resolution is optional—if no
registryPathis provided toloadConfig, unresolvedMultiStageTestConfigurationtests are never converted to literal form and will be silently dropped from the graph. This is a silent failure that hides misconfiguration.Include
test.MultiStageTestConfigurationin the condition to ensure these tests are added to the step graph, or guarantee that all configs enteringFromConfigStatichave been resolved:Suggested fix
- if test.ContainerTestConfiguration != nil || test.MultiStageTestConfigurationLiteral != nil { + if test.ContainerTestConfiguration != nil || test.MultiStageTestConfigurationLiteral != nil || test.MultiStageTestConfiguration != nil {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/defaults/defaults.go` around lines 817 - 826, The loop over config.Tests is currently only adding tests with ContainerTestConfiguration or MultiStageTestConfigurationLiteral to buildSteps, which drops unresolved MultiStageTestConfiguration entries; update the condition in the loop to also include test.MultiStageTestConfiguration (i.e. treat tests with MultiStageTestConfiguration as valid candidates to append to buildSteps) or alternatively ensure loadConfig/FromConfigStatic always resolves MultiStageTestConfiguration into MultiStageTestConfigurationLiteral before this loop; reference symbols: config.Tests, test.MultiStageTestConfiguration, test.MultiStageTestConfigurationLiteral, test.ContainerTestConfiguration, loadConfig, FromConfigStatic, buildSteps, api.StepConfiguration.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@pkg/defaults/defaults.go`:
- Around line 817-826: The loop over config.Tests is currently only adding tests
with ContainerTestConfiguration or MultiStageTestConfigurationLiteral to
buildSteps, which drops unresolved MultiStageTestConfiguration entries; update
the condition in the loop to also include test.MultiStageTestConfiguration (i.e.
treat tests with MultiStageTestConfiguration as valid candidates to append to
buildSteps) or alternatively ensure loadConfig/FromConfigStatic always resolves
MultiStageTestConfiguration into MultiStageTestConfigurationLiteral before this
loop; reference symbols: config.Tests, test.MultiStageTestConfiguration,
test.MultiStageTestConfigurationLiteral, test.ContainerTestConfiguration,
loadConfig, FromConfigStatic, buildSteps, api.StepConfiguration.
openshift-merge-robot
added
the
needs-rebase
Member
|
/approve |
Contributor
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: droslean, petr-muller 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 |
Member
|
man that feels good /meow |
Contributor
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. |
droslean
force-pushed
the
templates-code-cleanup
branch
from
fb8181c to
0ec3d6f
Compare
openshift-merge-robot
removed
the
needs-rebase
droslean
force-pushed
the
templates-code-cleanup
branch
from
0ec3d6f to
8101fb0
Compare
Signed-off-by: Nikolaos Moraitis <nmoraiti@redhat.com>
droslean
force-pushed
the
templates-code-cleanup
branch
from
8101fb0 to
102d82e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
pkg/defaults/defaults_test.go (2)
1624-1629: Rename this test case to reflect the new config type.The case name still says “openshift-installer test” while the fixture now uses
MultiStageTestConfigurationLiteral, which is confusing during failure triage.Suggested rename
- name: "openshift-installer test", + name: "multi-stage literal test",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/defaults/defaults_test.go` around lines 1624 - 1629, Test case name "openshift-installer test" is misleading given the fixture uses MultiStageTestConfigurationLiteral; update the test case's name string in defaults_test.go to something descriptive like "multi-stage test (MultiStageTestConfigurationLiteral)" so failures clearly indicate the config type and reference the TestStepConfiguration/MultiStageTestConfigurationLiteral fixture used.
1862-1882: Consider adding oneEnableSecretsStoreCSIDriver=truecoverage case.This block wires the new field, but only exercises the false path in updated fixtures.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/defaults/defaults_test.go` around lines 1862 - 1882, The test fixtures only exercise EnableSecretsStoreCSIDriver=false; add a new table-driven test case that sets EnableSecretsStoreCSIDriver=true in the constructed input (the struct with fields like CIConfig, GraphConf, JobSpec, EnableSecretsStoreCSIDriver, etc.), include any necessary paramFiles/other fields copied from an existing similar case, and add assertions that verify the code path for the true branch is exercised (e.g., expected behavior/state when EnableSecretsStoreCSIDriver is true). Ensure the new case is named/annotated so it runs with the rest of defaults_test.go cases.
🤖 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/defaults/defaults.go`:
- Line 810: The condition that decides which tests to append currently only
checks test.ContainerTestConfiguration and
test.MultiStageTestConfigurationLiteral, which omits unresolved multi-stage
tests; update the test selection condition (the if that guards adding to
buildSteps) to also check for test.MultiStageTestConfiguration != nil so
unresolved MultiStageTestConfiguration instances are included; this change
should be made where tests are filtered before calling FromConfigStatic /
building the graph so buildSteps and validation keep unresolved tests (refer to
the local variable test and the buildSteps filtering if-statement).
---
Nitpick comments:
In `@pkg/defaults/defaults_test.go`:
- Around line 1624-1629: Test case name "openshift-installer test" is misleading
given the fixture uses MultiStageTestConfigurationLiteral; update the test
case's name string in defaults_test.go to something descriptive like
"multi-stage test (MultiStageTestConfigurationLiteral)" so failures clearly
indicate the config type and reference the
TestStepConfiguration/MultiStageTestConfigurationLiteral fixture used.
- Around line 1862-1882: The test fixtures only exercise
EnableSecretsStoreCSIDriver=false; add a new table-driven test case that sets
EnableSecretsStoreCSIDriver=true in the constructed input (the struct with
fields like CIConfig, GraphConf, JobSpec, EnableSecretsStoreCSIDriver, etc.),
include any necessary paramFiles/other fields copied from an existing similar
case, and add assertions that verify the code path for the true branch is
exercised (e.g., expected behavior/state when EnableSecretsStoreCSIDriver is
true). Ensure the new case is named/annotated so it runs with the rest of
defaults_test.go cases.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 732528df-e5e8-459d-8ef5-77c2aae50d59
⛔ Files ignored due to path filters (2)
pkg/api/zz_generated.deepcopy.gois excluded by!**/zz_generated*pkg/webreg/zz_generated.ci_operator_reference.gois excluded by!**/zz_generated*
📒 Files selected for processing (39)
cmd/ci-operator/main.gocmd/ci-operator/main_test.gocmd/determinize-ci-operator/main.gocmd/determinize-ci-operator/main_test.gopkg/api/types.gopkg/defaults/config.gopkg/defaults/defaults.gopkg/defaults/defaults_test.gopkg/prowgen/jobbase.gopkg/prowgen/jobbase_test.gopkg/prowgen/podspec.gopkg/prowgen/podspec_test.gopkg/prowgen/prowgen_test.gopkg/prowgen/testdata/zz_fixture_TestReleaseRpms_envvar_generated_for_non_origin_repo.yamlpkg/prowgen/testdata/zz_fixture_TestTemplate_different_template_with_command.yamlpkg/prowgen/testdata/zz_fixture_TestTemplate_template_with_a_custom_test_image.yamlpkg/prowgen/testdata/zz_fixture_TestTemplate_template_with_command.yamlpkg/prowgen/testdata/zz_fixture_TestTemplate_template_with_different_command.yamlpkg/registry/server/testdata/zz_fixture_TestConfig_getting_config_works.yamlpkg/registry/server/testdata/zz_fixture_TestConfigserved_config.yamlpkg/steps/artifacts.gopkg/steps/clusterinstall/OWNERSpkg/steps/clusterinstall/clusterinstall.gopkg/steps/clusterinstall/template.gopkg/steps/template.gopkg/steps/template_test.gopkg/steps/testdata/zz_fixture_TestInjectLabelsToTemplate_jobspec_with_refs__label_injection_expected.yamlpkg/steps/testdata/zz_fixture_TestInjectLabelsToTemplate_nil_refs_in_jobspec__no_injection_expected.yamlpkg/steps/testdata/zz_fixture_TestInjectResourcesToPod_both_default_and_template_s_resource_requests_are_defined__pod_has_container_named_test__and_is_changed.yamlpkg/steps/testdata/zz_fixture_TestOperateOnTemplatePods_only_default_resources_defined_in_the_config_no_existing_resources_in_the_container__changes_are_expected.yamlpkg/steps/testdata/zz_fixture_TestOperateOnTemplatePods_resources_defined_in_the_config_but_the_test_container_has_existing_resources__no_changes_expected.yamlpkg/steps/testdata/zz_fixture_TestOperateOnTemplatePods_resources_defined_in_the_config_no_existing_resources_in_the_container__changes_are_expected.yamlpkg/steps/testdata/zz_fixture_TestOperateOnTemplatePods_template_with_no_pod__no_changes_expected.yamlpkg/steps/testdata/zz_fixture_TestOperateOnTemplatePods_template_with_pod_but_with_no_artifacts_Volume_VolumeMount__no_changes_expected.yamlpkg/steps/testdata/zz_fixture_TestOperateOnTemplatePods_template_with_pod_with_artifacts_Volume_VolumeMount__changes_expected.yamlpkg/steps/testdata/zz_fixture_TestOperateOnTemplatePods_template_with_pod_with_artifacts_Volume_VolumeMount_and_artifacts_dir_defined__changes_expected.yamlpkg/steps/testdata/zz_fixture_TestOperateOnTemplatePods_template_with_pod_with_artifacts_Volume_VolumeMount_but_not_artifacts_dir_defined__no_changes_expected.yamlpkg/validation/test.gopkg/validation/test_test.go
💤 Files with no reviewable changes (31)
- pkg/prowgen/testdata/zz_fixture_TestTemplate_template_with_command.yaml
- pkg/steps/testdata/zz_fixture_TestOperateOnTemplatePods_template_with_pod_but_with_no_artifacts_Volume_VolumeMount__no_changes_expected.yaml
- pkg/prowgen/testdata/zz_fixture_TestTemplate_template_with_different_command.yaml
- pkg/steps/testdata/zz_fixture_TestInjectResourcesToPod_both_default_and_template_s_resource_requests_are_defined__pod_has_container_named_test__and_is_changed.yaml
- cmd/ci-operator/main_test.go
- pkg/prowgen/jobbase.go
- pkg/steps/clusterinstall/OWNERS
- pkg/registry/server/testdata/zz_fixture_TestConfig_getting_config_works.yaml
- pkg/prowgen/podspec.go
- pkg/steps/testdata/zz_fixture_TestOperateOnTemplatePods_template_with_no_pod__no_changes_expected.yaml
- pkg/steps/testdata/zz_fixture_TestOperateOnTemplatePods_template_with_pod_with_artifacts_Volume_VolumeMount_and_artifacts_dir_defined__changes_expected.yaml
- pkg/steps/testdata/zz_fixture_TestOperateOnTemplatePods_template_with_pod_with_artifacts_Volume_VolumeMount__changes_expected.yaml
- pkg/prowgen/jobbase_test.go
- pkg/registry/server/testdata/zz_fixture_TestConfigserved_config.yaml
- pkg/steps/testdata/zz_fixture_TestInjectLabelsToTemplate_jobspec_with_refs__label_injection_expected.yaml
- pkg/prowgen/testdata/zz_fixture_TestReleaseRpms_envvar_generated_for_non_origin_repo.yaml
- pkg/steps/testdata/zz_fixture_TestOperateOnTemplatePods_only_default_resources_defined_in_the_config_no_existing_resources_in_the_container__changes_are_expected.yaml
- pkg/steps/template_test.go
- pkg/prowgen/prowgen_test.go
- pkg/validation/test.go
- pkg/steps/testdata/zz_fixture_TestOperateOnTemplatePods_resources_defined_in_the_config_no_existing_resources_in_the_container__changes_are_expected.yaml
- cmd/determinize-ci-operator/main_test.go
- pkg/prowgen/testdata/zz_fixture_TestTemplate_template_with_a_custom_test_image.yaml
- pkg/steps/testdata/zz_fixture_TestOperateOnTemplatePods_template_with_pod_with_artifacts_Volume_VolumeMount_but_not_artifacts_dir_defined__no_changes_expected.yaml
- pkg/steps/testdata/zz_fixture_TestOperateOnTemplatePods_resources_defined_in_the_config_but_the_test_container_has_existing_resources__no_changes_expected.yaml
- pkg/steps/clusterinstall/clusterinstall.go
- pkg/steps/clusterinstall/template.go
- pkg/steps/artifacts.go
- pkg/steps/template.go
- pkg/prowgen/testdata/zz_fixture_TestTemplate_different_template_with_command.yaml
- pkg/steps/testdata/zz_fixture_TestInjectLabelsToTemplate_nil_refs_in_jobspec__no_injection_expected.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/prowgen/podspec_test.go
| for i := range config.Tests { | ||
| test := &config.Tests[i] | ||
| if test.ContainerTestConfiguration != nil || test.MultiStageTestConfigurationLiteral != nil || (test.OpenshiftInstallerClusterTestConfiguration != nil && test.OpenshiftInstallerClusterTestConfiguration.Upgrade) { | ||
| if test.ContainerTestConfiguration != nil || test.MultiStageTestConfigurationLiteral != nil { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Call sites of FromConfigStatic =="
rg -nP --type=go -C3 '\bFromConfigStatic\s*\('
echo
echo "== Where MultiStageTestConfigurationLiteral is populated =="
rg -nP --type=go -C3 'MultiStageTestConfigurationLiteral'
echo
echo "== Places constructing tests with MultiStageTestConfiguration (steps) =="
rg -nP --type=go -C3 'MultiStageTestConfiguration\s*:'Repository: openshift/ci-tools
Length of output: 50375
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Context around line 810 in defaults.go ==="
sed -n '800,820p' pkg/defaults/defaults.go
echo
echo "=== Full FromConfigStatic function signature and start ==="
sed -n '527,550p' pkg/defaults/defaults.go
echo
echo "=== Looking for where tests are processed in FromConfigStatic ==="
grep -n "MultiStageTestConfiguration" pkg/defaults/defaults.go | head -20Repository: openshift/ci-tools
Length of output: 2393
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Checking when resolver is invoked (before or after FromConfigStatic) ==="
echo "From ci-operator/main.go - checking resolver usage pattern:"
grep -n -B5 -A5 "FromConfigStatic" cmd/ci-operator/main.go
echo
echo "From ci-operator-checkconfig/main.go:"
grep -n -B5 -A5 "FromConfigStatic" cmd/ci-operator-checkconfig/main.goRepository: openshift/ci-tools
Length of output: 1202
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Finding resolver.Resolve calls ==="
rg -n "\.Resolve\(" --type=go -B2 -A2
echo
echo "=== Finding ResolveTests or test resolution patterns ==="
rg -n "ResolveTests|Resolve.*Test" --type=go | head -20
echo
echo "=== Check if there's a resolver invocation in main flow before FromConfigStatic ==="
sed -n '550,600p' cmd/ci-operator/main.goRepository: openshift/ci-tools
Length of output: 25282
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Search for where config.Tests are modified to have literal versions ==="
rg -n "MultiStageTestConfigurationLiteral\s*=" --type=go | grep -v "test\|spec" | head -15Repository: openshift/ci-tools
Length of output: 412
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== IsValidResolvedConfiguration definition and usage ==="
rg -n "IsValidResolvedConfiguration" --type=go -B2 -A10 | head -50
echo
echo "=== Check if registry resolver is invoked in checkconfig path ==="
grep -n "resolver\|Resolve" cmd/ci-operator-checkconfig/main.goRepository: openshift/ci-tools
Length of output: 3964
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Check the helper function that detects unresolved tests ==="
sed -n '110,120p' pkg/api/helper/imageextraction.goRepository: openshift/ci-tools
Length of output: 528
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Full checkconfig validation and FromConfigStatic flow ==="
sed -n '140,160p' cmd/ci-operator-checkconfig/main.goRepository: openshift/ci-tools
Length of output: 909
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Check ResolveConfig function ==="
rg -n "func ResolveConfig" --type=go -A15Repository: openshift/ci-tools
Length of output: 3846
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Check if ci-operator's IsValidResolvedConfiguration actually resolves tests ==="
sed -n '43,90p' pkg/validation/config.goRepository: openshift/ci-tools
Length of output: 1742
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Check what loadConfig or resolverClient.ConfigWithTest returns ==="
rg -n "ConfigWithTest\|loadConfig" cmd/ci-operator/main.go -B2 -A8 | head -40Repository: openshift/ci-tools
Length of output: 132
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Look for where o.configSpec is populated in ci-operator/main.go ==="
grep -n "o.configSpec\s*=" cmd/ci-operator/main.go | head -10
echo
echo "=== Check the resolver client interface ==="
rg -n "type.*Client.*interface" pkg/load/agents/ -A20Repository: openshift/ci-tools
Length of output: 205
Add check for unresolved steps config in test filtering condition
Line 810 currently only appends tests with ContainerTestConfiguration or MultiStageTestConfigurationLiteral set. This omits tests with unresolved MultiStageTestConfiguration (steps). In the ci-operator-checkconfig path (lines 142-151), the resolved config is validated but then discarded; the original unresolved configuration is passed to FromConfigStatic, allowing unresolved tests to silently bypass this condition and be excluded from buildSteps. This breaks test validation and graph configuration.
Add || test.MultiStageTestConfiguration != nil to the condition at line 810 to ensure unresolved tests are not silently omitted.
Location
if test.ContainerTestConfiguration != nil || test.MultiStageTestConfigurationLiteral != nil {
Should become:
if test.ContainerTestConfiguration != nil || test.MultiStageTestConfigurationLiteral != nil || test.MultiStageTestConfiguration != nil {
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/defaults/defaults.go` at line 810, The condition that decides which tests
to append currently only checks test.ContainerTestConfiguration and
test.MultiStageTestConfigurationLiteral, which omits unresolved multi-stage
tests; update the test selection condition (the if that guards adding to
buildSteps) to also check for test.MultiStageTestConfiguration != nil so
unresolved MultiStageTestConfiguration instances are included; this change
should be made where tests are filtered before calling FromConfigStatic /
building the graph so buildSteps and validation keep unresolved tests (refer to
the local variable test and the buildSteps filtering if-statement).
Contributor
|
@droslean: 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. |
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
4 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.
Cleanup all template-related code from all tools that were using templates.
/cc @openshift/test-platform
Summary by CodeRabbit
Chores