runtime transformers - #629
Draft
mdbooth wants to merge 12 commits into
Draft
Conversation
Contributor
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
Contributor
|
Skipping CI for Draft Pull Request. |
Contributor
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
WalkthroughThe revision generator now produces parsed revisions with stored manifest substitutions. Runtime transformers handle envsubst expansion, managed-by labels, and adopt-existing behavior across revision validation and installer phase construction. ChangesParsed revision model
Runtime transformation and controller integration
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant OperatorConfig
participant RevisionController
participant RuntimeTransformers
participant InstallerController
participant Boxcutter
OperatorConfig->>RevisionController: provide transformer pipeline
RevisionController->>RuntimeTransformers: validate parsed revision objects
OperatorConfig->>InstallerController: provide transformer pipeline
InstallerController->>Boxcutter: construct transformed phases
Boxcutter->>RuntimeTransformers: apply object transformers
RuntimeTransformers->>Boxcutter: return object changes and reconcile options
Boxcutter->>InstallerController: reconcile transformed phases
Possibly related PRs
🚥 Pre-merge checks | ✅ 11 | ❌ 4❌ Failed checks (3 warnings, 1 inconclusive)
✅ Passed checks (11 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![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
🧹 Nitpick comments (1)
pkg/controllers/revision/helpers_test.go (1)
206-209: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueStale
RenderedRevisionreferences in doc comments. TheRenderedRevisiontype was replaced byParsedRevision; the rename wasn't propagated to these comments.
pkg/controllers/revision/helpers_test.go#L206-L209: update to "fakeRevision implements revisiongenerator.ParsedRevision for unit tests."pkg/revisiongenerator/helpers_test.go#L57-L58: update theforInstallcomment to say "from a ParsedRevision".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/controllers/revision/helpers_test.go` around lines 206 - 209, Update the stale documentation references to the renamed ParsedRevision type: in pkg/controllers/revision/helpers_test.go lines 206-209, change the fakeRevision comment to identify revisiongenerator.ParsedRevision; in pkg/revisiongenerator/helpers_test.go lines 57-58, update the forInstall comment to say it operates from a ParsedRevision. No code behavior changes are needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/controllers/installer/revision_reconciler.go`:
- Around line 366-370: Update the error return in the toBoxcutterRevision
failure path within the revision teardown reconciliation flow to wrap err with
reconcile.TerminalError(...). Keep complete=true and the existing empty message
unchanged, ensuring this deterministic construction failure is treated as
terminal rather than retryable.
In `@pkg/runtimetransformer/envsubst_transformer.go`:
- Around line 54-65: Update EnvsubstTransformer.WithRevision to ensure the map
returned by revision.ManifestSubstitutions() is initialized before maps.Copy
writes staticSubs into it. Preserve the existing precedence and returned
staticSubs/mergedSubs behavior for both nil and non-nil revision substitutions.
---
Nitpick comments:
In `@pkg/controllers/revision/helpers_test.go`:
- Around line 206-209: Update the stale documentation references to the renamed
ParsedRevision type: in pkg/controllers/revision/helpers_test.go lines 206-209,
change the fakeRevision comment to identify revisiongenerator.ParsedRevision; in
pkg/revisiongenerator/helpers_test.go lines 57-58, update the forInstall comment
to say it operates from a ParsedRevision. No code behavior changes are needed.
🪄 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: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: d74a5a62-0e25-41c0-b063-8ccbe760058c
📒 Files selected for processing (26)
cmd/capi-installer/main.gopkg/controllers/installer/boxcutter.gopkg/controllers/installer/boxcutter_test.gopkg/controllers/installer/helpers_test.gopkg/controllers/installer/installer_controller.gopkg/controllers/installer/revision_reconciler.gopkg/controllers/installer/suite_test.gopkg/controllers/revision/helpers_test.gopkg/controllers/revision/revision_controller.gopkg/controllers/revision/revision_controller_test.gopkg/revisiongenerator/helpers_test.gopkg/revisiongenerator/revision.gopkg/revisiongenerator/revision_test.gopkg/revisiongenerator/transform.gopkg/revisiongenerator/transform_test.gopkg/revisiongenerator/validate.gopkg/revisiongenerator/validate_test.gopkg/runtimetransformer/adopt_existing_transformer.gopkg/runtimetransformer/adopt_existing_transformer_test.gopkg/runtimetransformer/envsubst_transformer.gopkg/runtimetransformer/envsubst_transformer_test.gopkg/runtimetransformer/managed_by_transformer.gopkg/runtimetransformer/managed_by_transformer_test.gopkg/runtimetransformer/runtime_transformer.gopkg/runtimetransformer/validate.gopkg/runtimetransformer/validate_test.go
💤 Files with no reviewable changes (3)
- pkg/revisiongenerator/transform_test.go
- pkg/revisiongenerator/validate_test.go
- pkg/revisiongenerator/transform.go
Comment on lines
+366
to
+370
| bcRevision, err := toBoxcutterRevision(ctx, revision, r.transformers) | ||
| if err != nil { | ||
| // Cannot tear down a revision that cannot be constructed — treat as complete. | ||
| return true, "", err | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -nP 'teardownRevision\(' pkg/controllers/installer -C4Repository: openshift/cluster-capi-operator
Length of output: 1610
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '330,430p' pkg/controllers/installer/revision_reconciler.go
printf '\n---\n'
rg -n 'mergeWithTail|teardownRevisions|teardownRevision\(' pkg/controllers/installer -n -C3Repository: openshift/cluster-capi-operator
Length of output: 7024
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '440,520p' pkg/controllers/installer/revision_reconciler.go
printf '\n---\n'
sed -n '140,210p' pkg/controllers/installer/revision_reconciler.goRepository: openshift/cluster-capi-operator
Length of output: 5734
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n 'TerminalError|IsTerminal|errors\.As|errors\.Is|[]error|[]string|requeue|Retry' pkg/controllers/installer/revision_reconciler.go pkg/controllers/installer -C3Repository: openshift/cluster-capi-operator
Length of output: 364
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n 'TerminalError|IsTerminal|errors\.As|errors\.Is|requeue|Retry|terminal' pkg/controllers/installer -C3Repository: openshift/cluster-capi-operator
Length of output: 24083
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,140p' pkg/controllers/installer/installer_controller.go
printf '\n---\n'
sed -n '140,240p' pkg/controllers/installer/installer_controller.goRepository: openshift/cluster-capi-operator
Length of output: 8616
Wrap this teardown error as terminal. complete=true is fine here, but a bare error is treated as retryable by the controller, so this deterministic construction failure will keep requeueing. Use reconcile.TerminalError(...) instead of returning a plain error. (pkg/controllers/installer/revision_reconciler.go:366-370)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/controllers/installer/revision_reconciler.go` around lines 366 - 370,
Update the error return in the toBoxcutterRevision failure path within the
revision teardown reconciliation flow to wrap err with
reconcile.TerminalError(...). Keep complete=true and the existing empty message
unchanged, ensuring this deterministic construction failure is treated as
terminal rather than retryable.
This allows us to add code to component generation which can fail.
We moved the rendering out, so this name is now confusing.
This makes them a pure runtime substitution which doesn't require a rollout.
mdbooth
force-pushed
the
runtime-transformers
branch
from
July 21, 2026 14:31
fe23908 to
6f9d77d
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/runtimetransformer/envsubst_transformer_test.go (1)
59-217: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffUses plain
testing.Ttables instead of GinkgoDescribeTable/Entry.As per coding guidelines,
**/*_test.gofiles should "Use Ginkgo/Gomega framework and prefer built-in features: useDescribeTablewithEntryfor table-driven tests... andMatchErrorfor error checking." This file only imports Gomega and uses standardt.Run()tables instead.Also applies to: 229-311
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/runtimetransformer/envsubst_transformer_test.go` around lines 59 - 217, The table-driven tests in TestEnvsubstTransformerTransformObject and TestEnvsubstTransformerTransformObjectErrors should use Ginkgo/Gomega conventions instead of testing.T subtests. Convert both tables to DescribeTable with Entry, replace t.Run and per-case NewWithT usage with Gomega assertions in the table bodies, and use MatchError where exact error matching is appropriate while preserving the existing cases and assertions.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@pkg/runtimetransformer/envsubst_transformer_test.go`:
- Around line 59-217: The table-driven tests in
TestEnvsubstTransformerTransformObject and
TestEnvsubstTransformerTransformObjectErrors should use Ginkgo/Gomega
conventions instead of testing.T subtests. Convert both tables to DescribeTable
with Entry, replace t.Run and per-case NewWithT usage with Gomega assertions in
the table bodies, and use MatchError where exact error matching is appropriate
while preserving the existing cases and assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: bbbf8647-2323-434c-b9ad-b5f5414a3087
📒 Files selected for processing (28)
cmd/capi-installer/main.gopkg/controllers/installer/boxcutter.gopkg/controllers/installer/boxcutter_test.gopkg/controllers/installer/helpers_test.gopkg/controllers/installer/installer_controller.gopkg/controllers/installer/installer_controller_test.gopkg/controllers/installer/revision_reconciler.gopkg/controllers/installer/suite_test.gopkg/controllers/installer/testvalue_transformer_test.gopkg/controllers/revision/helpers_test.gopkg/controllers/revision/revision_controller.gopkg/controllers/revision/revision_controller_test.gopkg/revisiongenerator/helpers_test.gopkg/revisiongenerator/revision.gopkg/revisiongenerator/revision_test.gopkg/revisiongenerator/transform.gopkg/revisiongenerator/transform_test.gopkg/revisiongenerator/validate.gopkg/revisiongenerator/validate_test.gopkg/runtimetransformer/adopt_existing_transformer.gopkg/runtimetransformer/adopt_existing_transformer_test.gopkg/runtimetransformer/envsubst_transformer.gopkg/runtimetransformer/envsubst_transformer_test.gopkg/runtimetransformer/managed_by_transformer.gopkg/runtimetransformer/managed_by_transformer_test.gopkg/runtimetransformer/runtime_transformer.gopkg/runtimetransformer/validate.gopkg/runtimetransformer/validate_test.go
💤 Files with no reviewable changes (3)
- pkg/revisiongenerator/transform_test.go
- pkg/revisiongenerator/validate_test.go
- pkg/revisiongenerator/transform.go
🚧 Files skipped from review as they are similar to previous changes (19)
- pkg/runtimetransformer/validate.go
- pkg/revisiongenerator/helpers_test.go
- pkg/runtimetransformer/managed_by_transformer.go
- pkg/controllers/revision/helpers_test.go
- pkg/controllers/installer/installer_controller.go
- pkg/revisiongenerator/validate.go
- pkg/runtimetransformer/runtime_transformer.go
- pkg/runtimetransformer/managed_by_transformer_test.go
- pkg/controllers/installer/boxcutter.go
- pkg/controllers/installer/helpers_test.go
- cmd/capi-installer/main.go
- pkg/runtimetransformer/validate_test.go
- pkg/runtimetransformer/adopt_existing_transformer.go
- pkg/controllers/revision/revision_controller.go
- pkg/runtimetransformer/adopt_existing_transformer_test.go
- pkg/controllers/revision/revision_controller_test.go
- pkg/controllers/installer/revision_reconciler.go
- pkg/revisiongenerator/revision.go
- pkg/revisiongenerator/revision_test.go
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
1 participant
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.
Summary by CodeRabbit
New Features
Bug Fixes