OCPCLOUD-3647: Move TLS substitution variables out of the Revision - #637
OCPCLOUD-3647: Move TLS substitution variables out of the Revision#637mdbooth wants to merge 11 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. |
openshift-ci
Bot
added
the
do-not-merge/hold
WalkthroughThis PR replaces rendered revisions with parsed revisions and introduces install-time manifest transformers for envsubst expansion, managed-by labels, and adopt-existing handling. Controller reconciliation, Boxcutter phase construction, validation, TLS-aware wiring, and related tests are updated. ChangesManifest processing pipeline
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Installer
participant RevisionController
participant RevisionReconciler
participant ManifestTransformers
participant Boxcutter
Installer->>RevisionController: configure transformer chain
RevisionController->>RevisionReconciler: reconcile API revision
RevisionReconciler->>ManifestTransformers: validate and transform objects
ManifestTransformers-->>RevisionReconciler: transformed objects and reconcile options
RevisionReconciler->>Boxcutter: construct CRD and component phases
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 12 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (12 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/revisiongenerator/revision.go (1)
396-412: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winSkip empty manifests before appending.
Manifests()only skips separator-only empties; comment-only or whitespace-only documents still unmarshal to a nilObject, which then flows through validation/boxcutter as a GVK-less object (/).🤖 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/revisiongenerator/revision.go` around lines 396 - 412, Update the manifest loop around providerProfile.Manifests() to detect and skip documents that are empty after replacing SelfImageRef, including whitespace-only and comment-only YAML, before unmarshalling or appending. Preserve processing for manifests containing actual resources, and ensure nil or empty unstructured objects never reach component.objects.
🧹 Nitpick comments (7)
pkg/controllers/installer/installer_controller.go (1)
142-142: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueStale comment above this line.
The preceding comment still says the label is "applied by the revision generator"; it is now applied by
ManagedByTransformer. Label key value is unchanged, so cache behaviour is preserved.🤖 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/installer_controller.go` at line 142, Update the preceding comment near the managedByReq construction to state that the label is applied by ManagedByTransformer instead of the revision generator. Leave the label key and requirement logic unchanged.pkg/controllers/installer/revision_reconciler.go (1)
146-152: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winPer-revision parsing now happens on every reconcile pass.
Each revision in the recursion re-reads and re-parses provider manifests and recomputes the content ID, for reconcile and teardown alike. With several retained revisions and frequent watch-driven reconciles this is repeated I/O plus SHA-256 work on the reconcile thread. Consider memoizing installer revisions by (name, contentID) on the controller.
Also applies to: 172-172
🤖 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 146 - 152, Update revisionReconciler.reconcileRevisions and the related per-revision processing to memoize parsed provider manifests and computed content IDs on the controller, keyed by revision name and contentID. Reuse cached results across reconcile and teardown passes, while invalidating or replacing entries when either key changes.pkg/controllers/revision/revision_controller_test.go (1)
573-595: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert on the
Reconcilereturn instead of discarding it.Silently dropping both return values means the test still passes if
Reconcilestarts returning an unexpected (retryable) error while conditions happen to match.💚 Suggested change
- _, _ = r.Reconcile(ctx, reconcile.Request{NamespacedName: client.ObjectKey{Name: "cluster"}}) + _, err := r.Reconcile(ctx, reconcile.Request{NamespacedName: client.ObjectKey{Name: "cluster"}}) + Expect(err).NotTo(HaveOccurred(), "non-retryable errors are reported via conditions, not returned")🤖 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 573 - 595, The test case around the `Reconcile` call should assert its returned result instead of discarding both return values. Capture the `Reconcile` response and error, then verify the expected non-retryable outcome while retaining the existing ClusterOperator condition assertions.pkg/controllers/installer/boxcutter.go (1)
109-133: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winPhase is appended even when every object was skipped.
The
len(objects) == 0early return signals empty phases are undesirable, but if all objects are skipped by a transformer the function still appends a zero-object phase. Skip the append whentransformedObjectsis empty.♻️ Suggested change
+ if len(transformedObjects) == 0 { + return phases, errors.Join(allErrs...) + } + allOpts := slices.Concat(probeOpts, xfmrOpts) bcPhase := boxcutter.NewPhase(name, util.SliceMap(transformedObjects, toClientObject)).WithReconcileOptions(allOpts...)🤖 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 109 - 133, Avoid appending an empty phase when transformers skip every object: after the transformation loop in the phase-building flow, check whether transformedObjects is empty and return the existing phases with joined errors before constructing or appending bcPhase. Preserve accumulated transformer errors and the current behavior when at least one transformed object remains.pkg/revisiongenerator/revision.go (1)
354-364: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
opts = append(opts, ...)mutates the caller's variadic slice.If a caller ever passes a named slice with spare capacity, this write is visible to them. Prefer building a new slice.
♻️ Suggested change
- opts = append(opts, WithManifestSubstitutions(apiSubs)) - - parsed, err := newParsedRevision(matched, opts...) + allOpts := append(slices.Clone(opts), WithManifestSubstitutions(apiSubs)) + + parsed, err := newParsedRevision(matched, allOpts...)🤖 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/revisiongenerator/revision.go` around lines 354 - 364, Update the options assembly before calling newParsedRevision so adding WithManifestSubstitutions does not append into the caller-owned variadic slice when it has spare capacity. Create an independent options slice, preserving the existing options and appending the manifest substitutions to that copy before passing it to newParsedRevision.pkg/manifesttransformer/envsubst_transformer.go (1)
44-48: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winStatic substitutions are silently ignored until
WithRevisionis called.
mergedSubsis only populated inWithRevision, so a transformer used directly fromNewEnvsubstTransformerexpands everything to empty strings — including the TLS substitutions this PR moves to runtime. SeedingmergedSubswithstaticSubsmakes the base transformer behave correctly regardless of call order.♻️ Proposed change
func NewEnvsubstTransformer(staticSubs map[string]string) *EnvsubstTransformer { return &EnvsubstTransformer{ staticSubs: maps.Clone(staticSubs), + mergedSubs: maps.Clone(staticSubs), } }🤖 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.go` around lines 44 - 48, Initialize mergedSubs from staticSubs in NewEnvsubstTransformer, while retaining the cloned staticSubs map for later revision merging. Ensure direct use of the newly constructed EnvsubstTransformer applies static substitutions before WithRevision is called.pkg/manifesttransformer/validate_test.go (1)
134-181: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider a shared test fake for the new transformer/revision contracts. Each package re-implements near-identical
ManifestTransformerandParsedRevisiondoubles, so every future interface change requires three parallel edits.
pkg/manifesttransformer/validate_test.go#L134-L181: extractstubTransformer/fakeComponent/fakeRevisioninto an exported test helper package (e.g. undertestutils) and consume it here.pkg/controllers/revision/helpers_test.go#L192-L229: replace the localstubTransformer/fakeRevisioncopies with the shared helpers.pkg/controllers/installer/boxcutter_test.go#L68-L115: replace the localstubTransformerwith the shared one, keeping only the installer-specificfnTransformer.🤖 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/validate_test.go` around lines 134 - 181, Extract the shared stubTransformer, fakeComponent, and fakeRevision implementations into an exported test helper package, preserving their current contract behavior. Update pkg/manifesttransformer/validate_test.go lines 134-181 and pkg/controllers/revision/helpers_test.go lines 192-229 to use the shared helpers, and update pkg/controllers/installer/boxcutter_test.go lines 68-115 to use the shared stubTransformer while retaining only its installer-specific fnTransformer.
🤖 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 171-188: Update the revisiongenerator.NewInstallerRevisionFromAPI
error path in reconcileRevision to return the formatted error message instead of
an empty string, matching the other failure paths. Apply the same change to the
corresponding construction failure paths in teardownRevision so ClusterOperator
status preserves the error reason, including content-ID mismatches.
---
Outside diff comments:
In `@pkg/revisiongenerator/revision.go`:
- Around line 396-412: Update the manifest loop around
providerProfile.Manifests() to detect and skip documents that are empty after
replacing SelfImageRef, including whitespace-only and comment-only YAML, before
unmarshalling or appending. Preserve processing for manifests containing actual
resources, and ensure nil or empty unstructured objects never reach
component.objects.
---
Nitpick comments:
In `@pkg/controllers/installer/boxcutter.go`:
- Around line 109-133: Avoid appending an empty phase when transformers skip
every object: after the transformation loop in the phase-building flow, check
whether transformedObjects is empty and return the existing phases with joined
errors before constructing or appending bcPhase. Preserve accumulated
transformer errors and the current behavior when at least one transformed object
remains.
In `@pkg/controllers/installer/installer_controller.go`:
- Line 142: Update the preceding comment near the managedByReq construction to
state that the label is applied by ManagedByTransformer instead of the revision
generator. Leave the label key and requirement logic unchanged.
In `@pkg/controllers/installer/revision_reconciler.go`:
- Around line 146-152: Update revisionReconciler.reconcileRevisions and the
related per-revision processing to memoize parsed provider manifests and
computed content IDs on the controller, keyed by revision name and contentID.
Reuse cached results across reconcile and teardown passes, while invalidating or
replacing entries when either key changes.
In `@pkg/controllers/revision/revision_controller_test.go`:
- Around line 573-595: The test case around the `Reconcile` call should assert
its returned result instead of discarding both return values. Capture the
`Reconcile` response and error, then verify the expected non-retryable outcome
while retaining the existing ClusterOperator condition assertions.
In `@pkg/manifesttransformer/envsubst_transformer.go`:
- Around line 44-48: Initialize mergedSubs from staticSubs in
NewEnvsubstTransformer, while retaining the cloned staticSubs map for later
revision merging. Ensure direct use of the newly constructed EnvsubstTransformer
applies static substitutions before WithRevision is called.
In `@pkg/manifesttransformer/validate_test.go`:
- Around line 134-181: Extract the shared stubTransformer, fakeComponent, and
fakeRevision implementations into an exported test helper package, preserving
their current contract behavior. Update pkg/manifesttransformer/validate_test.go
lines 134-181 and pkg/controllers/revision/helpers_test.go lines 192-229 to use
the shared helpers, and update pkg/controllers/installer/boxcutter_test.go lines
68-115 to use the shared stubTransformer while retaining only its
installer-specific fnTransformer.
In `@pkg/revisiongenerator/revision.go`:
- Around line 354-364: Update the options assembly before calling
newParsedRevision so adding WithManifestSubstitutions does not append into the
caller-owned variadic slice when it has spare capacity. Create an independent
options slice, preserving the existing options and appending the manifest
substitutions to that copy before passing it to newParsedRevision.
🪄 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: a975c5b8-a8e4-445b-93bf-07db5482e776
📒 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_test.go
- pkg/revisiongenerator/validate_test.go
- pkg/revisiongenerator/transform.go
| 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 := conv.revision | ||
| bcRevision, err := toBoxcutterRevision(ctx, revision, r.transformers, r.collectObjects) | ||
| if err != nil { | ||
| return false, err.Error(), reconcile.TerminalError(fmt.Errorf("building boxcutter revision %s: %w", revision.RevisionName(), err)) | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Construction failure returns an empty status message.
Line 174 returns "" as the message while every other failure path returns err.Error(), so the ClusterOperator status loses the reason when a revision can't be constructed (e.g. content-ID mismatch). Same pattern at Line 349/357 in teardownRevision.
🐛 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 := conv.revision | |
| bcRevision, err := toBoxcutterRevision(ctx, revision, r.transformers, r.collectObjects) | |
| if err != nil { | |
| return false, err.Error(), reconcile.TerminalError(fmt.Errorf("building boxcutter revision %s: %w", revision.RevisionName(), err)) | |
| } | |
| func (r *revisionReconciler) reconcileRevision(ctx context.Context, apiRevision operatorv1alpha1.ClusterAPIInstallerRevision) (bool, string, error) { | |
| 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 | |
| } | |
| // 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 | |
| } | |
| bcRevision, err := toBoxcutterRevision(ctx, revision, r.transformers, r.collectObjects) | |
| if err != nil { | |
| return false, err.Error(), reconcile.TerminalError(fmt.Errorf("building boxcutter revision %s: %w", revision.RevisionName(), 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 - 188,
Update the revisiongenerator.NewInstallerRevisionFromAPI error path in
reconcileRevision to return the formatted error message instead of an empty
string, matching the other failure paths. Apply the same change to the
corresponding construction failure paths in teardownRevision so ClusterOperator
status preserves the error reason, including content-ID mismatches.
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.
This makes them a pure runtime substitution which doesn't require a rollout.
722d7c2 to
a44f32d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/revisiongenerator/revision.go (1)
51-93: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winUpdate the test transformer to the renamed revision interfaces.
pkg/controllers/installer/testvalue_transformer_test.gostill refers torevisiongenerator.RenderedRevisionandrevisiongenerator.RenderedComponent; switch these torevisiongenerator.ParsedRevisionandrevisiongenerator.ParsedComponentor the package won’t compile.🤖 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/revisiongenerator/revision.go` around lines 51 - 93, Update the test transformer in installer test values to use revisiongenerator.ParsedRevision and revisiongenerator.ParsedComponent instead of the removed RenderedRevision and RenderedComponent interfaces, preserving the existing transformer behavior.Source: Linters/SAST tools
🧹 Nitpick comments (2)
pkg/controllers/installer/installer_controller_test.go (2)
198-241: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the repository’s required Ginkgo test style.
The new
Ittitles should use theshould...form, and each scenario should expose setup, mutation, trigger, and verification steps throughBy().As per coding guidelines, Ginkgo tests should use descriptive
should...names andBy()for test steps.🤖 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/installer_controller_test.go` around lines 198 - 241, Update the two `It` blocks in the `ManifestTransformer Drift` context to use descriptive titles beginning with “should”. Add `By()` steps that clearly identify setup, mutation, reconciliation trigger, and verification in each scenario, while preserving the existing test behavior and assertions.Source: Coding guidelines
203-218: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAssert that drift correction does not create a new revision.
This test verifies the updated annotation but not the “without a new revision” contract stated in its title. Capture the revision state before changing
testAnnotationValueand assert that the revision name/status remains unchanged aftertriggerReconcile().The PR objective specifically requires transformer changes to roll out without creating a new Revision.
🤖 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/installer_controller_test.go` around lines 203 - 218, Update the test identified by “re-applies an updated transformer output without a new revision” to capture the current revision name and status after the initial successful reconciliation, before changing testAnnotationValue. After triggerReconcile() and verifying the updated annotation, assert that the revision name and status are unchanged, preserving the no-new-Revision contract.
🤖 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 72-79: Update testValueTransformer.WithRevision and WithComponent
to accept revisiongenerator.ParsedRevision and revisiongenerator.ParsedComponent
respectively, replacing the nonexistent RenderedRevision and RenderedComponent
types while preserving the existing no-op behavior and ManifestTransformer
return values.
---
Outside diff comments:
In `@pkg/revisiongenerator/revision.go`:
- Around line 51-93: Update the test transformer in installer test values to use
revisiongenerator.ParsedRevision and revisiongenerator.ParsedComponent instead
of the removed RenderedRevision and RenderedComponent interfaces, preserving the
existing transformer behavior.
---
Nitpick comments:
In `@pkg/controllers/installer/installer_controller_test.go`:
- Around line 198-241: Update the two `It` blocks in the `ManifestTransformer
Drift` context to use descriptive titles beginning with “should”. Add `By()`
steps that clearly identify setup, mutation, reconciliation trigger, and
verification in each scenario, while preserving the existing test behavior and
assertions.
- Around line 203-218: Update the test identified by “re-applies an updated
transformer output without a new revision” to capture the current revision name
and status after the initial successful reconciliation, before changing
testAnnotationValue. After triggerReconcile() and verifying the updated
annotation, assert that the revision name and status are unchanged, preserving
the no-new-Revision contract.
🪄 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: d4d9425d-988e-4207-bc26-0a73b2fd5a8b
📒 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/transform.go
- pkg/revisiongenerator/transform_test.go
- pkg/revisiongenerator/validate_test.go
🚧 Files skipped from review as they are similar to previous changes (15)
- pkg/manifesttransformer/manifest_transformer.go
- pkg/controllers/installer/suite_test.go
- pkg/manifesttransformer/validate.go
- pkg/revisiongenerator/helpers_test.go
- pkg/controllers/installer/helpers_test.go
- cmd/capi-installer/main.go
- pkg/manifesttransformer/validate_test.go
- pkg/controllers/installer/boxcutter.go
- pkg/controllers/installer/installer_controller.go
- pkg/controllers/installer/revision_reconciler_test.go
- pkg/revisiongenerator/validate.go
- pkg/controllers/installer/boxcutter_test.go
- pkg/controllers/revision/revision_controller.go
- pkg/revisiongenerator/revision_test.go
- pkg/controllers/revision/revision_controller_test.go
| 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
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n 'WithRevision|WithComponent|ParsedRevision|ParsedComponent|RenderedRevision|RenderedComponent' \
pkg/manifesttransformer/manifest_transformer.go \
pkg/controllers/installer/testvalue_transformer_test.go \
pkg/revisiongeneratorRepository: openshift/cluster-capi-operator
Length of output: 7405
Use ParsedRevision and ParsedComponent in the transformer methods. revisiongenerator.RenderedRevision and revisiongenerator.RenderedComponent don’t exist, so this test file won’t compile.
🧰 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 72 -
79, Update testValueTransformer.WithRevision and WithComponent to accept
revisiongenerator.ParsedRevision and revisiongenerator.ParsedComponent
respectively, replacing the nonexistent RenderedRevision and RenderedComponent
types while preserving the existing no-op behavior and ManifestTransformer
return values.
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
TLS substitutions are moved out of the revision. They are now a pure runtime concern, meaning they rollout without creating a new revision. If the latest revision cannot be installed, they will be still applied to a prior revision.
TODO:
/hold
Summary by CodeRabbit
New Features
Bug Fixes
Tests