OCPCLOUD-3647: Convert adopt-existing to use a ManifestTransformer - #635
OCPCLOUD-3647: Convert adopt-existing to use a ManifestTransformer#635mdbooth wants to merge 7 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 PR adds configurable manifest transformers, moves adopt-existing validation into the transformer package, unifies rendered objects, applies transformations during Boxcutter revision construction, and wires transformers through installer and revision controller reconciliation. ChangesManifest transformer integration
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CAPIInstaller
participant RevisionController
participant InstallerController
participant ManifestTransformer
participant Boxcutter
CAPIInstaller->>RevisionController: configure Transformers
CAPIInstaller->>InstallerController: configure transformers
RevisionController->>ManifestTransformer: validate rendered objects
InstallerController->>ManifestTransformer: transform installer objects
ManifestTransformer-->>InstallerController: transformed objects and reconcile options
InstallerController->>Boxcutter: build revision phases
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 13 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (13 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
/hold |
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: 1
🧹 Nitpick comments (6)
pkg/controllers/installer/revision_reconciler_test.go (1)
201-219: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFollow the required Ginkgo test conventions.
Rename the
Ittitle to start with “should”, add aBy(...)before reconciliation, and give the first three assertions failure messages.As per coding guidelines, use descriptive test names in “should...” format with
By()for test steps, and include meaningful assertion messages.🤖 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_test.go` around lines 201 - 219, Update the test title in revisionReconciler.reconcileRevision to start with “should” and add a By(...) step immediately before calling reconcileRevision. Add meaningful failure messages to the first three assertions while preserving their existing expectations and behavior.Source: Coding guidelines
pkg/manifesttransformer/validate_test.go (1)
31-132: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffUse the repository’s Ginkgo suite pattern.
This
testing.T/t.Runsuite should be converted toDescribe/It; the nil/empty and multi-object variants are goodDescribeTablecandidates.As per coding guidelines, “Use Ginkgo/Gomega framework and prefer built-in features: use
DescribeTablewithEntryfor table-driven tests.”🤖 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 31 - 132, The TestValidateTransformers suite currently uses testing.T and t.Run instead of the repository’s Ginkgo pattern. Convert TestValidateTransformers to Describe/It with Gomega assertions, and consolidate the nil-transformers, empty-transformers, and multi-object cases into appropriate DescribeTable entries using Entry; preserve all existing validation and error-aggregation coverage.Source: Coding guidelines
pkg/controllers/revision/revision_controller_test.go (1)
588-588: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the reconcile result rather than discarding it.
Swallowing both returns hides whether
Reconcilesurfaced a non-retryable (terminal) error; assert the error explicitly alongside the conditions.♻️ 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).To(MatchError(ContainSubstring("invalid manifest")), "Reconcile should surface the transformer validation failure")🤖 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` at line 588, Update the Reconcile invocation in the test to capture both returned values instead of discarding them, then explicitly assert that the returned error matches the expected non-retryable outcome alongside the existing condition assertions.pkg/controllers/installer/boxcutter.go (1)
69-70: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueHoist the CRD GroupKind literal.
The same
schema.GroupKind{Group: "apiextensions.k8s.io", Kind: "CustomResourceDefinition"}literal is repeated inpkg/controllers/installer/revision_reconciler.go(Line 462). A package-levelvar crdGroupKind = ...(orapiextensionsv1.SchemeGroupVersion.WithKind(...)) keeps the two in sync.🤖 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 69 - 70, Define a shared package-level CRD GroupKind value and replace the inline literal in the installer controller’s object-kind comparison with that variable. Reuse the same value in revision_reconciler.go so both CRD checks remain synchronized.pkg/revisiongenerator/validate.go (1)
19-24: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider renaming the file now that it only holds annotation constants.
All validation moved to
pkg/manifesttransformer;validate.gocontaining just the adopt-existing constants is misleading (e.g.annotations.go).🤖 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/validate.go` around lines 19 - 24, Rename validate.go to annotations.go because it now only defines annotation constants after validation moved to pkg/manifesttransformer. Keep the existing annotation constants and contents unchanged.pkg/controllers/installer/boxcutter_test.go (1)
180-213: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd failure messages to these assertions.
Lines 181, 199–201 and 211–213 assert phase counts/names without messages, unlike the rest of the file. As per coding guidelines, "assertions should include meaningful failure messages, flag missing messages".
🤖 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 180 - 213, Add meaningful failure messages to the phase count and phase name assertions in the affected tests around the mixed, no-CRD, and CRD-only cases. Update the assertions on phases length, phases[0].GetName(), and related phase expectations to describe the expected phase structure, matching the existing message style in the file.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.go`:
- Around line 64-75: Move the collectObjects invocation in the object-processing
flow so it receives each object only after applyTransformers has transformed or
retained it. Skip collection when the transformer returns nil, and ensure
relatedObjects, gvks, and CRD plural mappings use the transformed object
identity rather than the original.
---
Nitpick comments:
In `@pkg/controllers/installer/boxcutter_test.go`:
- Around line 180-213: Add meaningful failure messages to the phase count and
phase name assertions in the affected tests around the mixed, no-CRD, and
CRD-only cases. Update the assertions on phases length, phases[0].GetName(), and
related phase expectations to describe the expected phase structure, matching
the existing message style in the file.
In `@pkg/controllers/installer/boxcutter.go`:
- Around line 69-70: Define a shared package-level CRD GroupKind value and
replace the inline literal in the installer controller’s object-kind comparison
with that variable. Reuse the same value in revision_reconciler.go so both CRD
checks remain synchronized.
In `@pkg/controllers/installer/revision_reconciler_test.go`:
- Around line 201-219: Update the test title in
revisionReconciler.reconcileRevision to start with “should” and add a By(...)
step immediately before calling reconcileRevision. Add meaningful failure
messages to the first three assertions while preserving their existing
expectations and behavior.
In `@pkg/controllers/revision/revision_controller_test.go`:
- Line 588: Update the Reconcile invocation in the test to capture both returned
values instead of discarding them, then explicitly assert that the returned
error matches the expected non-retryable outcome alongside the existing
condition assertions.
In `@pkg/manifesttransformer/validate_test.go`:
- Around line 31-132: The TestValidateTransformers suite currently uses
testing.T and t.Run instead of the repository’s Ginkgo pattern. Convert
TestValidateTransformers to Describe/It with Gomega assertions, and consolidate
the nil-transformers, empty-transformers, and multi-object cases into
appropriate DescribeTable entries using Entry; preserve all existing validation
and error-aggregation coverage.
In `@pkg/revisiongenerator/validate.go`:
- Around line 19-24: Rename validate.go to annotations.go because it now only
defines annotation constants after validation moved to pkg/manifesttransformer.
Keep the existing annotation constants and contents unchanged.
🪄 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: 35d18b42-f171-4128-a3bd-bb64473d9aea
📒 Files selected for processing (19)
cmd/capi-installer/main.gopkg/controllers/installer/boxcutter.gopkg/controllers/installer/boxcutter_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/manifest_transformer.gopkg/manifesttransformer/validate.gopkg/manifesttransformer/validate_test.gopkg/revisiongenerator/revision.gopkg/revisiongenerator/revision_test.gopkg/revisiongenerator/validate.gopkg/revisiongenerator/validate_test.go
💤 Files with no reviewable changes (1)
- pkg/revisiongenerator/validate_test.go
| for _, obj := range component.Objects() { | ||
| if collectObjects != nil { | ||
| collectObjects(obj) | ||
| } | ||
|
|
||
| gvk := obj.GetObjectKind().GroupVersionKind() | ||
| if gvk.GroupKind() == (schema.GroupKind{Group: "apiextensions.k8s.io", Kind: "CustomResourceDefinition"}) { | ||
| crds = append(crds, obj) | ||
| } else { | ||
| objects = append(objects, obj) | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
collectObjects sees pre-transform objects, including ones transformers skip.
Objects are collected before applyTransformers runs, so an object a transformer drops (nil return) still lands in relatedObjects/gvks/the CRD plural mapping, and renamed objects are collected under their original identity. If collection is meant to describe what is actually applied, move the callback after transformation.
🤖 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 64 - 75, Move the
collectObjects invocation in the object-processing flow so it receives each
object only after applyTransformers has transformed or retained it. Skip
collection when the transformer returns nil, and ensure relatedObjects, gvks,
and CRD plural mappings use the transformed object identity rather than the
original.
b92c47a to
4f0f78d
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/controllers/installer/installer_controller_test.go (1)
198-241: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAlign the new specs with the required Ginkgo conventions.
Rename the
Itblocks toshould...form, addBy()steps around setup/trigger/update operations, and provide meaningful messages for the new assertions.As per coding guidelines, Ginkgo tests should use descriptive
should...names,By()steps, and meaningful assertion messages.🤖 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 descriptive should... names. Add By() steps around setup, transformer changes, reconcile triggers, and annotation removal, and provide meaningful messages for each newly added assertion, especially the Eventually checks verifying annotation values.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/controllers/installer/installer_controller_test.go`:
- Around line 198-241: Update the two It blocks in the ManifestTransformer Drift
context to descriptive should... names. Add By() steps around setup, transformer
changes, reconcile triggers, and annotation removal, and provide meaningful
messages for each newly added assertion, especially the Eventually checks
verifying annotation values.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 6d8cc960-d1f2-4614-8db3-737025457d50
📒 Files selected for processing (11)
cmd/capi-installer/main.gopkg/controllers/installer/boxcutter.gopkg/controllers/installer/installer_controller_test.gopkg/controllers/installer/suite_test.gopkg/controllers/installer/testvalue_transformer_test.gopkg/controllers/revision/helpers_test.gopkg/manifesttransformer/adopt_existing_transformer.gopkg/manifesttransformer/adopt_existing_transformer_test.gopkg/revisiongenerator/revision.gopkg/revisiongenerator/validate.gopkg/revisiongenerator/validate_test.go
💤 Files with no reviewable changes (2)
- pkg/revisiongenerator/validate_test.go
- pkg/revisiongenerator/revision.go
🚧 Files skipped from review as they are similar to previous changes (6)
- pkg/manifesttransformer/adopt_existing_transformer_test.go
- pkg/revisiongenerator/validate.go
- pkg/controllers/revision/helpers_test.go
- pkg/manifesttransformer/adopt_existing_transformer.go
- cmd/capi-installer/main.go
- pkg/controllers/installer/boxcutter.go
|
@mdbooth: all tests passed! 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
Converts the existing adopt-existing transformation to use a ManifestTransformer.
TODO:
Summary by CodeRabbit
New Features
Bug Fixes
Tests