OCPCLOUD-3647: Move envsubst transformations to use ManifestTransformer - #636
OCPCLOUD-3647: Move envsubst transformations to use ManifestTransformer#636mdbooth wants to merge 10 commits into
Conversation
We were previously pre-converting all API revisions into installer revisions. The purpose of this was to assemble all related objects before reconciliation. However, we weren't actually writing them so this served no purpose. We take the opportunity to remove the unnecessary complexity.
This allows us to add code to component generation which can fail.
This is now handled in toBoxcutterRevision
ManifestTransformer is a common interface for transformations on resource manifests. It is intended to cover all existing ad-hoc transformations, although they are not implemented in this change.
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@mdbooth: This pull request references OCPCLOUD-3647 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. 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 openshift-eng/jira-lifecycle-plugin repository. |
WalkthroughThe revision model now stores parsed manifests and substitutions. A shared manifest transformer pipeline validates revisions, transforms install objects, applies reconcile options, supports adoption behavior, and is wired through revision and installer controllers. ChangesManifest transformation pipeline
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant RevisionController
participant ValidateTransformers
participant revisionReconciler
participant toBoxcutterRevision
participant Boxcutter
RevisionController->>ValidateTransformers: validate parsed revision objects
ValidateTransformers-->>RevisionController: validation result
revisionReconciler->>toBoxcutterRevision: convert API revision with transformers
toBoxcutterRevision->>Boxcutter: construct CRD and object phases
Boxcutter-->>revisionReconciler: reconcile transformed phases
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
Bot
added
the
do-not-merge/hold
|
[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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (4)
pkg/controllers/installer/boxcutter.go (1)
144-159: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTransformer errors leave subsequent transformers operating on the pre-transform object. On error the loop continues without advancing
obj, so later transformers in the chain see stale input. Harmless today (any error discards the object), but worth a short comment noting the aggregation intent so a future refactor doesn't turn this into silent divergence.🤖 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/boxcutter.go` around lines 144 - 159, Add a concise comment in the transformer loop around the TransformObject error branch explaining that errors are aggregated while subsequent transformers intentionally continue with the current object, preserving the existing behavior and documenting the intent.cmd/capi-installer/main.go (1)
134-138: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNo canonical constructor for the default transformer pipeline. The pipeline is re-declared at each wiring site, and the revision-controller envtest already diverges from production (adopt-existing only), so validation coverage in tests can drift from what runs in the operator. Export something like
manifesttransformer.DefaultTransformers()and use it everywhere.
cmd/capi-installer/main.go#L134-L138: replace the inline slice with the shared constructor.pkg/controllers/installer/suite_test.go#L110-L114: use the same constructor so the suite exercises the production pipeline.pkg/controllers/revision/helpers_test.go#L77-L82: use the shared constructor instead of adopt-existing only.🤖 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 `@cmd/capi-installer/main.go` around lines 134 - 138, Introduce and export a canonical manifesttransformer.DefaultTransformers() constructor containing the production default pipeline, then replace each inline transformer slice with it: cmd/capi-installer/main.go lines 134-138, pkg/controllers/installer/suite_test.go lines 110-114, and pkg/controllers/revision/helpers_test.go lines 77-82. Ensure all three sites use the shared constructor, including the full envsubst, managed-by, and adopt-existing sequence.pkg/controllers/revision/revision_controller_test.go (1)
524-530: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueIndex-based substitution assertions are order-coupled. Consider
ConsistOf/HaveFieldso a future ordering change insubstitutionsFromMapdoesn't break these specs.As per coding guidelines, prefer Gomega built-ins such as
HaveField/ConsistOffor unordered slice matching.🤖 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/revision_controller_test.go` around lines 524 - 530, Update the ManifestSubstitutions assertions in this test to validate the expected key/value entries without relying on slice indexes or ordering. Use Gomega’s unordered matching built-ins, such as ConsistOf with HaveField, while preserving the existing three expected substitutions and values.Source: Coding guidelines
pkg/manifesttransformer/managed_by_transformer_test.go (1)
27-101: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffAlign these manifesttransformer tests with the Ginkgo style
pkg/manifesttransformer/managed_by_transformer_test.gopkg/manifesttransformer/adopt_existing_transformer_test.gopkg/manifesttransformer/validate_test.goThey still use
testing.T/t.Run; convert them toDescribe/Context/ItwithBy()steps, and switchvalidate_test.goto matcher-based error assertions instead of checkingerr.Error()directly.🤖 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/manifesttransformer/managed_by_transformer_test.go` around lines 27 - 101, Convert the tests in pkg/manifesttransformer/managed_by_transformer_test.go (lines 27-101), pkg/manifesttransformer/adopt_existing_transformer_test.go (lines 41-120), and pkg/manifesttransformer/validate_test.go (lines 31-132) from testing.T and t.Run to Ginkgo Describe/Context/It structure, adding By() steps for the test actions. In validate_test.go, replace direct err.Error() comparisons with the appropriate matcher-based error assertions while preserving the existing validation behavior.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.
Inline comments:
In `@pkg/controllers/installer/boxcutter_test.go`:
- Around line 146-170: Update the DescribeTable entry named “adopt-existing
annotation is stable across calls” so it exercises adopt-existing transformation
by passing an AdoptExistingTransformer instead of nil for that case, or rename
the entry to describe only phase stability. Ensure the test title matches the
behavior actually asserted while preserving the existing phase-count checks.
In `@pkg/controllers/installer/installer_controller.go`:
- Around line 140-142: Update the comment immediately above the managedByReq
initialization to state that the managed-by label is applied at install time by
ManagedByTransformer, replacing the outdated reference to the revision
generator; leave the labels.NewRequirement logic unchanged.
In `@pkg/controllers/installer/revision_reconciler.go`:
- Around line 171-183: Update reconcileRevision’s
revisiongenerator.NewInstallerRevisionFromAPI error path to return the formatted
error’s message in the second result, matching the
manifesttransformer.ValidateTransformers failure path. Preserve the existing
terminal error wrapping and ensure malformed API revisions no longer return an
empty status message.
In `@pkg/controllers/revision/revision_controller_test.go`:
- Around line 581-591: Update the “sets NonRetryableError when a transformer
Validate fails” test to reuse the outer RevisionController `r` configured by the
error-handling Describe/BeforeEach instead of declaring a shadowing local
controller. Capture the Reconcile result and error, then assert the expected
non-retryable error outcome with a meaningful failure message rather than
discarding both return values.
In `@pkg/manifesttransformer/envsubst_transformer_test.go`:
- Around line 42-47: Update fakeRevisionWithSubs.ManifestSubstitutions to
preserve nil substitutions: return nil when f.subs is nil, while continuing to
return a copied non-nil map for initialized substitutions. This will allow the
no-substitutions test to exercise EnvsubstTransformer.WithRevision’s merged ==
nil branch without changing behavior for empty-but-initialized maps.
---
Nitpick comments:
In `@cmd/capi-installer/main.go`:
- Around line 134-138: Introduce and export a canonical
manifesttransformer.DefaultTransformers() constructor containing the production
default pipeline, then replace each inline transformer slice with it:
cmd/capi-installer/main.go lines 134-138,
pkg/controllers/installer/suite_test.go lines 110-114, and
pkg/controllers/revision/helpers_test.go lines 77-82. Ensure all three sites use
the shared constructor, including the full envsubst, managed-by, and
adopt-existing sequence.
In `@pkg/controllers/installer/boxcutter.go`:
- Around line 144-159: Add a concise comment in the transformer loop around the
TransformObject error branch explaining that errors are aggregated while
subsequent transformers intentionally continue with the current object,
preserving the existing behavior and documenting the intent.
In `@pkg/controllers/revision/revision_controller_test.go`:
- Around line 524-530: Update the ManifestSubstitutions assertions in this test
to validate the expected key/value entries without relying on slice indexes or
ordering. Use Gomega’s unordered matching built-ins, such as ConsistOf with
HaveField, while preserving the existing three expected substitutions and
values.
In `@pkg/manifesttransformer/managed_by_transformer_test.go`:
- Around line 27-101: Convert the tests in
pkg/manifesttransformer/managed_by_transformer_test.go (lines 27-101),
pkg/manifesttransformer/adopt_existing_transformer_test.go (lines 41-120), and
pkg/manifesttransformer/validate_test.go (lines 31-132) from testing.T and t.Run
to Ginkgo Describe/Context/It structure, adding By() steps for the test actions.
In validate_test.go, replace direct err.Error() comparisons with the appropriate
matcher-based error assertions while preserving the existing validation
behavior.
🪄 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: 49dd196c-281f-4dd4-966f-e7d14e4db2c0
📒 Files selected for processing (27)
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/revision_reconciler_test.gopkg/controllers/installer/suite_test.gopkg/controllers/revision/helpers_test.gopkg/controllers/revision/revision_controller.gopkg/controllers/revision/revision_controller_test.gopkg/manifesttransformer/adopt_existing_transformer.gopkg/manifesttransformer/adopt_existing_transformer_test.gopkg/manifesttransformer/envsubst_transformer.gopkg/manifesttransformer/envsubst_transformer_test.gopkg/manifesttransformer/managed_by_transformer.gopkg/manifesttransformer/managed_by_transformer_test.gopkg/manifesttransformer/manifest_transformer.gopkg/manifesttransformer/validate.gopkg/manifesttransformer/validate_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.go
💤 Files with no reviewable changes (3)
- pkg/revisiongenerator/transform.go
- pkg/revisiongenerator/validate_test.go
- pkg/revisiongenerator/transform_test.go
| DescribeTable("should return stable phases on every call", | ||
| func(providerName string, wantPhaseCount int) { | ||
| rev := installerRevisionFromProfiles(providerName) | ||
|
|
||
| bcRev, err := toBoxcutterRevision(context.Background(), rev, nil, noopCollector) | ||
| Expect(err).NotTo(HaveOccurred()) | ||
|
|
||
| first := bcRev.GetPhases() | ||
| second := bcRev.GetPhases() | ||
|
|
||
| Expect(second).To(HaveLen(wantPhaseCount), | ||
| "expected %d phase(s) for provider %q", wantPhaseCount, providerName) | ||
|
|
||
| for i := range first { | ||
| Expect(second[i].GetName()).To(Equal(first[i].GetName()), | ||
| "phase[%d] name must be stable across GetPhases calls", i) | ||
| Expect(second[i].GetObjects()).To(HaveLen(len(first[i].GetObjects())), | ||
| "phase[%d] object count must be stable across GetPhases calls", i) | ||
| } | ||
| }, | ||
| Entry("objects only — one objects phase", providerCore, 1), | ||
| Entry("CRDs only — one CRD phase", providerCRD, 1), | ||
| Entry("CRDs and objects — two phases", providerMixed, 2), | ||
| Entry("adopt-existing annotation is stable across calls", providerAdoptExisting, 1), | ||
| ) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Entry title promises adopt-existing behaviour that isn't exercised. The table passes nil transformers, so the providerAdoptExisting entry only checks phase stability — the annotation is never stripped and no collision-protection option is produced. Either rename the entry to reflect what it asserts or pass &manifesttransformer.AdoptExistingTransformer{} for that case.
As per coding guidelines, flag "overly-specific test titles likely to change" and ensure each test asserts the behaviour it names.
🤖 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/boxcutter_test.go` around lines 146 - 170, Update
the DescribeTable entry named “adopt-existing annotation is stable across calls”
so it exercises adopt-existing transformation by passing an
AdoptExistingTransformer instead of nil for that case, or rename the entry to
describe only phase stability. Ensure the test title matches the behavior
actually asserted while preserving the existing phase-count checks.
Source: Coding guidelines
| func (r *revisionReconciler) reconcileRevision(ctx context.Context, apiRevision operatorv1alpha1.ClusterAPIInstallerRevision) (bool, string, error) { | ||
| revision, err := revisiongenerator.NewInstallerRevisionFromAPI(apiRevision, r.providerProfiles) | ||
| if err != nil { | ||
| return false, "", fmt.Errorf("error creating installer revision from API revision %s: %w", apiRevision.Name, reconcile.TerminalError(err)) | ||
| } | ||
|
|
||
| // Defence in depth: the revision controller validates transformers before | ||
| // writing a new revision, but a revision read back from the API could in | ||
| // principle be malformed (e.g. hand-edited), so validate again here. | ||
| if err := manifesttransformer.ValidateTransformers(r.transformers, revision); err != nil { | ||
| err = fmt.Errorf("validating revision %s: %w", revision.RevisionName(), reconcile.TerminalError(err)) | ||
| return false, err.Error(), err | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Conversion failure returns an empty status message. Every other failure path returns err.Error() as the message, so a malformed API revision surfaces as an error with a blank user-visible message (and an empty entry in the merged message list). Suggest mirroring the validation path.
🩹 Proposed fix
revision, err := revisiongenerator.NewInstallerRevisionFromAPI(apiRevision, r.providerProfiles)
if err != nil {
- return false, "", fmt.Errorf("error creating installer revision from API revision %s: %w", apiRevision.Name, reconcile.TerminalError(err))
+ err = fmt.Errorf("error creating installer revision from API revision %s: %w", apiRevision.Name, reconcile.TerminalError(err))
+ return false, err.Error(), err
}📝 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.
| func (r *revisionReconciler) reconcileRevision(ctx context.Context, apiRevision operatorv1alpha1.ClusterAPIInstallerRevision) (bool, string, error) { | |
| revision, err := revisiongenerator.NewInstallerRevisionFromAPI(apiRevision, r.providerProfiles) | |
| if err != nil { | |
| return false, "", fmt.Errorf("error creating installer revision from API revision %s: %w", apiRevision.Name, reconcile.TerminalError(err)) | |
| } | |
| // Defence in depth: the revision controller validates transformers before | |
| // writing a new revision, but a revision read back from the API could in | |
| // principle be malformed (e.g. hand-edited), so validate again here. | |
| if err := manifesttransformer.ValidateTransformers(r.transformers, revision); err != nil { | |
| err = fmt.Errorf("validating revision %s: %w", revision.RevisionName(), reconcile.TerminalError(err)) | |
| return false, err.Error(), err | |
| } | |
| revision, err := revisiongenerator.NewInstallerRevisionFromAPI(apiRevision, r.providerProfiles) | |
| if err != nil { | |
| err = fmt.Errorf("error creating installer revision from API revision %s: %w", apiRevision.Name, reconcile.TerminalError(err)) | |
| return false, err.Error(), err | |
| } |
🤖 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 171 - 183,
Update reconcileRevision’s revisiongenerator.NewInstallerRevisionFromAPI error
path to return the formatted error’s message in the second result, matching the
manifesttransformer.ValidateTransformers failure path. Preserve the existing
terminal error wrapping and ensure malformed API revisions no longer return an
empty status message.
| It("sets NonRetryableError when a transformer Validate fails", func(ctx context.Context) { | ||
| stub := &stubTransformer{validateErr: errors.New("invalid manifest")} | ||
| r := &RevisionController{ | ||
| Client: cl, | ||
| ProviderProfiles: defaultProviderImgs, | ||
| ReleaseVersion: "4.18.0", | ||
| Transformers: []manifesttransformer.ManifestTransformer{stub}, | ||
| } | ||
|
|
||
| _, _ = r.Reconcile(ctx, reconcile.Request{NamespacedName: client.ObjectKey{Name: "cluster"}}) | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Shadowed r and discarded reconcile result weaken this test. The spec lives in the "error handling" Describe, whose BeforeEach builds a failing interceptor client and assigns the outer r; here a second r shadows it, so the surrounding fixture is misleading. Also assert the reconcile outcome instead of discarding both return values.
💚 Suggested changes
- stub := &stubTransformer{validateErr: errors.New("invalid manifest")}
- r := &RevisionController{
+ stub := &stubTransformer{validateErr: errors.New("invalid manifest")}
+ nonRetryableR := &RevisionController{
Client: cl,
ProviderProfiles: defaultProviderImgs,
ReleaseVersion: "4.18.0",
Transformers: []manifesttransformer.ManifestTransformer{stub},
}
- _, _ = r.Reconcile(ctx, reconcile.Request{NamespacedName: client.ObjectKey{Name: "cluster"}})
+ _, err := nonRetryableR.Reconcile(ctx, reconcile.Request{NamespacedName: client.ObjectKey{Name: "cluster"}})
+ Expect(err).NotTo(HaveOccurred(), "a non-retryable error should not be returned for requeue")As per coding guidelines, tests should be "Consistent with codebase" and assertions "should include meaningful failure messages".
📝 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.
| It("sets NonRetryableError when a transformer Validate fails", func(ctx context.Context) { | |
| stub := &stubTransformer{validateErr: errors.New("invalid manifest")} | |
| r := &RevisionController{ | |
| Client: cl, | |
| ProviderProfiles: defaultProviderImgs, | |
| ReleaseVersion: "4.18.0", | |
| Transformers: []manifesttransformer.ManifestTransformer{stub}, | |
| } | |
| _, _ = r.Reconcile(ctx, reconcile.Request{NamespacedName: client.ObjectKey{Name: "cluster"}}) | |
| stub := &stubTransformer{validateErr: errors.New("invalid manifest")} | |
| nonRetryableR := &RevisionController{ | |
| Client: cl, | |
| ProviderProfiles: defaultProviderImgs, | |
| ReleaseVersion: "4.18.0", | |
| Transformers: []manifesttransformer.ManifestTransformer{stub}, | |
| } | |
| _, err := nonRetryableR.Reconcile(ctx, reconcile.Request{NamespacedName: client.ObjectKey{Name: "cluster"}}) | |
| Expect(err).NotTo(HaveOccurred(), "a non-retryable error should not be returned for requeue") |
🤖 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/revision_controller_test.go` around lines 581 - 591,
Update the “sets NonRetryableError when a transformer Validate fails” test to
reuse the outer RevisionController `r` configured by the error-handling
Describe/BeforeEach instead of declaring a shadowing local controller. Capture
the Reconcile result and error, then assert the expected non-retryable error
outcome with a meaningful failure message rather than discarding both return
values.
Source: Coding guidelines
| func (f *fakeRevisionWithSubs) ManifestSubstitutions() map[string]string { | ||
| out := make(map[string]string, len(f.subs)) | ||
| maps.Copy(out, f.subs) | ||
|
|
||
| return out | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Fake doesn't mirror the real nil-substitutions contract, leaving WithRevision's nil-guard untested.
parsedRevision.ManifestSubstitutions() returns nil when there are no substitutions (via maps.Clone(nil)), and EnvsubstTransformer.WithRevision has an explicit if merged == nil branch to handle that. This fake always returns a non-nil map (even for f.subs == nil), so the "does not panic when revision has no substitutions" test (lines 196-207) never actually exercises that nil branch.
🧪 Proposed fix to mirror the real nil-vs-empty contract
func (f *fakeRevisionWithSubs) ManifestSubstitutions() map[string]string {
+ if f.subs == nil {
+ return nil
+ }
+
out := make(map[string]string, len(f.subs))
maps.Copy(out, f.subs)
return out
}📝 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.
| func (f *fakeRevisionWithSubs) ManifestSubstitutions() map[string]string { | |
| out := make(map[string]string, len(f.subs)) | |
| maps.Copy(out, f.subs) | |
| return out | |
| } | |
| func (f *fakeRevisionWithSubs) ManifestSubstitutions() map[string]string { | |
| if f.subs == nil { | |
| return nil | |
| } | |
| out := make(map[string]string, len(f.subs)) | |
| maps.Copy(out, f.subs) | |
| return out | |
| } |
🤖 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/manifesttransformer/envsubst_transformer_test.go` around lines 42 - 47,
Update fakeRevisionWithSubs.ManifestSubstitutions to preserve nil substitutions:
return nil when f.subs is nil, while continuing to return a copied non-nil map
for initialized substitutions. This will allow the no-substitutions test to
exercise EnvsubstTransformer.WithRevision’s merged == nil branch without
changing behavior for empty-but-initialized maps.
Adds EnvsubstTransformer and ManagedByTransformer, moving envsubst expansion and managed-by labelling out of the revision generator and into install-time ManifestTransformers. The revision generator now stores manifests with envsubst variables unexpanded, and records substitutions on the revision for use by EnvsubstTransformer.
We moved manifest rendering (envsubst expansion, managed-by labelling) out to install-time ManifestTransformers, so the old "Rendered" name is now confusing. Renamed RenderedRevision/RenderedComponent (and their constructors and unexported implementations) to ParsedRevision/ParsedComponent throughout, adapted to the manifesttransformer package.
fda9884 to
220bb05
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/testvalue_transformer_test.go`:
- Around line 70-80: Update the WithRevision and WithComponent method signatures
on testValueTransformer to use revisiongenerator.ParsedRevision and
revisiongenerator.ParsedComponent respectively, matching the renamed types while
preserving their existing no-op behavior and return value.
In `@pkg/manifesttransformer/adopt_existing_transformer.go`:
- Around line 99-109: Update the installer test package in
testvalue_transformer_test.go to use revisiongenerator.ParsedRevision and
revisiongenerator.ParsedComponent wherever the obsolete RenderedRevision and
RenderedComponent types are referenced, preserving the existing test behavior.
🪄 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: d4fdd8c9-29e7-4ba2-8e38-fb654453e569
📒 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_test.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/manifesttransformer/adopt_existing_transformer.gopkg/manifesttransformer/adopt_existing_transformer_test.gopkg/manifesttransformer/envsubst_transformer.gopkg/manifesttransformer/envsubst_transformer_test.gopkg/manifesttransformer/managed_by_transformer.gopkg/manifesttransformer/managed_by_transformer_test.gopkg/manifesttransformer/manifest_transformer.gopkg/manifesttransformer/validate.gopkg/manifesttransformer/validate_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.go
💤 Files with no reviewable changes (3)
- pkg/revisiongenerator/validate_test.go
- pkg/revisiongenerator/transform_test.go
- pkg/revisiongenerator/transform.go
🚧 Files skipped from review as they are similar to previous changes (16)
- pkg/controllers/installer/suite_test.go
- pkg/manifesttransformer/manifest_transformer.go
- cmd/capi-installer/main.go
- pkg/controllers/installer/helpers_test.go
- pkg/revisiongenerator/helpers_test.go
- pkg/manifesttransformer/validate.go
- pkg/controllers/revision/helpers_test.go
- pkg/controllers/revision/revision_controller_test.go
- pkg/controllers/installer/revision_reconciler_test.go
- pkg/revisiongenerator/validate.go
- pkg/controllers/revision/revision_controller.go
- pkg/controllers/installer/installer_controller.go
- pkg/manifesttransformer/validate_test.go
- pkg/controllers/installer/boxcutter.go
- pkg/controllers/installer/boxcutter_test.go
- pkg/revisiongenerator/revision_test.go
| // WithRevision implements manifesttransformer.ManifestTransformer. It is a | ||
| // no-op; testValueTransformer does not need revision context. | ||
| func (t testValueTransformer) WithRevision(_ context.Context, _ revisiongenerator.RenderedRevision) manifesttransformer.ManifestTransformer { | ||
| return t | ||
| } | ||
|
|
||
| // WithComponent implements manifesttransformer.ManifestTransformer. It is a | ||
| // no-op; testValueTransformer does not need component context. | ||
| func (t testValueTransformer) WithComponent(_ context.Context, _ revisiongenerator.RenderedComponent) manifesttransformer.ManifestTransformer { | ||
| return t | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Build break: stale Rendered* types no longer exist.
revisiongenerator.RenderedRevision/RenderedComponent were renamed to ParsedRevision/ParsedComponent in pkg/revisiongenerator/revision.go as part of this same PR. This file still references the old names, breaking compilation of the pkg/controllers/installer test package (confirmed by the golangci-lint typecheck error).
🐛 Proposed fix
-func (t testValueTransformer) WithRevision(_ context.Context, _ revisiongenerator.RenderedRevision) manifesttransformer.ManifestTransformer {
+func (t testValueTransformer) WithRevision(_ context.Context, _ revisiongenerator.ParsedRevision) manifesttransformer.ManifestTransformer {
return t
}
// WithComponent implements manifesttransformer.ManifestTransformer. It is a
// no-op; testValueTransformer does not need component context.
-func (t testValueTransformer) WithComponent(_ context.Context, _ revisiongenerator.RenderedComponent) manifesttransformer.ManifestTransformer {
+func (t testValueTransformer) WithComponent(_ context.Context, _ revisiongenerator.ParsedComponent) manifesttransformer.ManifestTransformer {
return t
}📝 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.
| // WithRevision implements manifesttransformer.ManifestTransformer. It is a | |
| // no-op; testValueTransformer does not need revision context. | |
| func (t testValueTransformer) WithRevision(_ context.Context, _ revisiongenerator.RenderedRevision) manifesttransformer.ManifestTransformer { | |
| return t | |
| } | |
| // WithComponent implements manifesttransformer.ManifestTransformer. It is a | |
| // no-op; testValueTransformer does not need component context. | |
| func (t testValueTransformer) WithComponent(_ context.Context, _ revisiongenerator.RenderedComponent) manifesttransformer.ManifestTransformer { | |
| return t | |
| } | |
| // WithRevision implements manifesttransformer.ManifestTransformer. It is a | |
| // no-op; testValueTransformer does not need revision context. | |
| func (t testValueTransformer) WithRevision(_ context.Context, _ revisiongenerator.ParsedRevision) manifesttransformer.ManifestTransformer { | |
| return t | |
| } | |
| // WithComponent implements manifesttransformer.ManifestTransformer. It is a | |
| // no-op; testValueTransformer does not need component context. | |
| func (t testValueTransformer) WithComponent(_ context.Context, _ revisiongenerator.ParsedComponent) manifesttransformer.ManifestTransformer { | |
| return t | |
| } |
🧰 Tools
🪛 golangci-lint (2.12.2)
[error] 72-72: : # github.com/openshift/cluster-capi-operator/pkg/controllers/installer [github.com/openshift/cluster-capi-operator/pkg/controllers/installer.test]
pkg/controllers/installer/testvalue_transformer_test.go:72:83: undefined: revisiongenerator.RenderedRevision
pkg/controllers/installer/testvalue_transformer_test.go:78:84: undefined: revisiongenerator.RenderedComponent
(typecheck)
🤖 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/testvalue_transformer_test.go` around lines 70 -
80, Update the WithRevision and WithComponent method signatures on
testValueTransformer to use revisiongenerator.ParsedRevision and
revisiongenerator.ParsedComponent respectively, matching the renamed types while
preserving their existing no-op behavior and return value.
Source: Linters/SAST tools
| // WithRevision implements ManifestTransformer. AdoptExistingTransformer does | ||
| // not need revision context. | ||
| func (a *AdoptExistingTransformer) WithRevision(_ context.Context, _ revisiongenerator.ParsedRevision) ManifestTransformer { | ||
| return a | ||
| } | ||
|
|
||
| // WithComponent implements ManifestTransformer. AdoptExistingTransformer does | ||
| // not need component context. | ||
| func (a *AdoptExistingTransformer) WithComponent(_ context.Context, _ revisiongenerator.ParsedComponent) ManifestTransformer { | ||
| return a | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm remaining references to the old type names across the repo.
rg -n 'RenderedRevision|RenderedComponent' --type=goRepository: openshift/cluster-capi-operator
Length of output: 169
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant files and remaining revisiongenerator type usage.
git ls-files 'pkg/manifesttransformer/adopt_existing_transformer.go' 'pkg/controllers/installer/testvalue_transformer_test.go' || true
printf '\n-- matches for ParsedRevision/ParsedComponent --\n'
rg -n 'ParsedRevision|ParsedComponent' --type=go pkg
printf '\n-- matches for RenderedRevision/RenderedComponent --\n'
rg -n 'RenderedRevision|RenderedComponent' --type=go pkg || trueRepository: openshift/cluster-capi-operator
Length of output: 12868
Update pkg/controllers/installer/testvalue_transformer_test.go to ParsedRevision / ParsedComponent. The old revisiongenerator.RenderedRevision and revisiongenerator.RenderedComponent names still break the installer test package build.
🤖 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/manifesttransformer/adopt_existing_transformer.go` around lines 99 - 109,
Update the installer test package in testvalue_transformer_test.go to use
revisiongenerator.ParsedRevision and revisiongenerator.ParsedComponent wherever
the obsolete RenderedRevision and RenderedComponent types are referenced,
preserving the existing test behavior.
Source: Linters/SAST tools
|
@mdbooth: 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
Adds EnvsubstTransformer and ManagedByTransformer, moving envsubst expansion and managed-by labelling out of the revision generator and into install-time ManifestTransformers. The revision generator now stores manifests with envsubst variables unexpanded, and records substitutions on the revision for use by EnvsubstTransformer.
Moves the hardcoded
EXP_BOOTSTRAP_FORMAT_IGNITIONsubstitution to manifest substitutions to ensure it upgrades cleanly when we remove it later.Renames RenderedRevision to ParsedRevision, as it no longer does rendering.
TODO:
/hold
Summary by CodeRabbit