Update transform with kustomize and multistage#211
Merged
aufi merged 31 commits intomigtools:mainfrom Apr 10, 2026
Merged
Conversation
Implements Phase 6-F1: - Stage discovery scans transform directory for subdirectories matching pattern <number>_<pluginName> (e.g., 10_kubernetes, 20_openshift) - Stages sorted by priority (numeric prefix) for deterministic ordering - FilterStages supports multiple selectors: specific stage, from-stage, to-stage, or list of stages - Helper functions for stage navigation: GetFirstStage, GetLastStage, GetPreviousStage, GetNextStage - ValidateStageName and GenerateStageName utilities Comprehensive test coverage for all discovery and filtering scenarios. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Implements Phase 2-B1 and B2: - KustomizeWriter orchestrates writing all transform artifacts to Kustomize layout (resources/, patches/, kustomization.yaml) - Groups resources by type and writes multi-doc YAML files - Generates patches with target selectors - Creates whiteout and ignored-patches reports in YAML format - Implements dirty check integration to prevent overwriting user edits - Writes metadata with SHA256 content hashes for change tracking - Helper functions to parse ResourceGroup TypeKey and extract JSONPatch operation details Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Implements Phase 6-F2: - Orchestrator coordinates single-stage and multi-stage transform execution - RunSingleStage: Default mode creating one transform stage - RunMultiStage: Executes pipeline with stage chaining - Stage chaining: output of stage N becomes input for stage N+1 - Integrates with KustomizeWriter to write Kustomize layout - Parses JSONPatch from existing Runner.Run() response - loadStageOutput reads resources from completed stage Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Implements Phase 3-C1 and C2: - KustomizeApplier orchestrates applying transformations via kubectl kustomize - ApplySingleStage: Applies a specific stage to produce output YAML - ApplyMultiStage: Applies multiple stages with selector support - ApplyFinalStage: Applies the last stage in pipeline (typical use case) - Integrates with stage discovery to find and process stages - Validates kubectl availability before execution - Writes combined output YAML for each stage Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Implements Phase 3-C3: - ValidateStage: Checks stage directory structure, kustomization.yaml, resources, patches, and metadata - ValidateAllStages: Validates all discovered stages - ValidatePipeline: End-to-end pipeline validation with error reporting - ValidateOutputDirectory: Ensures output directory is suitable - ValidateStageChaining: Verifies stages can be chained correctly - Integrates dirty check to warn about user modifications - Validates kustomization.yaml by running kubectl kustomize build - Comprehensive error and warning reporting Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Implements Phase 6-F3: - Transform command flags: --stage: Run specific stage only --from-stage: Run from stage onwards --to-stage: Run up to and including stage --stages: Run specific stages (comma-separated) --stage-name: Name for output stage (default: '10_transform') --plugin-name: Plugin name for metadata (default: 'transform') --force: Force overwrite of existing stages with user modifications - Apply command flags: --stage, --from-stage, --to-stage, --stages: Stage selectors --final-only: Apply only final stage (default: true) --validate: Run preflight validation (default: true) --force: Force overwrite of output directory Flags support full multi-stage pipeline control and integrate with stage discovery and filtering mechanisms. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Implements Phase 6-F4:
- AutoAssignPriorities: Generates priority map from discovered stages
- AutoAssignPrioritiesFromDir: Discovers stages and assigns priorities
- MergePriorities: Merges user-specified and auto-assigned priorities
- SuggestStageName: Suggests next available stage name for a plugin
- ParsePluginPrioritiesFromStrings: Parses priority strings ("plugin:10")
- GetPriorityAssignments: Returns all plugin priority assignments
- ValidatePriorityAssignments: Checks for priority conflicts
- RecommendPriorityOrder: Heuristic-based priority recommendations
Features:
- Fills significant gaps (> 5) in priority sequence
- Appends new stages with increment of 10
- Intelligent plugin categorization (kubernetes:10, openshift:20, etc.)
- Comprehensive test coverage for all functions
Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Implements task migtools#11: - TestKustomizeWriterIntegration: End-to-end test of KustomizeWriter creating stage directory structure, resources, kustomization.yaml, and metadata - TestStageDiscoveryIntegration: Tests stage discovery and filtering with multiple stages - TestDirtyCheckIntegration: Tests dirty check workflow including metadata generation, modification detection, and EnsureCleanDirectory Test fixtures: - deployment.yaml: Sample Kubernetes Deployment - service.yaml: Sample Kubernetes Service All integration tests pass with -tags=integration flag. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Implements task migtools#12: docs/kustomize-multistage.md: - Complete feature overview and key concepts - Stage directory structure and resource grouping - CLI usage for transform and apply commands - Priority assignment (auto, manual, recommended) - Stage chaining workflow - Migration guide from JSONPatch workflow - Advanced features and API reference - Troubleshooting guide and best practices docs/TRANSFORM_README.md: - Quick start guide for transform directory - Explanation of directory contents - Common workflows (review, customize, re-run, chain) - Git best practices - Troubleshooting common issues - Advanced Kustomize customization examples Documentation includes: - Complete CLI flag reference - Code examples in Go - Command-line examples - Directory structure diagrams - Migration paths - Best practices Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Implements tasks migtools#21 and migtools#23: Transform command (cmd/transform/transform.go): - Detects if new Kustomize workflow should be used based on flags - runKustomizeWorkflow(): Uses Orchestrator for single/multi-stage execution - runLegacyWorkflow(): Preserves existing JSONPatch workflow - Supports --stage, --from-stage, --to-stage, --stages flags - Validates mutually exclusive flag usage Apply command (cmd/apply/apply.go): - Detects if new Kustomize workflow should be used - runKustomizeWorkflow(): Uses KustomizeApplier with kubectl kustomize - runLegacyWorkflow(): Preserves existing apply workflow - Supports --stage, --from-stage, --to-stage, --stages, --final-only flags - Includes preflight validation with --validate flag - Validates mutually exclusive flag usage Both commands maintain backward compatibility by defaulting to legacy workflow when multi-stage flags are not used. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Implements task migtools#24: New subcommand: crane transform validate - Validates all stages in transform directory - Checks stage directory structure - Validates kustomization.yaml syntax - Verifies resource and patch file references - Validates stage chaining correctness - Checks dirty status of stages - Provides clear success/warning/error output with emoji indicators Features: - --transform-dir flag to specify transform directory - --verbose flag to show all warnings - Validates each stage individually - Validates stage chaining across pipeline - User-friendly output with ✓,⚠️ , ✗ indicators - Returns non-zero exit code on validation failure Usage: crane transform validate crane transform validate --transform-dir transform crane transform validate --verbose Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Signed-off-by: Marek Aufart <maufart@redhat.com>
Remove helper features while keeping core Kustomize and multi-stage functionality: Removed files: - priority.go, priority_test.go - auto-assignment not needed for MVP - dirtycheck.go, dirtycheck_test.go, metadata.go - can add later - validation.go, validate/ subcommand - can add later - reports.go - whiteout/ignored patches reports not critical - ordering.go, ordering_test.go - deterministic ordering not critical - integration_test.go - depends on removed features Simplified: - writer.go: Removed metadata, dirty check, reports generation Now just writes resources, patches, and kustomization.yaml - orchestrator.go: Updated to match simplified writer signature - apply.go: Removed validation flags and checks - transform.go: Removed validate subcommand reference Core functionality preserved: ✓ Kustomize layout generation (resources/, patches/, kustomization.yaml) ✓ Multi-stage pipeline support ✓ Stage discovery and filtering ✓ Resource grouping by type ✓ Integration with crane transform and apply commands All tests passing. Implementation is now simpler and focused on core features.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a Kustomize-native multi-stage transform and apply pipeline with stage discovery/selection, orchestrator, Kustomize writer/applier, CLI flags for stage selection, and validation enforcing mutually exclusive selection modes. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI as cmd/transform
participant StageDiscovery as Stage Discovery
participant Orchestrator
participant PluginLoader as Plugin Loader
participant Runner as cranelib.Runner
participant Writer as KustomizeWriter
participant FS as Filesystem
User->>CLI: run transform (--stage/--from-stage/--to-stage/--stages)
CLI->>CLI: shouldUseKustomizeWorkflow()
CLI->>StageDiscovery: DiscoverStages(TransformDir)
StageDiscovery->>FS: read transform dir
FS-->>StageDiscovery: stage dirs
StageDiscovery-->>CLI: sorted stages
CLI->>Orchestrator: RunMultiStage(StageSelector)
loop per selected stage
Orchestrator->>PluginLoader: load plugins
PluginLoader->>FS: read plugin files
FS-->>PluginLoader: plugin data
Orchestrator->>Orchestrator: determine stage input (export or prev stage)
loop per input resource
Orchestrator->>Runner: run(resource, plugins)
Runner-->>Orchestrator: TransformArtifact
end
Orchestrator->>Writer: WriteStage(artifacts, force)
Writer->>FS: create stage dirs & write resources/patches/kustomization
FS-->>Writer: write complete
end
Orchestrator-->>CLI: success
CLI-->>User: stages written
sequenceDiagram
participant User
participant CLI as cmd/apply
participant Applier as KustomizeApplier
participant StageDiscovery as Stage Discovery
participant Kubectl as kubectl
participant FS as Filesystem
User->>CLI: run apply (--stage/--from-stage/--to-stage/--stages/--final-only)
CLI->>CLI: shouldUseKustomizeWorkflow()
CLI->>StageDiscovery: DiscoverStages(TransformDir)
StageDiscovery->>FS: read transform dir
StageDiscovery-->>CLI: stages
CLI->>Applier: NewKustomizeApplier(TransformDir, OutputDir)
alt Multi-Stage
CLI->>Applier: ApplyMultiStage(selector)
loop per filtered stage
Applier->>Kubectl: kustomize build <stage>
Kubectl-->>Applier: rendered YAML
Applier->>FS: write <stage>.yaml to OutputDir
end
else Final-Only
CLI->>Applier: ApplyFinalStage()
Applier->>Kubectl: kustomize build <last-stage>
Kubectl-->>Applier: rendered YAML
Applier->>FS: write output.yaml to OutputDir
end
Applier-->>CLI: success
CLI-->>User: manifests rendered
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Signed-off-by: Marek Aufart <maufart@redhat.com>
Test Coverage ReportTotal: 28.1% Per-package coverage
Full function-level detailsPosted by CI |
Contributor
Author
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (7)
test-data/kustomize-transform/fixtures/service.yaml (1)
1-13: Consider renaming to follow the resource naming guideline.The filename
service.yamldoes not follow the documented resource naming convention:Kind_group_version_namespace_name.yaml. For consistency, consider renaming toService_v1_default_myapp-service.yaml.However, if test fixtures intentionally use simple names for readability, this can be considered acceptable for test data.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test-data/kustomize-transform/fixtures/service.yaml` around lines 1 - 13, The file uses a generic fixture filename; rename it to follow the project's resource naming convention by using the pattern Kind_group_version_namespace_name.yaml — for this resource change service.yaml to Service_v1_default_myapp-service.yaml (references: kind "Service", apiVersion "v1", metadata.name "myapp-service", metadata.namespace "default") unless you intentionally want simpler test filenames for readability.docs/TRANSFORM_README.md (2)
9-19: Add language specifier to fenced code block.The code block showing the directory structure should have a language specifier for consistent rendering. Since this shows a directory tree, use an empty language or a generic identifier.
📝 Proposed fix
-``` +```text transform/ └── 10_transform/🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/TRANSFORM_README.md` around lines 9 - 19, The fenced code block that shows the directory tree (starting with the lines "transform/" and "└── 10_transform/") needs a language specifier for consistent rendering; update the Markdown fenced block delimiter from ``` to ```text (or another generic identifier) so the directory tree is rendered as plain text, keeping the existing block contents unchanged.
70-75: Add language specifier to directory structure code block.📝 Proposed fix
-``` +```text transform/ ├── 10_kubernetes/ # Base Kubernetes transformations🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/TRANSFORM_README.md` around lines 70 - 75, The fenced code block showing the directory structure (the block beginning with the line "transform/") is missing a language specifier; update the opening fence from ``` to ```text so the snippet is explicitly marked as plain text (i.e., change the fenced block that contains "transform/" and the tree lines to start with ```text).internal/file/file_helper_test.go (1)
314-350: Good multi-stage path validation with table-driven approach.The test correctly validates the stage directory structure across multiple stage names. Consider adding edge case tests for boundary conditions such as empty
stageNameor stage names with special characters, if those scenarios should be handled gracefully.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/file/file_helper_test.go` around lines 314 - 350, Add table-driven edge-case subtests to TestMultiStagePaths to validate behavior when stageName is empty and when it contains special characters; use the existing opts (file.PathOpts) and call its methods GetStageDir, GetResourcesDir, and GetKustomizationPath for those inputs, asserting expected outputs (or explicit error/empty behavior if that's the defined contract) so the test covers boundary conditions alongside the current normal cases.internal/transform/stages_test.go (1)
58-62: Consider documenting the nonexistent directory behavior.
DiscoverStagesreturns an empty list without error for nonexistent directories. While this may be intentional (treating missing directory as "no stages"), this could mask configuration errors. Consider whether this behavior is documented or if an error should be returned.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/transform/stages_test.go` around lines 58 - 62, The test shows DiscoverStages returns an empty slice and no error for a missing directory; decide and document that behavior: if intended, add a clear doc comment on the DiscoverStages function explaining that a nonexistent path is treated as "no stages" and will not produce an error (and optionally rename TestDiscoverStagesNonexistentDir to reflect that expectation); if unintended, change DiscoverStages to return an error for nonexistent directories and update TestDiscoverStagesNonexistentDir to expect that error instead. Ensure references to DiscoverStages and TestDiscoverStagesNonexistentDir are updated accordingly.internal/file/file_helper.go (1)
6-6: Consider migrating from deprecatedioutilpackage.The
ioutilpackage is deprecated since Go 1.16. While this is existing code, consider updating to useos.ReadDirandos.ReadFileinstead ofioutil.ReadDirandioutil.ReadFilein future cleanup.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/file/file_helper.go` at line 6, Replace the deprecated import "io/ioutil" and any usages of ioutil.ReadDir and ioutil.ReadFile in this file with the modern equivalents from the os package (use os.ReadDir and os.ReadFile), update the import block to remove "io/ioutil" and add "os", and adjust any code handling the returned types (os.ReadDir returns []os.DirEntry) accordingly (e.g., use DirEntry methods or convert to os.FileInfo if needed).docs/kustomize-multistage.md (1)
15-33: Add language specifier to directory structure code blocks.Several fenced code blocks showing directory structures lack language specifiers. For consistency, add
textor leave empty with proper markdown.📝 Proposed fix for lines 15 and similar blocks
-``` +```text transform/ ├── 10_kubernetes/🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/kustomize-multistage.md` around lines 15 - 33, The fenced code blocks that show directory trees (e.g., the block starting with "transform/" and containing "10_kubernetes/", "20_openshift/", "30_imagestream/") are missing a language specifier; update those opening fences from ``` to ```text (or another explicit specifier) for consistency and proper rendering, and apply the same change to any other similar directory-structure blocks in the document.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/apply/apply.go`:
- Around line 51-69: The Validate() logic treats Flags.FinalOnly's default true
as an explicit selector, causing mutual-exclusion false-positives; change the
flag handling so FinalOnly is only considered when explicitly set (e.g., convert
Flags.FinalOnly to a pointer (*bool) or add an explicit Flags.FinalOnlySet
boolean that the flag parser sets), update Options.Validate() to increment
flagCount only when FinalOnly was explicitly provided (FinalOnlySet or FinalOnly
!= nil && *FinalOnly), and update shouldUseKustomizeWorkflow() and the other
checks mentioned (the blocks around the other mutual-exclusion checks) to use
the same explicit-set check so default state does not count as an opt-in. Ensure
you also update the flag registration/parsing code to set FinalOnlySet or to use
a pointer flag so the presence of the flag is detectable.
- Line 121: The --force flag is defined but ignored; modify the apply command to
respect o.Force by preventing accidental overwrites and by cleaning stale
outputs: when o.Force is false, refuse to create/overwrite the output directory
if it exists and is not empty (return an error); when o.Force is true, remove or
truncate the existing output directory before writing so stale files are deleted
(e.g., remove the directory contents or delete then recreate it) prior to the
write phase. Update the code path that currently creates the output dir in
cmd/apply (where cmd.Flags().BoolVar(&o.Force, ... ) is set) and the writer
logic in internal/apply/kustomize.go (the routines that call os.WriteFile) so
they check o.Force and avoid unconditional os.WriteFile overwrites or perform a
pre-write cleanup when Force==true.
In `@cmd/transform/transform.go`:
- Around line 125-132: The check that decides Kustomize vs legacy JSONPatch is
inspecting flag values (o.StageName, o.PluginName, o.Force) instead of whether
the user actually provided those flags, so default-valued flags incorrectly
trigger the Kustomize path; update the condition to use the FlagSet's Changed
method (e.g., cmd.Flags().Changed("stage-name"),
cmd.Flags().Changed("plugin-name"), cmd.Flags().Changed("force")) when deciding
the single-stage Kustomize flow (and apply the same change to the other similar
check at the other block around the 152-157 region), ensuring you reference the
existing variables o.StageName/o.PluginName/o.Force and the flag names exactly.
In `@docs/kustomize-multistage.md`:
- Around line 408-447: Update the API reference to match the real
implementation: remove undocumented/absent functions AutoAssignPriorities,
MergePriorities, IsDirectoryDirty, EnsureCleanDirectory, ValidateStage,
ValidatePipeline and replace name mismatches (use GenerateStageName instead of
SuggestStageName, and ValidateStageName instead of ValidateStage). In the
Transform examples, use the actual available helpers (DiscoverStages,
StageSelector, FilterStages) and show usage of GetFirstStage, GetLastStage,
GetPreviousStage and GetNextStage where stage navigation is demonstrated; in the
Apply examples, replace ApplyFinalStage/ValidateStage/ValidatePipeline with the
real applier methods ApplySingleStage and ApplyMultiStage and the
ValidateStageName function, and ensure each example matches the real function
signatures in the codebase.
In `@docs/TRANSFORM_README.md`:
- Around line 34-36: The README shows an incorrect mixed command `kubectl
kustomize build transform/10_transform/`; replace it with one correct form:
either use kubectl's built-in kustomize (`kubectl kustomize
transform/10_transform/`) or the standalone Kustomize build command (`kustomize
build transform/10_transform/`), updating the code block accordingly so it uses
a single valid tool invocation.
In `@internal/apply/kustomize.go`:
- Around line 143-166: The kubectl invocations use incorrect flags/subcommands:
in runKustomizeBuild remove the extra "build" argument so the command becomes
"kubectl kustomize DIR" (locate runKustomizeBuild to change exec.Command
invocation), and in ValidateKubectlAvailable remove the undocumented "--short"
flag so the command is "kubectl version --client" (locate
ValidateKubectlAvailable to update exec.Command). Ensure error messages and
Debugf log in runKustomizeBuild remain unchanged and continue to return stderr
on failure.
In `@internal/transform/orchestrator.go`:
- Around line 201-223: loadStageOutput currently reads only the resources
directory via opts.GetResourcesDir and file.ReadFiles, which ignores the stage's
patches wired through kustomization.yaml; change loadStageOutput to run a
kustomize build (e.g., invoke kubectl kustomize or the kustomize library)
against the stage's kustomization (stage directory or resourcesDir where
kustomization.yaml lives) and parse the rendered manifest into
[]unstructured.Unstructured before returning so previous-stage JSONPatch
operations are applied; preserve existing error wrapping and return types when
replacing the file.ReadFiles call.
- Around line 113-131: The first selected stage is always seeded from
o.ExportDir because the code checks if i == 0, but when doing a partial rerun
the first selected stage may have a predecessor in the full pipeline and should
instead load that predecessor's output via o.loadStageOutput. Change the
branching for seeding inputResources so that it only reads from o.ExportDir when
the selected stage is truly the pipeline's initial export stage; otherwise
compute the actual previous stage in the full pipeline for selectedStages[i] and
call o.loadStageOutput(prevStage) (do not rely on selectedStages[i-1] unless it
represents the immediate prior stage in the full pipeline). Use the
selectedStages slice and o.loadStageOutput to locate and load the correct
predecessor stage output (and preserve the existing error handling and logging
when loadStageOutput fails).
In `@internal/transform/stages.go`:
- Around line 116-137: The current range filter (iterating allStages and using
selector.FromStage/ToStage) doesn't detect missing stage names, so a misspelled
selector.ToStage (or FromStage) silently widens the range; fix by tracking
whether the requested FromStage and ToStage were actually seen: add booleans
like foundFrom and foundTo updated when stage.DirName matches
selector.FromStage/ToStage, and after the loop return an empty slice or an error
when selector.FromStage != "" and !foundFrom, or when selector.ToStage != "" and
!foundTo; keep the same logic in the loop (variables filtered, inRange,
allStages, Stage.DirName) but enforce that both specified endpoints must be
found before returning filtered results.
In `@internal/transform/writer.go`:
- Around line 32-37: The current logic only skips os.RemoveAll(stageDir) when
force is false but still proceeds to overwrite resource bundles, patches and
kustomization.yaml; change the flow so that before any writes (where you
currently use stageDir, write bundles/patches/kustomization) you check whether
stageDir exists and is non-empty and, if so and force==false, return an error
(or skip) instead of proceeding; if force==true keep the existing
RemoveAll(stageDir) path and allow writes. Apply the same existence/non-empty
guard to the other write locations referenced in the review (the blocks around
lines 78-80 and 105-120) so functions that create resource bundles, patch files,
and kustomization.yaml only run when either the directory is absent/empty or
force is true.
---
Nitpick comments:
In `@docs/kustomize-multistage.md`:
- Around line 15-33: The fenced code blocks that show directory trees (e.g., the
block starting with "transform/" and containing "10_kubernetes/",
"20_openshift/", "30_imagestream/") are missing a language specifier; update
those opening fences from ``` to ```text (or another explicit specifier) for
consistency and proper rendering, and apply the same change to any other similar
directory-structure blocks in the document.
In `@docs/TRANSFORM_README.md`:
- Around line 9-19: The fenced code block that shows the directory tree
(starting with the lines "transform/" and "└── 10_transform/") needs a language
specifier for consistent rendering; update the Markdown fenced block delimiter
from ``` to ```text (or another generic identifier) so the directory tree is
rendered as plain text, keeping the existing block contents unchanged.
- Around line 70-75: The fenced code block showing the directory structure (the
block beginning with the line "transform/") is missing a language specifier;
update the opening fence from ``` to ```text so the snippet is explicitly marked
as plain text (i.e., change the fenced block that contains "transform/" and the
tree lines to start with ```text).
In `@internal/file/file_helper_test.go`:
- Around line 314-350: Add table-driven edge-case subtests to
TestMultiStagePaths to validate behavior when stageName is empty and when it
contains special characters; use the existing opts (file.PathOpts) and call its
methods GetStageDir, GetResourcesDir, and GetKustomizationPath for those inputs,
asserting expected outputs (or explicit error/empty behavior if that's the
defined contract) so the test covers boundary conditions alongside the current
normal cases.
In `@internal/file/file_helper.go`:
- Line 6: Replace the deprecated import "io/ioutil" and any usages of
ioutil.ReadDir and ioutil.ReadFile in this file with the modern equivalents from
the os package (use os.ReadDir and os.ReadFile), update the import block to
remove "io/ioutil" and add "os", and adjust any code handling the returned types
(os.ReadDir returns []os.DirEntry) accordingly (e.g., use DirEntry methods or
convert to os.FileInfo if needed).
In `@internal/transform/stages_test.go`:
- Around line 58-62: The test shows DiscoverStages returns an empty slice and no
error for a missing directory; decide and document that behavior: if intended,
add a clear doc comment on the DiscoverStages function explaining that a
nonexistent path is treated as "no stages" and will not produce an error (and
optionally rename TestDiscoverStagesNonexistentDir to reflect that expectation);
if unintended, change DiscoverStages to return an error for nonexistent
directories and update TestDiscoverStagesNonexistentDir to expect that error
instead. Ensure references to DiscoverStages and
TestDiscoverStagesNonexistentDir are updated accordingly.
In `@test-data/kustomize-transform/fixtures/service.yaml`:
- Around line 1-13: The file uses a generic fixture filename; rename it to
follow the project's resource naming convention by using the pattern
Kind_group_version_namespace_name.yaml — for this resource change service.yaml
to Service_v1_default_myapp-service.yaml (references: kind "Service", apiVersion
"v1", metadata.name "myapp-service", metadata.namespace "default") unless you
intentionally want simpler test filenames for readability.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 51b7cff6-f438-4558-8c95-f9a0395fb01f
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (14)
cmd/apply/apply.gocmd/transform/transform.godocs/TRANSFORM_README.mddocs/kustomize-multistage.mdgo.modinternal/apply/kustomize.gointernal/file/file_helper.gointernal/file/file_helper_test.gointernal/transform/orchestrator.gointernal/transform/stages.gointernal/transform/stages_test.gointernal/transform/writer.gotest-data/kustomize-transform/fixtures/deployment.yamltest-data/kustomize-transform/fixtures/service.yaml
Signed-off-by: Marek Aufart <maufart@redhat.com>
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
internal/transform/writer.go (1)
29-48:⚠️ Potential issue | 🟠 MajorRefuse to write into a populated stage when
--forceis off.Skipping
RemoveAll(stageDir)is not enough here: the laterWriteFilecalls still replaceresources/,patches/, andkustomization.yaml. A rerun can still clobber manual edits even whenforce == false.Minimal fix
func (w *KustomizeWriter) WriteStage(artifacts []cranelib.TransformArtifact, force bool) error { stageDir := w.opts.GetStageDir(w.stageName) - // Remove existing stage directory if force is set - if force { + if !force { + entries, err := os.ReadDir(stageDir) + if err == nil && len(entries) > 0 { + return fmt.Errorf("stage directory %q already exists and is not empty; rerun with --force to overwrite", stageDir) + } + if err != nil && !os.IsNotExist(err) { + return fmt.Errorf("failed to inspect stage directory %q: %w", stageDir, err) + } + } + + if force { if err := os.RemoveAll(stageDir); err != nil && !os.IsNotExist(err) { - return fmt.Errorf("failed to remove existing stage directory: %w", err) + return fmt.Errorf("failed to remove existing stage directory %q: %w", stageDir, err) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/transform/writer.go` around lines 29 - 48, In KustomizeWriter.WriteStage, when force is false you must refuse to proceed if the target stage directory already exists and contains files; before creating resources/ and patches/ check stageDir (from w.opts.GetStageDir) with os.Stat or os.ReadDir and return a clear error instead of continuing so later WriteFile calls won't clobber manual edits. Keep the existing RemoveAll(stageDir) behavior when force is true, and only perform MkdirAll for resourcesDir/patchesDir after confirming stageDir is absent or empty.cmd/apply/apply.go (1)
50-68:⚠️ Potential issue | 🟠 MajorThis still doesn't track whether
--final-onlywas explicitly set.The flag is registered as a plain bool with a default, but
Validate()andshouldUseKustomizeWorkflow()treat it like a tri-state. Without a separateFinalOnlySet/Changed("final-only")check, the selector logic still can't reliably distinguish the default from an explicit opt-in.Run this read-only check to confirm there is no explicit-set tracking for
--final-onlyin the current tree:#!/bin/bash set -euo pipefail echo "=== final-only registration in cmd/apply/apply.go ===" sed -n '109,140p' cmd/apply/apply.go echo echo "=== explicit-set tracking for final-only ===" rg -n 'FinalOnlySet|Changed\("final-only"\)|IsSet\("final-only"\)' --type=go echo echo "=== all final-only usages ===" rg -n '\bFinalOnly\b|final-only' --type=goExpected result: the search finds default-value registration and value checks, but no mechanism that records whether the flag was actually provided by the user.
Also applies to: 119-120, 136-138
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/apply/apply.go` around lines 50 - 68, The Validate() logic incorrectly treats Flags.FinalOnly as a tri-state without tracking whether the user explicitly set --final-only; update the flag registration and checks so explicit-set is recorded (e.g. store a separate boolean like FinalOnlySet or use the flagset's Changed("final-only")/IsSet API) and change Options.Validate() and shouldUseKustomizeWorkflow() to consult that explicit-set indicator rather than the raw Flags.FinalOnly value; specifically, add and set FinalOnlySet when parsing the flag (or switch to a pointer/Var registration that can distinguish unset), then replace direct checks of Flags.FinalOnly in Validate() and shouldUseKustomizeWorkflow() with checks that require FinalOnlySet (or flagset.Changed) plus the value.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/kustomize-multistage.md`:
- Around line 143-160: The docs mention a --validate flag that no longer exists;
fix by either restoring the flag in the CLI or removing the docs. To restore,
add a boolean flag registration and wiring in cmd/apply/apply.go (e.g., on
NewApplyCmd or ApplyCommand initialization) that sets an ApplyOptions.Validate
(or similar) field and is respected by the apply handler (where ApplyOptions are
used); ensure the flag defaults to true and is parsed into the code path that
calls the existing validation routine. Alternatively, if you choose to remove
docs, delete or replace the `--validate` examples and the "Validation" section
in docs/kustomize-multistage.md so the docs reflect the current CLI behavior.
In `@internal/transform/orchestrator.go`:
- Around line 41-44: Currently the code loads the full plugin list into the
variable plugins via plugin.GetFilteredPlugins(o.PluginDir, o.SkipPlugins,
o.Log) and never filters it per-stage; update the orchestration so that before
executing a stage you filter plugins down to only those matching the stage's
target (use stage.PluginName / stage.PluginNames as appropriate) and pass that
filtered list into the transform execution. Concretely: after obtaining plugins,
derive a per-stage slice (e.g., filtered := filterByPluginName(plugins,
stage.PluginName)) and use that filtered variable wherever the code currently
uses plugins to run transforms (the code paths around plugin.GetFilteredPlugins
and the later transform-running block referenced at lines ~151-153). Ensure the
filtering logic handles empty/blank stage.PluginName (meaning “all plugins”) and
preserves existing skip behavior.
- Around line 166-178: Update the error returns in the inputResources loop to
include the API resource identity and stage info: when runner.Run(resource,
plugins) errors, wrap the error with fmt.Errorf to include the stage name
"transform", the resource kind/name/namespace (e.g. fmt.Sprintf("%s/%s/%s",
resource.Kind, resource.Namespace, resource.Name) or the real accessors on the
resource struct) and the actual type received using %T; do the same for the
jsonpatch.DecodePatch error (include "transform decode patches", resource
kind/name/namespace, and %T for the response or resource) so both failures read
like "transform: failed to run transform for <kind>/<ns>/<name> (type %T): %w"
and "transform: failed to decode patches for <kind>/<ns>/<name> (type %T): %w".
---
Duplicate comments:
In `@cmd/apply/apply.go`:
- Around line 50-68: The Validate() logic incorrectly treats Flags.FinalOnly as
a tri-state without tracking whether the user explicitly set --final-only;
update the flag registration and checks so explicit-set is recorded (e.g. store
a separate boolean like FinalOnlySet or use the flagset's
Changed("final-only")/IsSet API) and change Options.Validate() and
shouldUseKustomizeWorkflow() to consult that explicit-set indicator rather than
the raw Flags.FinalOnly value; specifically, add and set FinalOnlySet when
parsing the flag (or switch to a pointer/Var registration that can distinguish
unset), then replace direct checks of Flags.FinalOnly in Validate() and
shouldUseKustomizeWorkflow() with checks that require FinalOnlySet (or
flagset.Changed) plus the value.
In `@internal/transform/writer.go`:
- Around line 29-48: In KustomizeWriter.WriteStage, when force is false you must
refuse to proceed if the target stage directory already exists and contains
files; before creating resources/ and patches/ check stageDir (from
w.opts.GetStageDir) with os.Stat or os.ReadDir and return a clear error instead
of continuing so later WriteFile calls won't clobber manual edits. Keep the
existing RemoveAll(stageDir) behavior when force is true, and only perform
MkdirAll for resourcesDir/patchesDir after confirming stageDir is absent or
empty.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 876faf2f-4e3b-46f5-bb59-81510a9ee3b6
📒 Files selected for processing (8)
cmd/apply/apply.gocmd/runfn/util.godocs/TRANSFORM_README.mddocs/kustomize-multistage.mdinternal/apply/kustomize.gointernal/transform/orchestrator.gointernal/transform/stages.gointernal/transform/writer.go
✅ Files skipped from review due to trivial changes (1)
- cmd/runfn/util.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/transform/stages.go
Signed-off-by: Marek Aufart <maufart@redhat.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
docs/kustomize-multistage.md (3)
15-33: Add language specifier to code block.The code block showing the directory structure should have a language specifier for better rendering.
♻️ Suggested fix
-``` +```text transform/ ├── 10_kubernetes/🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/kustomize-multistage.md` around lines 15 - 33, Update the fenced code block that renders the directory tree (starting with "transform/" and the ASCII tree lines like "├── 10_kubernetes/") to include a language specifier by replacing the opening triple backticks with ```text so the block becomes ```text, ensuring the directory tree is rendered with the intended fixed-width formatting.
183-188: Add language specifier to code block.♻️ Suggested fix
-``` +```text export/ → 10_kubernetes/ → 20_openshift/ → 30_imagestream/ → output/🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/kustomize-multistage.md` around lines 183 - 188, The fenced code block that shows the stage chain (the block containing "export/ → 10_kubernetes/ → 20_openshift/ → 30_imagestream/ → output/") should include a language specifier to aid highlighting; update the opening fence from ``` to ```text so the block reads as a text code block.
264-285: Add language specifiers to directory structure code blocks.Both the "Old Workflow" and "New Workflow" directory structure blocks should have language specifiers.
♻️ Suggested fix
### Old Workflow -``` +```text transform/ ├── namespace/### New Workflow -``` +```text transform/ └── 10_transform/🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/kustomize-multistage.md` around lines 264 - 285, Update the two directory-structure code fences under "Old Workflow" and "New Workflow" to include a language specifier (e.g., add "text" after the opening triple backticks) so the blocks are rendered consistently; locate the code fences that start with the lines showing "transform/" in the docs/kustomize-multistage.md content (the blocks under the headings "Old Workflow" and "New Workflow") and add the language tag to each opening ``` line.internal/transform/orchestrator.go (1)
229-241: Add timeout to kubectl command execution.
exec.Command("kubectl", "kustomize", stageDir)runs without a timeout. A stuck or slow kubectl process could hang the orchestrator indefinitely. Consider usingexec.CommandContextwith a deadline.♻️ Suggested fix with timeout
+import ( + "time" +) // loadStageOutput loads resources from a completed stage's output func (o *Orchestrator) loadStageOutput(stage Stage) ([]unstructured.Unstructured, error) { opts := file.PathOpts{ TransformDir: o.TransformDir, } stageDir := opts.GetStageDir(stage.DirName) - // Run kubectl kustomize to build the stage with patches applied - cmd := exec.Command("kubectl", "kustomize", stageDir) + // Run kubectl kustomize with timeout to prevent hanging + ctx, cancel := context.WithTimeout(context.Background(), 2*time.Minute) + defer cancel() + cmd := exec.CommandContext(ctx, "kubectl", "kustomize", stageDir)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/transform/orchestrator.go` around lines 229 - 241, The kubectl kustomize invocation uses exec.Command without a timeout and can hang; wrap it with a context deadline (use context.WithTimeout) and swap exec.Command to exec.CommandContext(ctx, "kubectl", "kustomize", stageDir), defer cancel(), then keep using the existing stdout/stderr buffers and cmd.Run(); on error, detect context.DeadlineExceeded and return a clear timeout error (while still including stderr.String()) so the orchestrator won't block indefinitely.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/apply/apply.go`:
- Around line 39-45: The Flags struct's FinalOnly pointer (FinalOnly) is never
bound to the cobra flag so it stays nil; either bind the flag to that field with
BoolVar/BoolVarP (or store the returned *bool from cmd.Flags().Bool) so
o.FinalOnly is set when the user passes --final-only, or remove the pointer and
make FinalOnly a plain bool and rely on cmd.Flags().Changed("final-only") in
shouldUseKustomizeWorkflow() to detect explicit use; update the registration
call that currently calls cmd.Flags().Bool("final-only", ...) accordingly to use
BoolVar (binding to Flags.FinalOnly or its address) or change the type and
adjust any callers.
In `@internal/transform/orchestrator.go`:
- Around line 247-253: The current split logic using strings.Split(output,
"\n---\n") (creating variable docs) misses edge cases like leading/trailing
document separators; replace this ad‑hoc split with a proper YAML multi‑document
decoder (e.g., yaml.v3 Decoder on strings.NewReader(output)) and iterate
Decode() until io.EOF to obtain each document, or alternatively normalize the
input by ensuring separators are surrounded by newlines before splitting; update
the code that creates docs and the for loop over doc to consume decoder results
(or the normalized split) and keep the existing TrimSpace/empty checks.
- Around line 125-145: The code currently calls o.loadStageOutput(*prevStage)
and surfaces opaque kubectl errors if the previous stage's output directory is
missing; add a defensive existence check for the previous stage's output
directory (derived from prevStage.DirName / the orchestrator's output root)
before invoking o.loadStageOutput, and if the directory does not exist return an
explicit error that names prevStage.DirName and instructs the user to run
preceding stages in order or use --stage for single-stage execution; update the
block around GetPreviousStage/stage to perform this check and only call
o.loadStageOutput when the directory exists.
---
Nitpick comments:
In `@docs/kustomize-multistage.md`:
- Around line 15-33: Update the fenced code block that renders the directory
tree (starting with "transform/" and the ASCII tree lines like "├──
10_kubernetes/") to include a language specifier by replacing the opening triple
backticks with ```text so the block becomes ```text, ensuring the directory tree
is rendered with the intended fixed-width formatting.
- Around line 183-188: The fenced code block that shows the stage chain (the
block containing "export/ → 10_kubernetes/ → 20_openshift/ → 30_imagestream/ →
output/") should include a language specifier to aid highlighting; update the
opening fence from ``` to ```text so the block reads as a text code block.
- Around line 264-285: Update the two directory-structure code fences under "Old
Workflow" and "New Workflow" to include a language specifier (e.g., add "text"
after the opening triple backticks) so the blocks are rendered consistently;
locate the code fences that start with the lines showing "transform/" in the
docs/kustomize-multistage.md content (the blocks under the headings "Old
Workflow" and "New Workflow") and add the language tag to each opening ``` line.
In `@internal/transform/orchestrator.go`:
- Around line 229-241: The kubectl kustomize invocation uses exec.Command
without a timeout and can hang; wrap it with a context deadline (use
context.WithTimeout) and swap exec.Command to exec.CommandContext(ctx,
"kubectl", "kustomize", stageDir), defer cancel(), then keep using the existing
stdout/stderr buffers and cmd.Run(); on error, detect context.DeadlineExceeded
and return a clear timeout error (while still including stderr.String()) so the
orchestrator won't block indefinitely.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0bff1e87-d9ec-4f39-b1b7-89f31cf8cc0a
📒 Files selected for processing (4)
cmd/apply/apply.gocmd/transform/transform.godocs/kustomize-multistage.mdinternal/transform/orchestrator.go
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/transform/transform.go
Signed-off-by: Marek Aufart <maufart@redhat.com>
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
cmd/apply/apply.go (1)
137-144:⚠️ Potential issue | 🟠 Major
final-onlyrouting is still inconsistent and keeps default flow on Kustomize.Line 143 (
|| o.FinalOnly) makesshouldUseKustomizeWorkflow()true by default, so legacy flow is never selected. Also, Lines 190-192 always executeApplyFinalStage(), so explicit--final-only=falseis effectively ignored.Suggested fix
func (o *Options) shouldUseKustomizeWorkflow() bool { - // Use Kustomize workflow if any multi-stage flags are explicitly set by the user - // OR if final-only is true (including default case when no flags are provided) - return o.cmd.Flags().Changed("stage") || o.cmd.Flags().Changed("from-stage") || - o.cmd.Flags().Changed("to-stage") || o.cmd.Flags().Changed("stages") || - o.cmd.Flags().Changed("final-only") || o.FinalOnly + if o.cmd == nil { + return false + } + return o.cmd.Flags().Changed("stage") || o.cmd.Flags().Changed("from-stage") || + o.cmd.Flags().Changed("to-stage") || o.cmd.Flags().Changed("stages") || + o.cmd.Flags().Changed("final-only") } @@ - // Default or explicit --final-only: apply final stage - log.Info("Applying final stage...") - return applier.ApplyFinalStage() + // Explicitly disabled final-only means "not final-only": run all stages. + if o.cmd.Flags().Changed("final-only") && !o.FinalOnly { + log.Info("Applying all stages...") + return applier.ApplyMultiStage(internalTransform.StageSelector{}) + } + + log.Info("Applying final stage...") + return applier.ApplyFinalStage() }Also applies to: 176-192
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/apply/apply.go` around lines 137 - 144, The Kustomize routing treats FinalOnly as if it were explicitly requested because shouldUseKustomizeWorkflow() uses o.FinalOnly directly; change that to only treat final-only as set when the flag was explicitly changed (use o.cmd.Flags().Changed("final-only") && o.FinalOnly) so default true doesn’t force Kustomize; similarly, where ApplyFinalStage() is unconditionally called (the block invoking ApplyFinalStage), replace checks that use o.FinalOnly directly with an explicit check that the final-only flag was changed and true (o.cmd.Flags().Changed("final-only") && o.FinalOnly) or derive the decision from shouldUseKustomizeWorkflow() so that --final-only=false actually disables the final-only path.
🧹 Nitpick comments (3)
internal/transform/writer.go (1)
134-153: Consider usingstrings.Indexfor cleaner parsing.The manual loop to find the dot index works but is more verbose than necessary.
♻️ Simpler implementation
func parseTypeKey(typeKey string) (kind, group string) { - // Check if typeKey contains a dot (indicating non-core resource) - dotIndex := -1 - for i, ch := range typeKey { - if ch == '.' { - dotIndex = i - break - } - } - - if dotIndex == -1 { + dotIndex := strings.Index(typeKey, ".") + if dotIndex < 0 { // Core resource - capitalize first letter return capitalizeFirst(typeKey), "" } // Non-core resource - split on first dot kindPart := typeKey[:dotIndex] groupPart := typeKey[dotIndex+1:] return capitalizeFirst(kindPart), groupPart }Note: This requires adding
"strings"to imports if not already present.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/transform/writer.go` around lines 134 - 153, Replace the manual rune loop in parseTypeKey with strings.Index to find the first '.'; use idx := strings.Index(typeKey, ".") and branch on idx == -1 for core resources (return capitalizeFirst(typeKey), "") or split using kindPart := typeKey[:idx] and groupPart := typeKey[idx+1:] for non-core resources, and add "strings" to the imports; keep the existing calls to capitalizeFirst unchanged.internal/transform/orchestrator.go (1)
54-92: Consider extracting common transform loop logic.The transform loop in
RunSingleStage(lines 54-92) andexecuteStage(lines 192-221) are nearly identical. Extracting this into a shared helper would reduce duplication and ensure consistent error handling.♻️ Potential helper signature
// transformResources runs transforms on resources and returns artifacts func (o *Orchestrator) transformResources( stageName string, resources []unstructured.Unstructured, plugins []cranelib.Plugin, pluginName string, ) ([]cranelib.TransformArtifact, error) { runner := cranelib.Runner{ Log: o.Log, PluginPriorities: o.PluginPriorities, OptionalFlags: o.OptionalFlags, } var artifacts []cranelib.TransformArtifact for _, resource := range resources { // ... common transform logic ... } return artifacts, nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/transform/orchestrator.go` around lines 54 - 92, Extract the duplicated transform loop in RunSingleStage and executeStage into a shared method on Orchestrator (suggested name transformResources) that accepts stageName string, resources []unstructured.Unstructured, plugins []cranelib.Plugin and pluginName string and returns ([]cranelib.TransformArtifact, error); move the Runner construction (cranelib.Runner{Log: o.Log, PluginPriorities: o.PluginPriorities, OptionalFlags: o.OptionalFlags}), the loop that calls runner.Run, JSON patch decoding, error wrapping (include stageName, resource identity and type), and construction of cranelib.TransformArtifact (Resource, HaveWhiteOut, Patches, IgnoredOps, Target via cranelib.DeriveTargetFromResource, PluginName) into that helper, then replace the loop bodies in RunSingleStage and executeStage to call o.transformResources(...) and propagate its returned artifacts/error.cmd/apply/apply.go (1)
150-163: Wrap path/IO errors with context for faster debugging.At Lines 152, 157, and 162, raw errors are returned without indicating which input/path failed. Please include
transform-dir/output-dircontext in the error messages.Suggested fix
transformDir, err := filepath.Abs(o.TransformDir) if err != nil { - return err + return fmt.Errorf("failed to resolve --transform-dir %q: %w", o.TransformDir, err) } @@ outputDir, err := filepath.Abs(o.OutputDir) if err != nil { - return err + return fmt.Errorf("failed to resolve --output-dir %q: %w", o.OutputDir, err) } @@ if err := os.MkdirAll(outputDir, 0700); err != nil { - return fmt.Errorf("failed to create output directory: %w", err) + return fmt.Errorf("failed to create output directory %q: %w", outputDir, err) }As per coding guidelines: "Prefer explicit error messages with context in Go code".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/apply/apply.go` around lines 150 - 163, The errors from filepath.Abs and os.MkdirAll lack context; update the calls around transformDir and outputDir (the filepath.Abs calls for o.TransformDir and o.OutputDir and the os.MkdirAll call) to wrap returned errors with contextual messages indicating which path failed (e.g., include "transform-dir: <value>" for transformDir and "output-dir: <value>" for outputDir) using fmt.Errorf("%s: %w", ...) so callers can see which path/operation caused the error.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/apply/apply.go`:
- Around line 125-193: Tests are missing for the new stage-selection
routing/validation introduced around shouldUseKustomizeWorkflow and
runKustomizeWorkflow; add unit tests that exercise: (1) default routing when no
flags are set (assert shouldUseKustomizeWorkflow() returns true and
runKustomizeWorkflow invokes ApplyFinalStage), (2) selector exclusivity and
multi-flag selection (construct a StageSelector with Stage, FromStage, ToStage,
Stages and assert ApplyMultiStage is called with the expected selector), and (3)
explicit --final-only=false behavior (simulate the flag state where FinalOnly is
false and the "final-only" flag is explicitly set to false and assert
multi-stage vs final-stage routing). Use table-driven tests, mock or fake
apply.KustomizeApplier to observe calls to ApplyMultiStage and ApplyFinalStage,
and exercise shouldUseKustomizeWorkflow, runKustomizeWorkflow, and StageSelector
construction to cover validation/regression paths.
In `@internal/transform/orchestrator.go`:
- Around line 245-256: The kubectl kustomize invocation currently uses
exec.Command and cmd.Run() with no timeout; wrap the call in a context with
timeout (e.g., context.WithTimeout, configurable or a sensible default like
30s), use exec.CommandContext(ctx, "kubectl", "kustomize", stageDir), defer
cancel(), and handle ctx.Err() / context.DeadlineExceeded when returning the
error (include stderr.String() as before). Update the block that builds the
stage (where cmd := exec.Command("kubectl", "kustomize", stageDir),
stdout/stderr buffers, and the cmd.Run() call) to use the context-based command
and proper timeout handling, keeping the existing debug log and error message
(including stage.DirName and stderr).
In `@internal/transform/writer.go`:
- Around line 78-85: The patch serialization and write error messages lack the
resource identity; update the error handling around
kustomize.SerializePatchToYAML(artifact.Patches) and the os.WriteFile(patchPath,
...) call to include the artifact's resource identity (e.g., artifact.Name,
artifact.Namespace, artifact.Kind or artifact.ID) along with patchPath and the
wrapped error. Replace the current fmt.Errorf("failed to serialize patch: %w",
err) and fmt.Errorf("failed to write patch file: %w", err) with messages that
embed these identifying fields (for example: fmt.Errorf("failed to serialize
patch for %s/%s (%s): %w", artifact.Namespace, artifact.Name, artifact.Kind,
err)) so any failure clearly states which resource and file failed.
---
Duplicate comments:
In `@cmd/apply/apply.go`:
- Around line 137-144: The Kustomize routing treats FinalOnly as if it were
explicitly requested because shouldUseKustomizeWorkflow() uses o.FinalOnly
directly; change that to only treat final-only as set when the flag was
explicitly changed (use o.cmd.Flags().Changed("final-only") && o.FinalOnly) so
default true doesn’t force Kustomize; similarly, where ApplyFinalStage() is
unconditionally called (the block invoking ApplyFinalStage), replace checks that
use o.FinalOnly directly with an explicit check that the final-only flag was
changed and true (o.cmd.Flags().Changed("final-only") && o.FinalOnly) or derive
the decision from shouldUseKustomizeWorkflow() so that --final-only=false
actually disables the final-only path.
---
Nitpick comments:
In `@cmd/apply/apply.go`:
- Around line 150-163: The errors from filepath.Abs and os.MkdirAll lack
context; update the calls around transformDir and outputDir (the filepath.Abs
calls for o.TransformDir and o.OutputDir and the os.MkdirAll call) to wrap
returned errors with contextual messages indicating which path failed (e.g.,
include "transform-dir: <value>" for transformDir and "output-dir: <value>" for
outputDir) using fmt.Errorf("%s: %w", ...) so callers can see which
path/operation caused the error.
In `@internal/transform/orchestrator.go`:
- Around line 54-92: Extract the duplicated transform loop in RunSingleStage and
executeStage into a shared method on Orchestrator (suggested name
transformResources) that accepts stageName string, resources
[]unstructured.Unstructured, plugins []cranelib.Plugin and pluginName string and
returns ([]cranelib.TransformArtifact, error); move the Runner construction
(cranelib.Runner{Log: o.Log, PluginPriorities: o.PluginPriorities,
OptionalFlags: o.OptionalFlags}), the loop that calls runner.Run, JSON patch
decoding, error wrapping (include stageName, resource identity and type), and
construction of cranelib.TransformArtifact (Resource, HaveWhiteOut, Patches,
IgnoredOps, Target via cranelib.DeriveTargetFromResource, PluginName) into that
helper, then replace the loop bodies in RunSingleStage and executeStage to call
o.transformResources(...) and propagate its returned artifacts/error.
In `@internal/transform/writer.go`:
- Around line 134-153: Replace the manual rune loop in parseTypeKey with
strings.Index to find the first '.'; use idx := strings.Index(typeKey, ".") and
branch on idx == -1 for core resources (return capitalizeFirst(typeKey), "") or
split using kindPart := typeKey[:idx] and groupPart := typeKey[idx+1:] for
non-core resources, and add "strings" to the imports; keep the existing calls to
capitalizeFirst 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3e323c62-5c4a-482a-a34e-7e9ac5da3aec
📒 Files selected for processing (4)
cmd/apply/apply.gogo.modinternal/transform/orchestrator.gointernal/transform/writer.go
🚧 Files skipped from review as they are similar to previous changes (1)
- go.mod
Signed-off-by: Marek Aufart <maufart@redhat.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
cmd/apply/apply.go (1)
128-131: CreateoutputDironly when something is actually written.
ApplyMultiStage()andApplyFinalStage()already callos.MkdirAll(k.OutputDir, 0700)before writing. Doing it here means a failed stage discovery/build still leaves an empty output directory behind. Letting the applier create it lazily keeps failed runs side-effect free.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/apply/apply.go` around lines 128 - 131, The current unconditional call to os.MkdirAll(outputDir, 0700) creates an empty output directory even when the run fails; remove or defer that call in Apply (the block in apply.go that uses outputDir) so the directory is only created lazily by the applier when files are actually written (as already done by ApplyMultiStage() and ApplyFinalStage()); specifically, delete the os.MkdirAll(outputDir, 0700) invocation here (or replace it with a no-op) and rely on the existing MkdirAll calls in ApplyMultiStage() / ApplyFinalStage() to create k.OutputDir only when writing occurs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/apply/apply.go`:
- Around line 140-143: The branch that decides whether to call ApplyMultiStage()
vs ApplyFinalStage() incorrectly checks cobra flag change state via
o.cmd.Flags().Changed(...) instead of using the merged, config-backed option
values loaded into o.Flags in PreRun; update the conditional to inspect the
actual stage selector values on o.Flags (e.g., o.Flags.Stage, o.Flags.FromStage,
o.Flags.ToStage, o.Flags.Stages or whichever fields represent the selectors) and
call ApplyMultiStage() when any of those merged values are set, otherwise call
ApplyFinalStage(); after switching to value-based routing you can remove the
extra Options.cmd plumbing mentioned in the comment.
---
Nitpick comments:
In `@cmd/apply/apply.go`:
- Around line 128-131: The current unconditional call to os.MkdirAll(outputDir,
0700) creates an empty output directory even when the run fails; remove or defer
that call in Apply (the block in apply.go that uses outputDir) so the directory
is only created lazily by the applier when files are actually written (as
already done by ApplyMultiStage() and ApplyFinalStage()); specifically, delete
the os.MkdirAll(outputDir, 0700) invocation here (or replace it with a no-op)
and rely on the existing MkdirAll calls in ApplyMultiStage() / ApplyFinalStage()
to create k.OutputDir only when writing occurs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Signed-off-by: Marek Aufart <maufart@redhat.com>
Signed-off-by: Marek Aufart <maufart@redhat.com>
Signed-off-by: Marek Aufart <maufart@redhat.com>
Signed-off-by: Marek Aufart <maufart@redhat.com>
Signed-off-by: Marek Aufart <maufart@redhat.com>
Signed-off-by: Marek Aufart <maufart@redhat.com>
aufi
commented
Apr 8, 2026
| @@ -0,0 +1,373 @@ | |||
| # Crane Transform Directory Structure | |||
Contributor
Author
There was a problem hiding this comment.
Docs files structure will be updated/unified with #215
Signed-off-by: Marek Aufart <maufart@redhat.com>
Contributor
msajidmansoori12
left a comment
msajidmansoori12
left a comment
There was a problem hiding this comment.
@aufi Hi I noticed that in output dir manifests , the yaml indentation contains 4 spaces while in general the manifests we get from kubectl contains 2 spaces. Just a formatting difference not something that needs to be fixed right away just wanted to make a note of it.
apiVersion: apps/v1
kind: Deployment
metadata:
annotations:
deployment.kubernetes.io/revision: "1"
labels:
name: simple-nginx-nopv
name: simple-nginx-nopv-deployment
namespace: simple-nginxx-nopv
spec:
progressDeadlineSeconds: 600
replicas: 1
revisionHistoryLimit: 10
selector:
matchLabels:
app: simple-nginx-nopv
strategy:
rollingUpdate:
maxSurge: 25%
maxUnavailable: 25%
type: RollingUpdate
template:
orchestration unit test Signed-off-by: Marek Aufart <maufart@redhat.com>
Signed-off-by: Marek Aufart <maufart@redhat.com>
Signed-off-by: Marek Aufart <maufart@redhat.com>
Signed-off-by: Marek Aufart <maufart@redhat.com>
nachandr
reviewed
Apr 9, 2026
| // and returns them sorted by priority (numeric prefix) | ||
| func DiscoverStages(transformDir string) ([]Stage, error) { | ||
| // Pattern: <number>_<pluginName> | ||
| pattern := regexp.MustCompile(`^([0-9]+)_([a-zA-Z0-9_-]+)$`) |
Contributor
There was a problem hiding this comment.
The current stage name validation regex only allows [a-zA-Z0-9_-]+ for the plugin portion, which excludes dots (.).
This prevents valid Kubernetes-style identifiers like route.openshift.io from being used in stage names (e.g., 30_route.openshift.io would fail validation).
Considering that many Kubernetes API groups and resources use dotted naming conventions, it would be beneficial to support . in plugin names.
Contributor
Author
There was a problem hiding this comment.
Thanks for comment. Existing plugin names usually describe their use-case (typically with CamelCase describing technology or something - https://github.com/migtools/crane-plugins/tree/main/plugins, so in this case maybe RouteToGatewayPlugin would work too).
I'd keep the regex validation without ., just might document it in some plugin development doc.
msajidmansoori12
approved these changes
Apr 10, 2026
stillalearner
approved these changes
Apr 10, 2026
Contributor
stillalearner
left a comment
stillalearner
left a comment
There was a problem hiding this comment.
lgtm. Thanks @aufi , great PR
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
There is still some work on polishing this feature to make it really helpful and stable (mostly in follow-ups), but its basic version should work and PR is ready for review now.
Sorry for bigger amount of changes in this PR, but I already put some additional sub-features away.
Related to #188
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests
Chores