Sync/upstream main 20260604#5
Merged
Merged
Conversation
…ion. (migtools#378) * First draft for building crane binaries Signed-off-by: M Sajid Mansoori <mmansoor@redhat.com> * Temporary testing on fork Signed-off-by: M Sajid Mansoori <mmansoor@redhat.com> * Update workflow to not include checksum files as they are present in zip files already Signed-off-by: M Sajid Mansoori <mmansoor@redhat.com> * Add readme download section Signed-off-by: M Sajid Mansoori <mmansoor@redhat.com> * Rename file and update readme Signed-off-by: M Sajid Mansoori <mmansoor@redhat.com> * Finalize changes Signed-off-by: M Sajid Mansoori <mmansoor@redhat.com> --------- Signed-off-by: M Sajid Mansoori <mmansoor@redhat.com>
…s#383) * fix: Add tests for cluster-scoped resources in transform * fix comments * fixed comments
* embed kustomize to crane * fix resource order * fix coderabbit comments * Removed --load-restrictor and --enable-alpha-plugins flags and some other kubectl kustomize references
* Add --skip-cluster-scoped flag to apply * fix comments * fix coderabbit comments * fix coderabbit comment
* Add --config-file CLI flag to accept transform stage dir Signed-off-by: M Sajid Mansoori <mmansoor@redhat.com> * Add functions to load and validate config files Signed-off-by: M Sajid Mansoori <mmansoor@redhat.com> * Add precheck to make sure --config-file and --stage are not used at once Signed-off-by: M Sajid Mansoori <mmansoor@redhat.com> * Wire new stages creation from config file Signed-off-by: M Sajid Mansoori <mmansoor@redhat.com> * Add basic unit tests Signed-off-by: M Sajid Mansoori <mmansoor@redhat.com> * Add more edge cases unit tests Signed-off-by: M Sajid Mansoori <mmansoor@redhat.com> * Add a method to reconcile existing transform stages Signed-off-by: M Sajid Mansoori <mmansoor@redhat.com> * Update function comments Signed-off-by: M Sajid Mansoori <mmansoor@redhat.com> * Add more tests and make the schema check for transform file stricter Signed-off-by: M Sajid Mansoori <mmansoor@redhat.com> * Update config tests to table driven tests Signed-off-by: M Sajid Mansoori <mmansoor@redhat.com> * Add unit test to cover scenario of --stage and --config-file both being provided Signed-off-by: M Sajid Mansoori <mmansoor@redhat.com> * include wrapped error for failing to unmarshal config file Signed-off-by: M Sajid Mansoori <mmansoor@redhat.com> * Hardening to not accept multi doc yaml Signed-off-by: M Sajid Mansoori <mmansoor@redhat.com> * nit addressed Signed-off-by: M Sajid Mansoori <mmansoor@redhat.com> * Add cleanup for workdir with --force option Signed-off-by: M Sajid Mansoori <mmansoor@redhat.com> * Rename functions to match naming for instruction file Signed-off-by: M Sajid Mansoori <mmansoor@redhat.com> * Update rest of the code to use --instructions-file Signed-off-by: M Sajid Mansoori <mmansoor@redhat.com> --------- Signed-off-by: M Sajid Mansoori <mmansoor@redhat.com>
* Add validate command to framework Signed-off-by: M Sajid Mansoori <mmansoor@redhat.com> * Add validatedir to scenario paths Signed-off-by: M Sajid Mansoori <mmansoor@redhat.com> * Automate validate alternative GV suggestion test Signed-off-by: M Sajid Mansoori <mmansoor@redhat.com> * Re Signed-off-by: M Sajid Mansoori <mmansoor@redhat.com> --------- Signed-off-by: M Sajid Mansoori <mmansoor@redhat.com>
* Added kustomize version * fix coderabbit comments * Fix kustomize CLI version
…tion (migtools#407) * Update e2e test for CronJob PVC migration: Comment out PVC data integrity checks due to flaky behavior on Linux. Investigation planned for root cause analysis post-release. Signed-off-by: midays <midays@redhat.com> * Update e2e test for CronJob PVC migration: Add reference to BUG migtools#408 in test description and improve comments regarding PVC data integrity checks. Investigation into flaky behavior on Linux continues. Signed-off-by: midays <midays@redhat.com> --------- Signed-off-by: midays <midays@redhat.com>
…ile (migtools#433) * Add function to run transform with instructions file Signed-off-by: M Sajid Mansoori <mmansoor@redhat.com> * Add instructions-file basic tier0 test Signed-off-by: M Sajid Mansoori <mmansoor@redhat.com> --------- Signed-off-by: M Sajid Mansoori <mmansoor@redhat.com>
* Transform stages for all plugins by default Default execution of `crane transform` creates stages for all available plugins (instead of just default KubernetePlugin). This could be optionally restricted with --skip-plugins arg. Related to migtools#370 (Task 1) Signed-off-by: Marek Aufart <maufart@redhat.com> * Update Plugin name path sanitization Signed-off-by: Marek Aufart <maufart@redhat.com> * Update plugin&stage names validation Signed-off-by: Marek Aufart <maufart@redhat.com> * Update relpath stage safe check Signed-off-by: Marek Aufart <maufart@redhat.com> * Implement positional arguments for multi-stage selection (Issue migtools#410) Changes: - Replace --stage flag with positional arguments for transform and apply commands - Add support for multiple stages: crane transform stage1 stage2 - Support plugin name resolution: crane transform KubernetesPlugin - Implement duplicate plugin detection with helpful error messages - Update StageSelector to use []string instead of single string - Add resolveAndValidateStages() with plugin name → stage dir resolution Usage: crane transform [stage...] crane apply [stage...] Examples: crane transform # all stages crane transform 10_KubernetesPlugin # one stage by dir name crane transform KubernetesPlugin # one stage by plugin name crane transform stage1 stage2 # multiple stages crane apply 10_KubernetesPlugin OpenshiftPlugin Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> * Update tests for positional arguments - Update StageSelector usage from Stage string to Stages []string - Add tests for multiple stage selection - Add tests for plugin name matching - Update apply tests to use RequestedStages instead of Flags.Stage - All tests passing Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> * Fix stage plugin order counting Signed-off-by: Marek Aufart <maufart@redhat.com> * Fix E2E tests with stage as positional arg Signed-off-by: Marek Aufart <maufart@redhat.com> * Fix custom stage creation without plugin Signed-off-by: Marek Aufart <maufart@redhat.com> * Fix previous stage output Signed-off-by: Marek Aufart <maufart@redhat.com> * Refactoring & deduplication Signed-off-by: Marek Aufart <maufart@redhat.com> * Add automatic custom stage order lookup Signed-off-by: Marek Aufart <maufart@redhat.com> * Refactoring and cleanup Signed-off-by: Marek Aufart <maufart@redhat.com> * Add requested stages validation to apply Signed-off-by: Marek Aufart <maufart@redhat.com> * Update go mod Signed-off-by: Marek Aufart <maufart@redhat.com> * Unify find Stages dir function Signed-off-by: Marek Aufart <maufart@redhat.com> --------- Signed-off-by: Marek Aufart <maufart@redhat.com> Co-authored-by: Claude Sonnet 4.5 <noreply@anthropic.com>
* Add polarion case id for tier0 test Signed-off-by: M Sajid Mansoori <mmansoor@redhat.com> * Add test id for validate alternative GV testcase Signed-off-by: M Sajid Mansoori <mmansoor@redhat.com> * Add tier1 case for force reconcile migration Signed-off-by: M Sajid Mansoori <mmansoor@redhat.com> * Add testcase id from polarion Signed-off-by: M Sajid Mansoori <mmansoor@redhat.com> * Add check for stale .work/99_OrphanStage deletion post force reconcile Signed-off-by: M Sajid Mansoori <mmansoor@redhat.com> --------- Signed-off-by: M Sajid Mansoori <mmansoor@redhat.com>
The previous path construction using filepath.Join(config.CraneBin, "..", "..", "scripts", "capture-api-surface.sh") was incorrect because: - config.CraneBin is the path to the crane binary itself, not a directory - Going up two levels from the binary didn't reach the repository root Fixed by: - Using filepath.Dir() to get the directory of the crane binary - Going up one more level to reach the repository root - Then joining with "scripts" and the script name This ensures the script is found at the correct path during test execution. "
* Add autocompletion for transform plugins Signed-off-by: Marek Aufart <maufart@redhat.com> * Raise error on skipPlugins Signed-off-by: Marek Aufart <maufart@redhat.com> * Add full transform completion test Signed-off-by: Marek Aufart <maufart@redhat.com> --------- Signed-off-by: Marek Aufart <maufart@redhat.com>
* Add test for crane validate alternate GV in offline mode Signed-off-by: M Sajid Mansoori <mmansoor@redhat.com> * Add doc comment and simple unit test Signed-off-by: M Sajid Mansoori <mmansoor@redhat.com> --------- Signed-off-by: M Sajid Mansoori <mmansoor@redhat.com>
📝 WalkthroughWalkthroughThis PR refactors crane's transform and apply commands from single-stage ChangesCore multi-stage and instructions orchestration
Embedded kustomize runner and orchestration
Default stage generation from plugins
Test framework, utilities, and CLI enhancements
End-to-end tests for new features
Build/release and dependency management
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related issues
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
Test Coverage ReportTotal: 46.5% Per-package coverage
Full function-level detailsPosted by CI |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
internal/transform/writer.go (1)
216-226:⚠️ Potential issue | 🟠 Major | ⚡ Quick winInclude API group/version in the resource identity key.
WriteStage()usesgetResourceID()as the dedupe key for resources and whiteouts. With the currentkind/namespace/nameformat, two resources that share a kind/name but come from different API groups or versions will overwrite each other in those maps, so one manifest can disappear from the generated stage output.Suggested direction
func getResourceID(resource unstructured.Unstructured) string { + gvk := resource.GroupVersionKind() kind := resource.GetKind() namespace := resource.GetNamespace() name := resource.GetName() if namespace != "" { - return fmt.Sprintf("%s/%s/%s", kind, namespace, name) + return fmt.Sprintf("%s/%s/%s/%s/%s", gvk.Group, gvk.Version, kind, namespace, name) } - return fmt.Sprintf("%s/%s", kind, name) + return fmt.Sprintf("%s/%s/%s/%s", gvk.Group, gvk.Version, kind, name) }🤖 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 `@internal/transform/writer.go` around lines 216 - 226, getResourceID currently returns kind/namespace/name which omits API group/version and causes cross-group/version collisions; update getResourceID to include the resource API version (Group/Version) by using resource.GetAPIVersion() (or resource.GroupVersionKind() to build group/version) and return a key like apiVersion/kind/namespace/name (or apiVersion/kind/name for cluster-scoped). Modify the function getResourceID to prepend the apiVersion component to the existing format so all dedupe maps in WriteStage() distinguish resources by API group/version as well as kind/namespace/name.internal/kustomize/args.go (1)
64-67:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winValidator accepts
--enable-helm=<value>but the runner rejects it.
--enable-helmis a boolean flag, yet the whitelist check only comparesargName(the part before=). An input like--enable-helm=truepasses validation and is forwarded as a single token, butRunner.buildOptionsmatches only the exact string--enable-helm(and the--helm-command=prefix), so it falls through to thedefaultbranch and returnsunsupported kustomize argument: "--enable-helm=true". The resulting error is confusing because the flag is on the whitelist.Consider rejecting a value on boolean flags here so the error surfaces during validation with a clear message.
🤖 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 `@internal/kustomize/args.go` around lines 64 - 67, The validator currently only checks argName against AllowedKustomizeArgs and accepts tokens like "--enable-helm=true" even though Runner.buildOptions expects the bare boolean flag "--enable-helm"; update the validation in the arg handling block (the code that uses argName and AllowedKustomizeArgs) to detect tokens containing '=' for known boolean flags and reject them with a clear error, e.g. if argName is a boolean-typed allowed key (like "--enable-helm") and the original token contains '=', return an error asking to use the flag without a value; reference the argName variable and AllowedKustomizeArgs map when implementing this check and ensure Runner.buildOptions remains unchanged.
🧹 Nitpick comments (9)
README.md (1)
52-53: ⚡ Quick winGitHub Actions artifact links require authentication and expire after 30 days.
The
mainandNightlydownload links point to GitHub Actions workflow runs. Users must be authenticated to GitHub to download these artifacts, and the workflow setsretention-days: 30(line 70 in the workflow file), so artifacts expire monthly. This differs significantly from theStablerelease experience.Consider adding a note to set user expectations or providing alternative hosting for unsigned development builds.
📝 Suggested clarification
| Channel | Purpose | Link | | :-- | :-- | :-- | | **✅ Stable** | Tagged releases for production use | [GitHub Releases](https://github.com/migtools/crane/releases) | -| **🛠️ `main`** | Latest artifacts from pushes to `main` | [Build Crane Binaries (main push)](https://github.com/migtools/crane/actions/workflows/build-release-binaries.yml?query=branch%3Amain+event%3Apush) | -| **🌙 Nightly** | Scheduled artifacts for early validation | [Build Crane Binaries (scheduled)](https://github.com/migtools/crane/actions/workflows/build-release-binaries.yml?query=event%3Aschedule) | +| **🛠️ `main`** | Latest artifacts from pushes to `main` (requires GitHub login; 30-day retention) | [Build Crane Binaries (main push)](https://github.com/migtools/crane/actions/workflows/build-release-binaries.yml?query=branch%3Amain+event%3Apush) | +| **🌙 Nightly** | Scheduled artifacts for early validation (requires GitHub login; 30-day retention) | [Build Crane Binaries (scheduled)](https://github.com/migtools/crane/actions/workflows/build-release-binaries.yml?query=event%3Aschedule) |🤖 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 `@README.md` around lines 52 - 53, The README's table entries for the "main" and "Nightly" artifacts link to GitHub Actions workflow runs but don't warn readers that those artifacts require GitHub authentication and expire after 30 days (the workflows set retention-days: 30); update the README near the **🛠️ `main`** and **🌙 Nightly** rows to add a short note that these links point to authenticated, time-limited GitHub Actions artifacts (expires 30 days) and either suggest users sign into GitHub to download or provide/mention an alternate longer-term hosting option for unsigned development builds..github/workflows/build-release-binaries.yml (2)
57-58: ⚡ Quick winMissing version and commit metadata in binaries.
The build step does not inject version or commit information via ldflags, unlike the Dockerfile build which sets
MTA_OPS_VERSIONandMTA_OPS_GIT_COMMIT. Users of these binaries won't be able to identify which version or commit they're running.📦 Proposed fix to add version metadata
binary="crane_${GOOS}_${GOARCH}${ext}" - go build -trimpath -o "dist/${binary}" . + VERSION="${{ github.ref_name }}" + if [ "${{ github.ref_type }}" != "tag" ]; then + VERSION="${{ github.sha }}" + fi + go build -trimpath \ + -ldflags="-X github.com/konveyor/crane/internal/buildinfo.Version=${VERSION} -X github.com/konveyor/crane/internal/buildinfo.BuildCommit=${{ github.sha }}" \ + -o "dist/${binary}" .Based on learnings: konflux.Dockerfile (lines 20-35) demonstrates the repository convention of embedding
MTA_OPS_VERSIONandMTA_OPS_GIT_COMMITvia ldflags during multi-platform builds.🤖 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 @.github/workflows/build-release-binaries.yml around lines 57 - 58, The go build step that produces "dist/${binary}" currently omits embedding version and commit metadata; update the build invocation (the go build line that outputs to dist/${binary}) to pass ldflags that set the MTA_OPS_VERSION and MTA_OPS_GIT_COMMIT variables (using the pipeline's version and commit values, e.g. from a VERSION input or GITHUB_SHA) so the compiled binary contains those symbols; ensure the workflow exposes/sets the version and commit env vars before the build and uses them in the ldflags passed to go build.
59-62: ⚡ Quick winInefficient checksum file handling creates then deletes individual checksums.
The build step creates individual
.sha256files (line 61), the release job then recreates a combinedchecksums.txt(lines 88-97), and finally deletes the individual.sha256files (line 100). This wastes CI time and complicates the artifact flow.♻️ Proposed fix to skip individual checksum files
Remove individual checksum creation from the build step:
binary="crane_${GOOS}_${GOARCH}${ext}" go build -trimpath -o "dist/${binary}" . - ( - cd dist - sha256sum "${binary}" > "${binary}.sha256" - )Then remove the cleanup step from the release job since there are no
.sha256files to delete:- - name: Keep only binaries and combined checksums file - run: rm -f release-assets/*.sha256 - - name: Create draft GitHub releaseAlso applies to: 99-100
🤖 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 @.github/workflows/build-release-binaries.yml around lines 59 - 62, Remove the per-binary checksum creation to avoid redundant work: delete the sha256sum "${binary}" > "${binary}.sha256" invocation in the build step (the subshell that cd's into dist and emits "${binary}.sha256"), and also remove the cleanup step in the release job that deletes individual .sha256 files (the rm -f dist/*.sha256 or similar) since the release already generates a combined checksums.txt; ensure only the combined checksums generation remains.internal/buildinfo/buildinfo_test.go (1)
8-57: ⚡ Quick winConvert these cases to a table-driven test.
These three tests exercise one function across the same setup/teardown pattern. Folding them into a single table-driven test will remove the repeated
readBuildInfooverride logic and matches the repo’s test convention for multi-scenario coverage. As per coding guidelines, "Use table-driven tests for multiple scenarios".🤖 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 `@internal/buildinfo/buildinfo_test.go` around lines 8 - 57, Replace the three separate tests with a single table-driven test that iterates scenarios and sets readBuildInfo per case; save the original readBuildInfo once into a variable (original := readBuildInfo) and defer restoring it (defer func() { readBuildInfo = original }()), then define a slice of test cases with fields name, buildInfoReturn (a func returning *debug.BuildInfo, bool) and expected string, loop over cases using t.Run(tc.name, func(t *testing.T) { readBuildInfo = tc.buildInfoReturn(); got := readKustomizeVersion(); if got != tc.expected { t.Errorf(...) } }), and include the same three scenarios (build info unavailable, dep not found, dep found with "sigs.k8s.io/kustomize/kustomize/v5" => "v5.8.1") to remove duplicated setup/teardown while keeping the original assertions.cmd/transform/listplugins/listplugins.go (1)
95-101: ⚡ Quick winWrap plugin discovery errors with the directory being processed.
These raw returns drop the failing
pluginDir, which makeslist-pluginsand completion failures much harder to debug once the path has already been normalized.Proposed change
func getFilteredPlugins(pluginDir string, skipPlugins []string, log *logrus.Logger) ([]cranelib.Plugin, error) { absPluginDir, err := filepath.Abs(pluginDir) if err != nil { - return nil, err + return nil, fmt.Errorf("resolve plugin-dir %q to absolute path: %w", pluginDir, err) } - return plugin.GetFilteredPlugins(absPluginDir, skipPlugins, log) + plugins, err := plugin.GetFilteredPlugins(absPluginDir, skipPlugins, log) + if err != nil { + return nil, fmt.Errorf("load plugins from %q: %w", absPluginDir, err) + } + return plugins, nil }As per coding guidelines, "Prefer explicit error messages with context in Go code".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/transform/listplugins/listplugins.go` around lines 95 - 101, The getFilteredPlugins function currently returns raw errors from filepath.Abs and plugin.GetFilteredPlugins without context; wrap both errors to include the absolute plugin directory being processed (absPluginDir) so callers see which path failed. Specifically, update getFilteredPlugins to return fmt.Errorf or errors.Wrap-style messages that include absPluginDir when filepath.Abs fails and when plugin.GetFilteredPlugins returns an error (referencing getFilteredPlugins and plugin.GetFilteredPlugins to locate the spots), preserving the original error as the cause.cmd/transform/completion_test.go (1)
141-143: ⚡ Quick winRemove dead logger initialization in cmd/transform/completion_test.go
log := logrus.New()is only used to calllog.SetOutput(os.Stderr)and the resultinglogis never referenced afterward; remove those lines and drop thelogrusimport if it becomes unused.Proposed change
- log := logrus.New() - log.SetOutput(os.Stderr) - completionFunc := getPluginCompletions(f)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/transform/completion_test.go` around lines 141 - 143, Remove the dead logger creation: delete the local variable assignment "log := logrus.New()" and the subsequent "log.SetOutput(os.Stderr)" since the "log" variable is never used; also remove the "logrus" import if it becomes unused after this change so the test compiles cleanly, leaving only "f := &flags.GlobalFlags{}" as before.cmd/apply/apply.go (1)
154-194: 💤 Low valueStage-matching logic is duplicated with
FilterStages.The resolution loop here re-implements the exact
stage.DirName == requested || stage.PluginName == requestedmatching thatinternalTransform.FilterStagesalready performs (seeinternal/transform/stages.go:74-109), then handsRequestedStagesback to the selector soFilterStagesmatches a second time insideApplyMultiStage. Consider exposing a small helper (e.g., aResolveStages/validation function) in the transform package and reusing it, so the matching contract lives in one place and can't drift between apply and the orchestrator.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/apply/apply.go` around lines 154 - 194, The code duplicates stage-matching logic already implemented in internalTransform.FilterStages when validating o.RequestedStages before building the StageSelector; refactor to avoid drift by adding and using a single resolver/validator in the transform package (e.g., expose internalTransform.ResolveStages or ValidateRequestedStages) that takes existingStages (from internalTransform.DiscoverStages) and the slice o.RequestedStages and returns the resolved list and any unresolved names (or an error), then replace the inline loop in the apply logic to call that new helper and build the StageSelector using the resolved stage names (keep StageSelector and ApplyMultiStage behavior unchanged so FilterStages still receives canonical input).internal/kustomize/runner_test.go (1)
250-325: 💤 Low valueConsider adding a
Runner.Buildtest that exercises the--envrestore path.
TestBuildOptions_ArgMappingvalidates parsing but never runs a build with--env, sosetEnvVars/restoreEnv(the global-state mutation discussed inrunner.go) is untested. A small test asserting that a pre-existing env var is restored (and a previously-unset key is unset) afterBuildwould lock in that behavior.🤖 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 `@internal/kustomize/runner_test.go` around lines 250 - 325, Add a new unit test that calls Runner.Build with the --env flag to exercise setEnvVars/restoreEnv: create a Runner with Args containing "--env" and an env pair (e.g., "FOO=bar"), set up a pre-existing environment key and an unset key, run runner.Build(ctx) and then assert (1) the pre-existing key is restored to its original value after Build and (2) the previously-unset key is unset again; use the existing TestBuildOptions_ArgMapping patterns and reference Runner.Build, Runner.buildOptions, setEnvVars, and restoreEnv to locate the implementation to test.cmd/apply/apply_test.go (1)
90-114: 💤 Low value
TestStageSelectionRoutingre-implements the routing instead of asserting onrun().The test recomputes
hasCustomSelector := len(tt.requestedStages) > 0and compares lengths/indices locally; it never invokes the actualrun()/selector construction. It will keep passing even if the real routing inrun()regresses. SinceTestRun_UnresolvedStagesErroralready drives the real path, consider folding selector-shape assertions into that flow (or extracting the selector-building into a testable helper) to make this test meaningful.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/apply/apply_test.go` around lines 90 - 114, TestStageSelectionRouting currently re-implements the selector logic instead of exercising the real code path; change the test to call the actual run() path (which uses ApplyMultiStage()) or refactor the selector construction into a new helper (e.g., BuildStageSelector or ConstructSelector) that run() calls, then assert the returned selector shape against tt.selectorStages; ensure TestRun_UnresolvedStagesError is either extended to include these selector-shape assertions or the new helper is unit-tested directly so the test no longer compares len(tt.requestedStages) > 0 locally but validates the selector produced by run() or the extracted helper.
🤖 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 @.github/workflows/build-release-binaries.yml:
- Line 37: Replace the mutable GitHub Actions tags with the provided commit SHAs
to avoid CI supply-chain risks: update uses: actions/checkout@v4 to uses:
actions/checkout@34e114876b0b11c390a56381ad16ebd13914f8d5, actions/setup-go@v6
to actions/setup-go@4a3601121dd01d1626a1e23e37211e3254c1c06c,
actions/upload-artifact@v4 to
actions/upload-artifact@ea165f8d65b6e75b540449e92b4886f43607fa02,
actions/download-artifact@v4 to
actions/download-artifact@d3f86a106a0bac45b974a628896c90dbdf5c8093, and
softprops/action-gh-release@v2 to
softprops/action-gh-release@3bb12739c298aeb8a4eeaf626c5b8d85266b0e65 in the
workflow file so each uses: entry references the exact commit SHA.
In `@cmd/transform/transform_test.go`:
- Around line 760-791: The test currently accepts any error from
o.resolveAndValidateStages, so change the assertions to require that err is
non-nil and specifically indicates the previous-stage-output failure: assert err
!= nil and assert the error message contains either "10_KubernetesPlugin" or the
words "previous" or "output" (or use errors.Is with a sentinel like
ErrPreviousStageNoOutput if one exists). Update the failure branch to call
t.Fatalf or t.Errorf if the error does not include those tokens so the test only
passes when ensurePreviousStagesRun reports the expected previous-stage output
error for the "50_CustomModifications" -> "10_KubernetesPlugin" scenario.
In `@cmd/transform/transform.go`:
- Around line 449-483: The default plugin stage priority spacing is
inconsistent: in the loop that creates default stages (using GenerateStageName,
createStageDirectory, and the local variable priority) it currently increments
priority by 5 which conflicts with GenerateStageDirNames() and ad-hoc creation
that use 10-step spacing; update the loop to advance priority by 10 (leave the
initial priority = 10) and adjust the comment to reflect 10-step increments so
generated stage names match other paths and reconcileInstructionStages() won’t
treat them as extras.
In `@e2e-tests/tests/mta_813_cronJob_PVC_test.go`:
- Around line 225-276: The test currently disables the PVC data-integrity
assertion so it no longer verifies transfer-pvc preserved source data; either
mark the whole spec pending/skip or restore a non-flaky check: re-enable the
pvc-reader flow (creating pod "pvc-reader" via scenario.KubectlTgtNonAdmin.Run
using pvcName/namespace), but make the assertion robust by waiting for the pod
to reach a terminal phase with a longer Eventually timeout and retrying logs
until they contain expectedLogSubstring (use Eventually around
scenario.KubectlTgtNonAdmin.Run("logs", "pvc-reader", "-n", namespace) and
assert it Contains expectedLogSubstring), or if flakiness persists, add a Ginkgo
Skip at the top of the spec referencing crane#408 until that bug is fixed.
In `@e2e-tests/utils/utils_test.go`:
- Around line 312-331: Add direct unit tests for the TestdataFilePath helper:
create a new table-driven test in e2e-tests/utils/utils_test.go (alongside
TestCaptureAPISurfaceScriptPath) that exercises TestdataFilePath() for the happy
path (valid filename returns an absolute, existing path ending with the expected
suffix) and the error path (empty filename returns the expected error).
Reference the TestdataFilePath function name to locate the helper, assert
filepath.IsAbs, strings.HasSuffix on filepath.Clean(result), and that os.Stat
succeeds for the valid case, and assert a non-nil/expected error for the
empty-filename case.
In `@e2e-tests/utils/utils.go`:
- Around line 104-115: runtime.Caller(0) can return a trimmed/relative filename,
so update TestdataFilePath (and likewise CaptureAPISurfaceScriptPath) to resolve
absolute filesystem paths: after obtaining thisFile from runtime.Caller(0) call
filepath.Abs(thisFile) (handle the error), use filepath.Dir on that absolute
path, build the final path with filepath.Join, then call filepath.Abs on the
final path (or return the already absolute result) and propagate any errors;
ensure both functions return an absolute path and proper error on failure.
In `@internal/transform/stages.go`:
- Around line 92-108: FilterStages currently returns matches in selector.Stages
order which breaks RunMultiStage's dependency chain; change FilterStages to
iterate over allStages and append a stage only if its DirName or PluginName is
present in selector.Stages (use a lookup set built from selector.Stages) while
still skipping duplicates via seen, so the returned filtered slice preserves the
pipeline order as defined by allStages; update references inside FilterStages to
use selector.Stages lookup, seen, filtered, allStages, stage.DirName and
stage.PluginName accordingly.
In `@internal/transform/writer_integration_test.go`:
- Around line 545-547: The test currently skips when kubectl/oc are missing via
hasKustomizeCommand(t), but applyStageTransforms() now uses the in-process
kustomize runner so remove that external-gate: delete the conditional skip block
that calls hasKustomizeCommand(t) in writer_integration_test.go and any other
identical checks around the 630-639 region so the embedded-runner regression
test always runs; keep the rest of the test intact (it exercises
applyStageTransforms()) and ensure no remaining references to
hasKustomizeCommand affect this test.
---
Outside diff comments:
In `@internal/kustomize/args.go`:
- Around line 64-67: The validator currently only checks argName against
AllowedKustomizeArgs and accepts tokens like "--enable-helm=true" even though
Runner.buildOptions expects the bare boolean flag "--enable-helm"; update the
validation in the arg handling block (the code that uses argName and
AllowedKustomizeArgs) to detect tokens containing '=' for known boolean flags
and reject them with a clear error, e.g. if argName is a boolean-typed allowed
key (like "--enable-helm") and the original token contains '=', return an error
asking to use the flag without a value; reference the argName variable and
AllowedKustomizeArgs map when implementing this check and ensure
Runner.buildOptions remains unchanged.
In `@internal/transform/writer.go`:
- Around line 216-226: getResourceID currently returns kind/namespace/name which
omits API group/version and causes cross-group/version collisions; update
getResourceID to include the resource API version (Group/Version) by using
resource.GetAPIVersion() (or resource.GroupVersionKind() to build group/version)
and return a key like apiVersion/kind/namespace/name (or apiVersion/kind/name
for cluster-scoped). Modify the function getResourceID to prepend the apiVersion
component to the existing format so all dedupe maps in WriteStage() distinguish
resources by API group/version as well as kind/namespace/name.
---
Nitpick comments:
In @.github/workflows/build-release-binaries.yml:
- Around line 57-58: The go build step that produces "dist/${binary}" currently
omits embedding version and commit metadata; update the build invocation (the go
build line that outputs to dist/${binary}) to pass ldflags that set the
MTA_OPS_VERSION and MTA_OPS_GIT_COMMIT variables (using the pipeline's version
and commit values, e.g. from a VERSION input or GITHUB_SHA) so the compiled
binary contains those symbols; ensure the workflow exposes/sets the version and
commit env vars before the build and uses them in the ldflags passed to go
build.
- Around line 59-62: Remove the per-binary checksum creation to avoid redundant
work: delete the sha256sum "${binary}" > "${binary}.sha256" invocation in the
build step (the subshell that cd's into dist and emits "${binary}.sha256"), and
also remove the cleanup step in the release job that deletes individual .sha256
files (the rm -f dist/*.sha256 or similar) since the release already generates a
combined checksums.txt; ensure only the combined checksums generation remains.
In `@cmd/apply/apply_test.go`:
- Around line 90-114: TestStageSelectionRouting currently re-implements the
selector logic instead of exercising the real code path; change the test to call
the actual run() path (which uses ApplyMultiStage()) or refactor the selector
construction into a new helper (e.g., BuildStageSelector or ConstructSelector)
that run() calls, then assert the returned selector shape against
tt.selectorStages; ensure TestRun_UnresolvedStagesError is either extended to
include these selector-shape assertions or the new helper is unit-tested
directly so the test no longer compares len(tt.requestedStages) > 0 locally but
validates the selector produced by run() or the extracted helper.
In `@cmd/apply/apply.go`:
- Around line 154-194: The code duplicates stage-matching logic already
implemented in internalTransform.FilterStages when validating o.RequestedStages
before building the StageSelector; refactor to avoid drift by adding and using a
single resolver/validator in the transform package (e.g., expose
internalTransform.ResolveStages or ValidateRequestedStages) that takes
existingStages (from internalTransform.DiscoverStages) and the slice
o.RequestedStages and returns the resolved list and any unresolved names (or an
error), then replace the inline loop in the apply logic to call that new helper
and build the StageSelector using the resolved stage names (keep StageSelector
and ApplyMultiStage behavior unchanged so FilterStages still receives canonical
input).
In `@cmd/transform/completion_test.go`:
- Around line 141-143: Remove the dead logger creation: delete the local
variable assignment "log := logrus.New()" and the subsequent
"log.SetOutput(os.Stderr)" since the "log" variable is never used; also remove
the "logrus" import if it becomes unused after this change so the test compiles
cleanly, leaving only "f := &flags.GlobalFlags{}" as before.
In `@cmd/transform/listplugins/listplugins.go`:
- Around line 95-101: The getFilteredPlugins function currently returns raw
errors from filepath.Abs and plugin.GetFilteredPlugins without context; wrap
both errors to include the absolute plugin directory being processed
(absPluginDir) so callers see which path failed. Specifically, update
getFilteredPlugins to return fmt.Errorf or errors.Wrap-style messages that
include absPluginDir when filepath.Abs fails and when plugin.GetFilteredPlugins
returns an error (referencing getFilteredPlugins and plugin.GetFilteredPlugins
to locate the spots), preserving the original error as the cause.
In `@internal/buildinfo/buildinfo_test.go`:
- Around line 8-57: Replace the three separate tests with a single table-driven
test that iterates scenarios and sets readBuildInfo per case; save the original
readBuildInfo once into a variable (original := readBuildInfo) and defer
restoring it (defer func() { readBuildInfo = original }()), then define a slice
of test cases with fields name, buildInfoReturn (a func returning
*debug.BuildInfo, bool) and expected string, loop over cases using
t.Run(tc.name, func(t *testing.T) { readBuildInfo = tc.buildInfoReturn(); got :=
readKustomizeVersion(); if got != tc.expected { t.Errorf(...) } }), and include
the same three scenarios (build info unavailable, dep not found, dep found with
"sigs.k8s.io/kustomize/kustomize/v5" => "v5.8.1") to remove duplicated
setup/teardown while keeping the original assertions.
In `@internal/kustomize/runner_test.go`:
- Around line 250-325: Add a new unit test that calls Runner.Build with the
--env flag to exercise setEnvVars/restoreEnv: create a Runner with Args
containing "--env" and an env pair (e.g., "FOO=bar"), set up a pre-existing
environment key and an unset key, run runner.Build(ctx) and then assert (1) the
pre-existing key is restored to its original value after Build and (2) the
previously-unset key is unset again; use the existing
TestBuildOptions_ArgMapping patterns and reference Runner.Build,
Runner.buildOptions, setEnvVars, and restoreEnv to locate the implementation to
test.
In `@README.md`:
- Around line 52-53: The README's table entries for the "main" and "Nightly"
artifacts link to GitHub Actions workflow runs but don't warn readers that those
artifacts require GitHub authentication and expire after 30 days (the workflows
set retention-days: 30); update the README near the **🛠️ `main`** and **🌙
Nightly** rows to add a short note that these links point to authenticated,
time-limited GitHub Actions artifacts (expires 30 days) and either suggest users
sign into GitHub to download or provide/mention an alternate longer-term hosting
option for unsigned development builds.
🪄 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: 2de6a1fc-992a-4e42-9039-f09c489423c9
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (45)
.github/workflows/build-release-binaries.ymlREADME.mdcmd/apply/apply.gocmd/apply/apply_test.gocmd/transform/completion_test.gocmd/transform/listplugins/listplugins.gocmd/transform/listplugins/listplugins_test.gocmd/transform/transform.gocmd/transform/transform_test.gocmd/validate/validate.gocmd/validate/validate_test.gocmd/version/version.gocmd/version/version_test.goe2e-tests/framework/crane.goe2e-tests/framework/scenario.goe2e-tests/testdata/basic-instructions-file.yamle2e-tests/tests/mta_813_cronJob_PVC_test.goe2e-tests/tests/mta_827_custom_transformation_stage_test.goe2e-tests/tests/mta_828_instructions_file_migration_test.goe2e-tests/tests/mta_829_validate_alternative_gv_suggestion_test.goe2e-tests/tests/mta_830_instructions_file_force_reconcile_test.goe2e-tests/tests/mta_832_validate_alternative_gv_suggestion_offline_test.goe2e-tests/utils/utils.goe2e-tests/utils/utils_test.gogo.modinternal/apply/kustomize.gointernal/apply/kustomize_test.gointernal/buildinfo/buildinfo.gointernal/buildinfo/buildinfo_test.gointernal/file/file_helper.gointernal/kustomize/args.gointernal/kustomize/args_test.gointernal/kustomize/runner.gointernal/kustomize/runner_test.gointernal/plugin/plugin_helper.gointernal/transform/instructions.gointernal/transform/instructions_test.gointernal/transform/orchestrator.gointernal/transform/orchestrator_test.gointernal/transform/stages.gointernal/transform/stages_test.gointernal/transform/test_helpers.gointernal/transform/writer.gointernal/transform/writer_integration_test.gointernal/transform/writer_test.go
💤 Files with no reviewable changes (1)
- internal/file/file_helper.go
|
|
||
| steps: | ||
| - name: Checkout | ||
| uses: actions/checkout@v4 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Fetch current commit SHAs for all actions used in the workflow
echo "actions/checkout@v4:"
gh api repos/actions/checkout/commits/v4 --jq '.sha'
echo "actions/setup-go@v6:"
gh api repos/actions/setup-go/commits/v6 --jq '.sha'
echo "actions/upload-artifact@v4:"
gh api repos/actions/upload-artifact/commits/v4 --jq '.sha'
echo "actions/download-artifact@v4:"
gh api repos/actions/download-artifact/commits/v4 --jq '.sha'
echo "softprops/action-gh-release@v2:"
gh api repos/softprops/action-gh-release/commits/v2 --jq '.sha'Repository: migtools/mta-crane
Length of output: 399
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE=".github/workflows/build-release-binaries.yml"
echo "== Target file =="
ls -la "$FILE"
echo
echo "== uses: lines (with context) =="
rg -n "^\s*uses:" "$FILE" -n --context 2 || true
echo
echo "== Lines 30-110 (for the referenced ranges) =="
nl -ba "$FILE" | sed -n '30,110p'Repository: migtools/mta-crane
Length of output: 1010
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE=".github/workflows/build-release-binaries.yml"
echo "== All uses: entries =="
rg -n "^\s*uses:" "$FILE" || true
echo
echo "== Exact lines 30-110 =="
cat -n "$FILE" | sed -n '30,110p'Repository: migtools/mta-crane
Length of output: 3017
Unpinned GitHub Actions introduce CI supply-chain risk.
This workflow uses mutable tags; pin them to commit SHAs:
- actions/checkout@v4 (line 37) →
34e114876b0b11c390a56381ad16ebd13914f8d5 - actions/setup-go@v6 (lines 40-42) →
4a3601121dd01d1626a1e23e37211e3254c1c06c - actions/upload-artifact@v4 (line 65) →
ea165f8d65b6e75b540449e92b4886f43607fa02 - actions/download-artifact@v4 (line 82) →
d3f86a106a0bac45b974a628896c90dbdf5c8093 - softprops/action-gh-release@v2 (line 103) →
3bb12739c298aeb8a4eeaf626c5b8d85266b0e65
🧰 Tools
🪛 zizmor (1.25.2)
[warning] 36-37: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[error] 37-37: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
🤖 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 @.github/workflows/build-release-binaries.yml at line 37, Replace the mutable
GitHub Actions tags with the provided commit SHAs to avoid CI supply-chain
risks: update uses: actions/checkout@v4 to uses:
actions/checkout@34e114876b0b11c390a56381ad16ebd13914f8d5, actions/setup-go@v6
to actions/setup-go@4a3601121dd01d1626a1e23e37211e3254c1c06c,
actions/upload-artifact@v4 to
actions/upload-artifact@ea165f8d65b6e75b540449e92b4886f43607fa02,
actions/download-artifact@v4 to
actions/download-artifact@d3f86a106a0bac45b974a628896c90dbdf5c8093, and
softprops/action-gh-release@v2 to
softprops/action-gh-release@3bb12739c298aeb8a4eeaf626c5b8d85266b0e65 in the
workflow file so each uses: entry references the exact commit SHA.
Comment on lines
+760
to
+791
| // Configure orchestrator with minimal setup needed for ensurePreviousStagesRun | ||
| orchestrator := &internalTransform.Orchestrator{ | ||
| Log: log, | ||
| ExportDir: exportDir, | ||
| TransformDir: transformDir, | ||
| PluginDir: pluginDir, | ||
| NewlyCreatedStages: make(map[string]bool), | ||
| } | ||
|
|
||
| // Request a custom stage | ||
| // This should fail because the previous stage (10_KubernetesPlugin) has no output | ||
| _, err := o.resolveAndValidateStages( | ||
| []string{"50_CustomModifications"}, | ||
| orchestrator, | ||
| transformDir, | ||
| pluginDir, | ||
| log, | ||
| ) | ||
|
|
||
| // We expect this to fail because ensurePreviousStagesRun will try to run | ||
| // the previous stage which will fail due to missing export/plugin setup | ||
| if err == nil { | ||
| t.Errorf("expected error when previous stage has no output, but got none") | ||
| } | ||
|
|
||
| // The error should mention the previous stage or output directory | ||
| if err != nil && !strings.Contains(err.Error(), "10_KubernetesPlugin") && | ||
| !strings.Contains(err.Error(), "output") && | ||
| !strings.Contains(err.Error(), "previous") { | ||
| t.Logf("Got error (which is expected): %v", err) | ||
| // This is acceptable - the important thing is that it tried to ensure previous stages | ||
| } |
There was a problem hiding this comment.
Make this assert the previous-stage-output failure specifically.
Right now the test passes on any error, including unrelated setup failures from the intentionally incomplete orchestrator. That means it won't reliably catch a regression in the previous-stage-output check itself.
Based on learnings, "Bug fixes should include regression tests when possible".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmd/transform/transform_test.go` around lines 760 - 791, The test currently
accepts any error from o.resolveAndValidateStages, so change the assertions to
require that err is non-nil and specifically indicates the previous-stage-output
failure: assert err != nil and assert the error message contains either
"10_KubernetesPlugin" or the words "previous" or "output" (or use errors.Is with
a sentinel like ErrPreviousStageNoOutput if one exists). Update the failure
branch to call t.Fatalf or t.Errorf if the error does not include those tokens
so the test only passes when ensurePreviousStagesRun reports the expected
previous-stage output error for the "50_CustomModifications" ->
"10_KubernetesPlugin" scenario.
Comment on lines
+449
to
+483
| // Sort plugins by name for deterministic ordering | ||
| sort.Slice(allPlugins, func(i, j int) bool { | ||
| return allPlugins[i].Metadata().Name < allPlugins[j].Metadata().Name | ||
| }) | ||
|
|
||
| // Assign priority to each plugin | ||
| // Start at 10, increment by 5 for each plugin | ||
| priority := 10 | ||
|
|
||
| for _, plugin := range allPlugins { | ||
| pluginName := plugin.Metadata().Name | ||
|
|
||
| // Validate plugin name is safe to use as directory name | ||
| if err := validateStageNameToken(pluginName); err != nil { | ||
| log.Warnf("Skipping plugin %q: %v", pluginName, err) | ||
| continue | ||
| } | ||
|
|
||
| // Require "Plugin" suffix | ||
| if !strings.HasSuffix(pluginName, "Plugin") { | ||
| log.Warnf("Skipping plugin %q: name must end with 'Plugin'", pluginName) | ||
| continue | ||
| } | ||
|
|
||
| // Use exact plugin metadata name in stage directory | ||
| stageName := internalTransform.GenerateStageName(priority, pluginName) | ||
|
|
||
| log.Infof("Creating default stage for plugin: %s -> %s", pluginName, stageName) | ||
|
|
||
| if err := createStageDirectory(transformDir, stageName, orchestrator); err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| stageNames = append(stageNames, stageName) | ||
| priority += 5 |
There was a problem hiding this comment.
Use the same priority step here as the other stage-creation paths.
This helper advances priorities by 5, but GenerateStageDirNames() and ad-hoc stage creation both use 10-step spacing. That makes a repo bootstrapped with plain transform produce 10_Foo, 15_Bar, while the equivalent --instructions-file run expects 10_Foo, 20_Bar; reconcileInstructionStages() then treats the default-created stage dirs as extras and errors or deletes them.
Suggested fix
// Assign priority to each plugin
- // Start at 10, increment by 5 for each plugin
+ // Start at 10, increment by 10 to stay consistent with
+ // instructions-file and ad-hoc stage generation.
priority := 10
@@
stageNames = append(stageNames, stageName)
- priority += 5
+ priority += 10
}📝 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.
Suggested change
| // Sort plugins by name for deterministic ordering | |
| sort.Slice(allPlugins, func(i, j int) bool { | |
| return allPlugins[i].Metadata().Name < allPlugins[j].Metadata().Name | |
| }) | |
| // Assign priority to each plugin | |
| // Start at 10, increment by 5 for each plugin | |
| priority := 10 | |
| for _, plugin := range allPlugins { | |
| pluginName := plugin.Metadata().Name | |
| // Validate plugin name is safe to use as directory name | |
| if err := validateStageNameToken(pluginName); err != nil { | |
| log.Warnf("Skipping plugin %q: %v", pluginName, err) | |
| continue | |
| } | |
| // Require "Plugin" suffix | |
| if !strings.HasSuffix(pluginName, "Plugin") { | |
| log.Warnf("Skipping plugin %q: name must end with 'Plugin'", pluginName) | |
| continue | |
| } | |
| // Use exact plugin metadata name in stage directory | |
| stageName := internalTransform.GenerateStageName(priority, pluginName) | |
| log.Infof("Creating default stage for plugin: %s -> %s", pluginName, stageName) | |
| if err := createStageDirectory(transformDir, stageName, orchestrator); err != nil { | |
| return nil, err | |
| } | |
| stageNames = append(stageNames, stageName) | |
| priority += 5 | |
| // Sort plugins by name for deterministic ordering | |
| sort.Slice(allPlugins, func(i, j int) bool { | |
| return allPlugins[i].Metadata().Name < allPlugins[j].Metadata().Name | |
| }) | |
| // Assign priority to each plugin | |
| // Start at 10, increment by 10 to stay consistent with | |
| // instructions-file and ad-hoc stage generation. | |
| priority := 10 | |
| for _, plugin := range allPlugins { | |
| pluginName := plugin.Metadata().Name | |
| // Validate plugin name is safe to use as directory name | |
| if err := validateStageNameToken(pluginName); err != nil { | |
| log.Warnf("Skipping plugin %q: %v", pluginName, err) | |
| continue | |
| } | |
| // Require "Plugin" suffix | |
| if !strings.HasSuffix(pluginName, "Plugin") { | |
| log.Warnf("Skipping plugin %q: name must end with 'Plugin'", pluginName) | |
| continue | |
| } | |
| // Use exact plugin metadata name in stage directory | |
| stageName := internalTransform.GenerateStageName(priority, pluginName) | |
| log.Infof("Creating default stage for plugin: %s -> %s", pluginName, stageName) | |
| if err := createStageDirectory(transformDir, stageName, orchestrator); err != nil { | |
| return nil, err | |
| } | |
| stageNames = append(stageNames, stageName) | |
| priority += 10 | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmd/transform/transform.go` around lines 449 - 483, The default plugin stage
priority spacing is inconsistent: in the loop that creates default stages (using
GenerateStageName, createStageDirectory, and the local variable priority) it
currently increments priority by 5 which conflicts with GenerateStageDirNames()
and ad-hoc creation that use 10-step spacing; update the loop to advance
priority by 10 (leave the initial priority = 10) and adjust the comment to
reflect 10-step increments so generated stage names match other paths and
reconcileInstructionStages() won’t treat them as extras.
Comment on lines
+225
to
+276
| By("Skipped - Verify PVC data was transferred intact by running a reader pod on target") | ||
| // TODO(https://github.com/migtools/crane/issues/408): PVC data integrity assertion | ||
| // is commented out due to flaky behavior on Linux (locally and on CI). On macOS, | ||
| // migration works correctly as namespace-admin and PVC content is transferred | ||
| // consistently. On Linux however, even with cluster-admin contexts, the PVC transfers | ||
| // (Bound on target) but the content is sometimes empty with no error surfaced. | ||
| // | ||
| // When investigating the crane source, the rsync server health check result was found | ||
| // to be silently discarded in cmd/transfer-pvc/transfer-pvc.go — if the server never | ||
| // becomes healthy, the command proceeds anyway with no error, making it harder to | ||
| // diagnose why the content isn't being transferred on Linux. | ||
| // | ||
| // Re-enable this assertion once crane#408 is resolved. | ||
| // _, err = scenario.KubectlTgtNonAdmin.Run( | ||
| // "run", "pvc-reader", | ||
| // "-n", namespace, | ||
| // "--image=busybox", | ||
| // "--restart=Never", | ||
| // fmt.Sprintf(`--overrides={ | ||
| // "spec": { | ||
| // "containers": [{ | ||
| // "name": "pvc-reader", | ||
| // "image": "busybox", | ||
| // "command": ["sh", "-c", "cat /data/log.txt || echo FILE_NOT_FOUND"], | ||
| // "volumeMounts": [{"name":"data","mountPath":"/data"}] | ||
| // }], | ||
| // "volumes": [{"name":"data","persistentVolumeClaim":{"claimName":"%s"}}], | ||
| // "restartPolicy": "Never" | ||
| // } | ||
| // }`, pvcName), | ||
| // ) | ||
| // Expect(err).NotTo(HaveOccurred()) | ||
|
|
||
| Eventually(func() string { | ||
| out, err := scenario.KubectlTgtNonAdmin.Run( | ||
| "get", "pod", "pvc-reader", | ||
| "-n", namespace, | ||
| "-o", "jsonpath={.status.phase}", | ||
| ) | ||
| if err != nil { | ||
| return "" | ||
| } | ||
| return out | ||
| }, "2m", "5s").Should(Or(Equal("Succeeded"), Equal("Failed"))) | ||
| // Eventually(func() string { | ||
| // out, err := scenario.KubectlTgtNonAdmin.Run( | ||
| // "get", "pod", "pvc-reader", | ||
| // "-n", namespace, | ||
| // "-o", "jsonpath={.status.phase}", | ||
| // ) | ||
| // if err != nil { | ||
| // return "" | ||
| // } | ||
| // return out | ||
| // }, "2m", "5s").Should(Or(Equal("Succeeded"), Equal("Failed"))) | ||
|
|
||
| pvcReaderLogs, err := scenario.KubectlTgtNonAdmin.Run("logs", "pvc-reader", "-n", namespace) | ||
| Expect(err).NotTo(HaveOccurred()) | ||
| Expect(pvcReaderLogs).To(ContainSubstring(expectedLogSubstring), | ||
| "expected PVC log.txt to contain data written on source cluster") | ||
| log.Printf("PVC data integrity confirmed, source log entries present on target\n") | ||
| // pvcReaderLogs, err := scenario.KubectlTgtNonAdmin.Run("logs", "pvc-reader", "-n", namespace) | ||
| // Expect(err).NotTo(HaveOccurred()) | ||
| // Expect(pvcReaderLogs).To(ContainSubstring(expectedLogSubstring), | ||
| // "expected PVC log.txt to contain data written on source cluster") | ||
| // log.Printf("PVC data integrity confirmed, source log entries present on target\n") | ||
|
|
||
| _, _ = scenario.KubectlTgtNonAdmin.Run("delete", "pod", "pvc-reader", "-n", namespace, "--ignore-not-found=true") | ||
| // _, _ = scenario.KubectlTgtNonAdmin.Run("delete", "pod", "pvc-reader", "-n", namespace, "--ignore-not-found=true") |
There was a problem hiding this comment.
Don’t let this spec pass without proving PVC contents were migrated.
With this block disabled, the test only proves the target CronJob can start and write fresh data after unsuspending. It no longer verifies that transfer-pvc preserved the source data, so this spec can go green while the BUG #408 behavior still exists. Either mark the whole spec pending/skip until the integrity check is stable, or keep a non-flaky assertion against the transferred PVC contents so the test name and bug labels stay accurate.
Based on learnings: "Bug fixes should include regression tests when possible".
🤖 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 `@e2e-tests/tests/mta_813_cronJob_PVC_test.go` around lines 225 - 276, The test
currently disables the PVC data-integrity assertion so it no longer verifies
transfer-pvc preserved source data; either mark the whole spec pending/skip or
restore a non-flaky check: re-enable the pvc-reader flow (creating pod
"pvc-reader" via scenario.KubectlTgtNonAdmin.Run using pvcName/namespace), but
make the assertion robust by waiting for the pod to reach a terminal phase with
a longer Eventually timeout and retrying logs until they contain
expectedLogSubstring (use Eventually around
scenario.KubectlTgtNonAdmin.Run("logs", "pvc-reader", "-n", namespace) and
assert it Contains expectedLogSubstring), or if flakiness persists, add a Ginkgo
Skip at the top of the spec referencing crane#408 until that bug is fixed.
Comment on lines
+312
to
+331
| func TestCaptureAPISurfaceScriptPath(t *testing.T) { | ||
| // Ensure helper returns an absolute, existing path to scripts/capture-api-surface.sh. | ||
| scriptPath, err := CaptureAPISurfaceScriptPath() | ||
| if err != nil { | ||
| t.Fatalf("CaptureAPISurfaceScriptPath: %v", err) | ||
| } | ||
|
|
||
| if !filepath.IsAbs(scriptPath) { | ||
| t.Fatalf("script path must be absolute, got %q", scriptPath) | ||
| } | ||
|
|
||
| wantSuffix := filepath.Join("scripts", "capture-api-surface.sh") | ||
| if !strings.HasSuffix(filepath.Clean(scriptPath), wantSuffix) { | ||
| t.Fatalf("script path %q must end with %q", scriptPath, wantSuffix) | ||
| } | ||
|
|
||
| if _, err := os.Stat(scriptPath); err != nil { | ||
| t.Fatalf("os.Stat(%q): %v", scriptPath, err) | ||
| } | ||
| } |
There was a problem hiding this comment.
Add direct regression coverage for TestdataFilePath() as well.
utils.go added two path helpers, but this file only tests one of them. A small table-driven test for the happy path plus the empty-filename error would keep the new instructions-file path contract covered without relying only on the E2E suite.
Suggested test shape
+func TestTestdataFilePath(t *testing.T) {
+ tests := []struct {
+ name string
+ filename string
+ wantErr bool
+ wantSuffix string
+ }{
+ {
+ name: "existing_file",
+ filename: "basic-instructions-file.yaml",
+ wantSuffix: filepath.Join("e2e-tests", "testdata", "basic-instructions-file.yaml"),
+ },
+ {
+ name: "empty_filename",
+ filename: "",
+ wantErr: true,
+ },
+ }
+
+ for _, tt := range tests {
+ t.Run(tt.name, func(t *testing.T) {
+ got, err := TestdataFilePath(tt.filename)
+ if tt.wantErr {
+ if err == nil {
+ t.Fatal("expected error, got nil")
+ }
+ return
+ }
+ if err != nil {
+ t.Fatalf("TestdataFilePath(%q): %v", tt.filename, err)
+ }
+ if !filepath.IsAbs(got) {
+ t.Fatalf("path must be absolute, got %q", got)
+ }
+ if !strings.HasSuffix(filepath.Clean(got), tt.wantSuffix) {
+ t.Fatalf("path %q must end with %q", got, tt.wantSuffix)
+ }
+ })
+ }
+}As per coding guidelines, **/*_test.go: All new features require tests.
📝 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.
Suggested change
| func TestCaptureAPISurfaceScriptPath(t *testing.T) { | |
| // Ensure helper returns an absolute, existing path to scripts/capture-api-surface.sh. | |
| scriptPath, err := CaptureAPISurfaceScriptPath() | |
| if err != nil { | |
| t.Fatalf("CaptureAPISurfaceScriptPath: %v", err) | |
| } | |
| if !filepath.IsAbs(scriptPath) { | |
| t.Fatalf("script path must be absolute, got %q", scriptPath) | |
| } | |
| wantSuffix := filepath.Join("scripts", "capture-api-surface.sh") | |
| if !strings.HasSuffix(filepath.Clean(scriptPath), wantSuffix) { | |
| t.Fatalf("script path %q must end with %q", scriptPath, wantSuffix) | |
| } | |
| if _, err := os.Stat(scriptPath); err != nil { | |
| t.Fatalf("os.Stat(%q): %v", scriptPath, err) | |
| } | |
| } | |
| func TestCaptureAPISurfaceScriptPath(t *testing.T) { | |
| // Ensure helper returns an absolute, existing path to scripts/capture-api-surface.sh. | |
| scriptPath, err := CaptureAPISurfaceScriptPath() | |
| if err != nil { | |
| t.Fatalf("CaptureAPISurfaceScriptPath: %v", err) | |
| } | |
| if !filepath.IsAbs(scriptPath) { | |
| t.Fatalf("script path must be absolute, got %q", scriptPath) | |
| } | |
| wantSuffix := filepath.Join("scripts", "capture-api-surface.sh") | |
| if !strings.HasSuffix(filepath.Clean(scriptPath), wantSuffix) { | |
| t.Fatalf("script path %q must end with %q", scriptPath, wantSuffix) | |
| } | |
| if _, err := os.Stat(scriptPath); err != nil { | |
| t.Fatalf("os.Stat(%q): %v", scriptPath, err) | |
| } | |
| } | |
| func TestTestdataFilePath(t *testing.T) { | |
| tests := []struct { | |
| name string | |
| filename string | |
| wantErr bool | |
| wantSuffix string | |
| }{ | |
| { | |
| name: "existing_file", | |
| filename: "basic-instructions-file.yaml", | |
| wantSuffix: filepath.Join("e2e-tests", "testdata", "basic-instructions-file.yaml"), | |
| }, | |
| { | |
| name: "empty_filename", | |
| filename: "", | |
| wantErr: true, | |
| }, | |
| } | |
| for _, tt := range tests { | |
| t.Run(tt.name, func(t *testing.T) { | |
| got, err := TestdataFilePath(tt.filename) | |
| if tt.wantErr { | |
| if err == nil { | |
| t.Fatal("expected error, got nil") | |
| } | |
| return | |
| } | |
| if err != nil { | |
| t.Fatalf("TestdataFilePath(%q): %v", tt.filename, err) | |
| } | |
| if !filepath.IsAbs(got) { | |
| t.Fatalf("path must be absolute, got %q", got) | |
| } | |
| if !strings.HasSuffix(filepath.Clean(got), tt.wantSuffix) { | |
| t.Fatalf("path %q must end with %q", got, tt.wantSuffix) | |
| } | |
| }) | |
| } | |
| } |
🤖 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 `@e2e-tests/utils/utils_test.go` around lines 312 - 331, Add direct unit tests
for the TestdataFilePath helper: create a new table-driven test in
e2e-tests/utils/utils_test.go (alongside TestCaptureAPISurfaceScriptPath) that
exercises TestdataFilePath() for the happy path (valid filename returns an
absolute, existing path ending with the expected suffix) and the error path
(empty filename returns the expected error). Reference the TestdataFilePath
function name to locate the helper, assert filepath.IsAbs, strings.HasSuffix on
filepath.Clean(result), and that os.Stat succeeds for the valid case, and assert
a non-nil/expected error for the empty-filename case.
Comment on lines
+104
to
+115
| func TestdataFilePath(filename string) (string, error) { | ||
| if filename == "" { | ||
| return "", fmt.Errorf("filename is required") | ||
| } | ||
| _, thisFile, _, ok := runtime.Caller(0) | ||
| if !ok { | ||
| return "", fmt.Errorf("runtime.Caller failed") | ||
| } | ||
| baseDir := filepath.Dir(thisFile) | ||
| path := filepath.Join(baseDir, "..", "testdata", filename) | ||
| return path, nil | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Go, does runtime.Caller guarantee that the returned filename is absolute? How can -trimpath or different build modes affect runtime.Caller path values?
💡 Result:
In Go, the runtime.Caller function does not guarantee that the returned filename is an absolute path [1][2]. By default, without specific build flags, it typically returns the absolute path to the source file as it existed on the machine where the binary was compiled [3][4][5]. Because these paths are embedded into the binary at compile time, they reflect the build environment rather than the environment where the binary is currently executing [3][4]. The -trimpath build flag significantly alters this behavior to support reproducible builds [6][7]. When -trimpath is used: 1. Local filesystem paths are removed from the recorded file information in the resulting executable [6][8]. 2. Instead of absolute paths, the file names begin with a path independent of the local filesystem [7][8]. - For standard library packages, paths typically start with "go" [7][8]. - For module-based projects, paths typically start with the module path (and version, if applicable) [6][7]. - For other packages, they may appear as a plain import path [7][8]. Because these changes affect the string returned by runtime.Caller, any code that relies on these paths (e.g., to derive directory structures or locate project roots) may break or behave unexpectedly when -trimpath is enabled [4][9]. Additionally, the returned filename consistently uses forward slashes (/) as path separators, regardless of the host operating system, including Windows [1][6]. Other build modes or factors like the use of //line directives can also influence the specific path string returned, as they allow manual overriding of source position information [5].
Citations:
- 1: https://pkg.go.dev/runtime@go1.26.3
- 2: https://pkg.go.dev/runtime?GOOS=darwin
- 3: https://stackoverflow.com/questions/67430105/why-does-runtime-caller0-return-the-path-in-which-the-binary-was-built
- 4: https://stackoverflow.com/questions/51368837/golang-runtime-package-sets-filepaths-from-the-system-on-which-it-was-built
- 5: runtime: status of relative filenames in //line directives unclear golang/go#26207
- 6: Documentation Clarity on what -trimpath does to runtime.Caller golang/go#77468
- 7: golang/go@7469564
- 8: cmd/go: update -trimpath documentation golang/go#50402
- 9: runtime: ExampleFrames with -trimpath fails golang/go#43380
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the current implementations around the referenced line ranges
echo "=== utils.go: TestdataFilePath (around 104-115) ==="
sed -n '80,140p' e2e-tests/utils/utils.go
echo
echo "=== utils.go: CaptureAPISurfaceScriptPath (around 880-930) ==="
sed -n '860,930p' e2e-tests/utils/utils.go
echo
echo "=== Search for TestdataFilePath usage ==="
rg -n "TestdataFilePath\\(" -S e2e-tests || true
echo
echo "=== Search for CaptureAPISurfaceScriptPath usage ==="
rg -n "CaptureAPISurfaceScriptPath\\(" -S e2e-tests || true
echo
echo "=== Inspect the MTA-828 instructions file migration test ==="
rg -n "mta_828|instructions-file|basic-instructions-file\\.yaml|WorkDir" -S e2e-tests/tests || trueRepository: migtools/mta-crane
Length of output: 7941
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Inspect utils_test.go around CaptureAPISurfaceScriptPath test ==="
sed -n '260,380p' e2e-tests/utils/utils_test.go
echo
echo "=== Inspect any tests for TestdataFilePath ==="
rg -n "TestdataFilePath\\(" e2e-tests/utils -S || true
rg -n "TestdataFilePath" e2e-tests -S || true
echo
echo "=== Search repo for -trimpath usage ==="
rg -n --hidden --no-ignore-vcs "trimpath" -S . || trueRepository: migtools/mta-crane
Length of output: 4513
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Search workflows for go test / trimpath ==="
rg -n "go test|-trimpath" .github/workflows -S || true
echo
echo "=== Inspect mta_828_instructions_file_migration_test.go ==="
sed -n '1,140p' e2e-tests/tests/mta_828_instructions_file_migration_test.go
echo
echo "=== Inspect mta_830_instructions_file_force_reconcile_test.go ==="
sed -n '1,200p' e2e-tests/tests/mta_830_instructions_file_force_reconcile_test.goRepository: migtools/mta-crane
Length of output: 11176
Make TestdataFilePath / CaptureAPISurfaceScriptPath return absolute filesystem paths
runtime.Caller(0) doesn’t guarantee an absolute filename (notably when builds use -trimpath), and both helpers currently do only filepath.Join, which means they can return relative/trimmed paths—breaking the stated contracts. This is especially important because CaptureAPISurfaceScriptPath is tested to always return an absolute path (filepath.IsAbs in TestCaptureAPISurfaceScriptPath), and TestdataFilePath is used by MTA-828/MTA-830 after runner.WorkDir is set to a temp directory, so a relative result would resolve against the wrong working dir.
Suggested fix
func TestdataFilePath(filename string) (string, error) {
if filename == "" {
return "", fmt.Errorf("filename is required")
}
_, thisFile, _, ok := runtime.Caller(0)
if !ok {
return "", fmt.Errorf("runtime.Caller failed")
}
baseDir := filepath.Dir(thisFile)
- path := filepath.Join(baseDir, "..", "testdata", filename)
- return path, nil
+ path, err := filepath.Abs(filepath.Join(baseDir, "..", "testdata", filename))
+ if err != nil {
+ return "", fmt.Errorf("resolve testdata path for %q: %w", filename, err)
+ }
+ return path, nil
}
@@
func CaptureAPISurfaceScriptPath() (string, error) {
_, thisFile, _, ok := runtime.Caller(0)
if !ok {
return "", fmt.Errorf("runtime.Caller failed")
}
@@
baseDir := filepath.Dir(thisFile)
- scriptPath := filepath.Join(baseDir, "..", "..", "scripts", "capture-api-surface.sh")
+ scriptPath, err := filepath.Abs(filepath.Join(baseDir, "..", "..", "scripts", "capture-api-surface.sh"))
+ if err != nil {
+ return "", fmt.Errorf("resolve capture script path: %w", err)
+ }
if _, err := os.Stat(scriptPath); err != nil {
return "", fmt.Errorf("capture script not found at %s: %w", scriptPath, err)
}
return scriptPath, nil
}📝 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.
Suggested change
| func TestdataFilePath(filename string) (string, error) { | |
| if filename == "" { | |
| return "", fmt.Errorf("filename is required") | |
| } | |
| _, thisFile, _, ok := runtime.Caller(0) | |
| if !ok { | |
| return "", fmt.Errorf("runtime.Caller failed") | |
| } | |
| baseDir := filepath.Dir(thisFile) | |
| path := filepath.Join(baseDir, "..", "testdata", filename) | |
| return path, nil | |
| } | |
| func TestdataFilePath(filename string) (string, error) { | |
| if filename == "" { | |
| return "", fmt.Errorf("filename is required") | |
| } | |
| _, thisFile, _, ok := runtime.Caller(0) | |
| if !ok { | |
| return "", fmt.Errorf("runtime.Caller failed") | |
| } | |
| baseDir := filepath.Dir(thisFile) | |
| path, err := filepath.Abs(filepath.Join(baseDir, "..", "testdata", filename)) | |
| if err != nil { | |
| return "", fmt.Errorf("resolve testdata path for %q: %w", filename, err) | |
| } | |
| return path, nil | |
| } |
🤖 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 `@e2e-tests/utils/utils.go` around lines 104 - 115, runtime.Caller(0) can
return a trimmed/relative filename, so update TestdataFilePath (and likewise
CaptureAPISurfaceScriptPath) to resolve absolute filesystem paths: after
obtaining thisFile from runtime.Caller(0) call filepath.Abs(thisFile) (handle
the error), use filepath.Dir on that absolute path, build the final path with
filepath.Join, then call filepath.Abs on the final path (or return the already
absolute result) and propagate any errors; ensure both functions return an
absolute path and proper error on failure.
Comment on lines
+92
to
+108
| for _, requested := range selector.Stages { | ||
| for _, stage := range allStages { | ||
| // Skip if already added | ||
| if seen[stage.DirName] { | ||
| continue | ||
| } | ||
|
|
||
| // Match by directory name OR plugin name | ||
| if stage.DirName == requested || stage.PluginName == requested { | ||
| filtered = append(filtered, stage) | ||
| seen[stage.DirName] = true | ||
| break | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Stage not found, return empty | ||
| return []Stage{} | ||
| return filtered |
There was a problem hiding this comment.
Preserve pipeline order when selecting multiple stages.
FilterStages now returns matches in the caller's argument order, but RunMultiStage executes selectedStages sequentially and feeds each later stage from the previous selected stage's output. A selector like ["30_ImagestreamPlugin", "10_KubernetesPlugin"] will therefore run stage 10 against stage 30 output, which is the wrong dependency chain.
Suggested fix
func FilterStages(allStages []Stage, selector StageSelector) []Stage {
// If no stages specified, return all stages
if len(selector.Stages) == 0 {
return allStages
}
- var filtered []Stage
- seen := make(map[string]bool) // Prevent duplicates
-
- for _, requested := range selector.Stages {
- for _, stage := range allStages {
- // Skip if already added
- if seen[stage.DirName] {
- continue
- }
-
- // Match by directory name OR plugin name
- if stage.DirName == requested || stage.PluginName == requested {
- filtered = append(filtered, stage)
- seen[stage.DirName] = true
- break
- }
- }
- }
+ requested := make(map[string]struct{}, len(selector.Stages))
+ for _, name := range selector.Stages {
+ requested[name] = struct{}{}
+ }
+
+ filtered := make([]Stage, 0, len(selector.Stages))
+ seen := make(map[string]bool, len(selector.Stages))
+ for _, stage := range allStages {
+ if seen[stage.DirName] {
+ continue
+ }
+ if _, ok := requested[stage.DirName]; ok {
+ filtered = append(filtered, stage)
+ seen[stage.DirName] = true
+ continue
+ }
+ if _, ok := requested[stage.PluginName]; ok {
+ filtered = append(filtered, stage)
+ seen[stage.DirName] = true
+ }
+ }
return filtered
}📝 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.
Suggested change
| for _, requested := range selector.Stages { | |
| for _, stage := range allStages { | |
| // Skip if already added | |
| if seen[stage.DirName] { | |
| continue | |
| } | |
| // Match by directory name OR plugin name | |
| if stage.DirName == requested || stage.PluginName == requested { | |
| filtered = append(filtered, stage) | |
| seen[stage.DirName] = true | |
| break | |
| } | |
| } | |
| } | |
| // Stage not found, return empty | |
| return []Stage{} | |
| return filtered | |
| for _, requested := range selector.Stages { | |
| for _, stage := range allStages { | |
| // Skip if already added | |
| if seen[stage.DirName] { | |
| continue | |
| } | |
| // Match by directory name OR plugin name | |
| if stage.DirName == requested || stage.PluginName == requested { | |
| filtered = append(filtered, stage) | |
| seen[stage.DirName] = true | |
| break | |
| } | |
| } | |
| } | |
| requested := make(map[string]struct{}, len(selector.Stages)) | |
| for _, name := range selector.Stages { | |
| requested[name] = struct{}{} | |
| } | |
| filtered := make([]Stage, 0, len(selector.Stages)) | |
| seen := make(map[string]bool, len(selector.Stages)) | |
| for _, stage := range allStages { | |
| if seen[stage.DirName] { | |
| continue | |
| } | |
| if _, ok := requested[stage.DirName]; ok { | |
| filtered = append(filtered, stage) | |
| seen[stage.DirName] = true | |
| continue | |
| } | |
| if _, ok := requested[stage.PluginName]; ok { | |
| filtered = append(filtered, stage) | |
| seen[stage.DirName] = true | |
| } | |
| } | |
| return filtered |
🤖 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 `@internal/transform/stages.go` around lines 92 - 108, FilterStages currently
returns matches in selector.Stages order which breaks RunMultiStage's dependency
chain; change FilterStages to iterate over allStages and append a stage only if
its DirName or PluginName is present in selector.Stages (use a lookup set built
from selector.Stages) while still skipping duplicates via seen, so the returned
filtered slice preserves the pipeline order as defined by allStages; update
references inside FilterStages to use selector.Stages lookup, seen, filtered,
allStages, stage.DirName and stage.PluginName accordingly.
Comment on lines
+545
to
+547
| if !hasKustomizeCommand(t) { | ||
| t.Skip("kubectl/oc not available, skipping kustomize build test") | ||
| } |
There was a problem hiding this comment.
Remove the kubectl/oc gate from this embedded-runner test.
applyStageTransforms() now exercises the in-process kustomize runner, so this skip condition makes the new regression test disappear in environments that lack kubectl/oc even though the code under test no longer shells out to either binary. As per coding guidelines, "All new features require tests".
Suggested fix
func TestWriteStage_KustomizeBuildWithMixedResources(t *testing.T) {
- if !hasKustomizeCommand(t) {
- t.Skip("kubectl/oc not available, skipping kustomize build test")
- }
-
tmpDir, err := os.MkdirTemp("", "crane-writer-kustomize-mixed-*")
if err != nil {
t.Fatalf("Failed to create temp dir: %v", err)
@@
- // Run kubectl kustomize to verify the output is valid
+ // Run embedded kustomize to verify the output is valid
stageDir := filepath.Join(transformDir, stageName)
@@
resources, err := o.applyStageTransforms(stageDir)
if err != nil {
- t.Fatalf("kubectl kustomize failed on mixed resources: %v", err)
+ t.Fatalf("embedded kustomize build failed on mixed resources: %v", err)
}Also applies to: 630-639
🤖 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 `@internal/transform/writer_integration_test.go` around lines 545 - 547, The
test currently skips when kubectl/oc are missing via hasKustomizeCommand(t), but
applyStageTransforms() now uses the in-process kustomize runner so remove that
external-gate: delete the conditional skip block that calls
hasKustomizeCommand(t) in writer_integration_test.go and any other identical
checks around the 630-639 region so the embedded-runner regression test always
runs; keep the rest of the test intact (it exercises applyStageTransforms()) and
ensure no remaining references to hasKustomizeCommand affect this test.
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
5 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.
Summary by CodeRabbit
Release Notes
New Features
--stageflag--instructions-fileand--forcefor stage reconciliation--skip-cluster-scopedflagImprovements
Tests