Sync/upstream main 20260626#25
Merged
Merged
Conversation
…es (migtools#439) * crane validate command offline Mode - All compatible standard resources Signed-off-by: Nandini Chandra <nachandr@redhat.com> * crane validate command offline Mode - All compatible standard resources Signed-off-by: Nandini Chandra <nachandr@redhat.com> * crane validate command offline Mode - All compatible standard resources Signed-off-by: Nandini Chandra <nachandr@redhat.com> --------- Signed-off-by: Nandini Chandra <nachandr@redhat.com>
migtools#451) * fix bug 429: --api resources mutual exclusion for --server, --token, --cluster, --user * fix coderabbit comments
* Update build CI to update crane version according to the tag Signed-off-by: M Sajid Mansoori <mmansoor@redhat.com> * Inject correct version for crane from tag Signed-off-by: M Sajid Mansoori <mmansoor@redhat.com> * Add git sha as part of crane version command Signed-off-by: M Sajid Mansoori <mmansoor@redhat.com> * minor update Signed-off-by: M Sajid Mansoori <mmansoor@redhat.com> * Default BuildCommit to main if ldf flags are not used Signed-off-by: M Sajid Mansoori <mmansoor@redhat.com> * keep version main by default if not overwritten from ldflags Signed-off-by: M Sajid Mansoori <mmansoor@redhat.com> --------- Signed-off-by: M Sajid Mansoori <mmansoor@redhat.com>
* Add validation for exportDir exists beforehand to avoid creating transformDir Signed-off-by: M Sajid Mansoori <mmansoor@redhat.com> * Add unit tests for transform Signed-off-by: M Sajid Mansoori <mmansoor@redhat.com> * Add similar check for transform dir existence in apply Signed-off-by: M Sajid Mansoori <mmansoor@redhat.com> * Add unit tests for apply changes Signed-off-by: M Sajid Mansoori <mmansoor@redhat.com> * Fix existing unit test failure Signed-off-by: M Sajid Mansoori <mmansoor@redhat.com> --------- Signed-off-by: M Sajid Mansoori <mmansoor@redhat.com>
… help output (migtools#507) * Update help usage for validate command Signed-off-by: M Sajid Mansoori <mmansoor@redhat.com> * Split flags for export Signed-off-by: M Sajid Mansoori <mmansoor@redhat.com> * Reuse flag-name slice Signed-off-by: M Sajid Mansoori <mmansoor@redhat.com> * Fix ci Signed-off-by: M Sajid Mansoori <mmansoor@redhat.com> --------- Signed-off-by: M Sajid Mansoori <mmansoor@redhat.com>
* Add end-to-end tests for various migration scenarios - Introduced tests for migrating stateful applications, including handling PVCs, ensuring data integrity, and validating configurations. - Added tests for specific cases such as empty PVC migration, ignored resources, and cronjob behavior during migration. - Enhanced logging and validation checks throughout the test suite to ensure robustness and clarity in migration processes. Also updated .gitignore to exclude .DS_Store files. Signed-off-by: midays <midays@redhat.com> * Enhance end-to-end test execution and documentation - Updated Ginkgo commands in the CI workflow to include the `--recurse` flag for running tests, allowing for recursive test discovery. - Modified the test file matching logic to include specific tiers (tier0 and tier1) for better organization and execution of tests. - Improved the README documentation to clarify the structure of the test suite, detailing the purpose of tier0 and tier1 tests and their respective entry points. Signed-off-by: [Your Name] [your.email@example.com] Signed-off-by: midays <midays@redhat.com> * Update Ginkgo commands to use the `-r` flag for recursive test execution - Modified Ginkgo commands in the CI workflow and action configuration to replace `--recurse` with `-r`, streamlining the syntax for running end-to-end tests. - Ensured consistency across test execution commands for improved readability and maintainability. Signed-off-by: midays <midays@redhat.com> * fix unused import Signed-off-by: midays <midays@redhat.com> --------- Signed-off-by: midays <midays@redhat.com> Signed-off-by: [Your Name] [your.email@example.com]
* Fix export resources with long names Adding truncation and hash to resources which exported file name lenght would exceed typically allowed len 255 chars. Fixes migtools#492 Signed-off-by: Marek Aufart <maufart@redhat.com> * Use whole filename length Signed-off-by: Marek Aufart <maufart@redhat.com> * Harden random hex suffix Signed-off-by: Marek Aufart <maufart@redhat.com> --------- Signed-off-by: Marek Aufart <maufart@redhat.com>
* Add validation test for core group omission Signed-off-by: M Sajid Mansoori <mmansoor@redhat.com> * Add testcase id Signed-off-by: M Sajid Mansoori <mmansoor@redhat.com> * Remove redundant assertion Signed-off-by: M Sajid Mansoori <mmansoor@redhat.com> * Add ns assertion Signed-off-by: M Sajid Mansoori <mmansoor@redhat.com> --------- Signed-off-by: M Sajid Mansoori <mmansoor@redhat.com>
Signed-off-by: M Sajid Mansoori <mmansoor@redhat.com>
…e mode (migtools#512) Signed-off-by: Nandini Chandra <nachandr@redhat.com>
…are Forbidden (migtools#515) * Return non zero exit code when ns not present / or insufficient permissions for user Signed-off-by: M Sajid Mansoori <mmansoor@redhat.com> * Add unit tests Signed-off-by: M Sajid Mansoori <mmansoor@redhat.com> * Update based on review suggestions Signed-off-by: M Sajid Mansoori <mmansoor@redhat.com> --------- Signed-off-by: M Sajid Mansoori <mmansoor@redhat.com>
Adding a check for transform stage with empty resources to get error message understandable to user. Fixes: migtools#474 Signed-off-by: Marek Aufart <maufart@redhat.com>
…ode (migtools#504) * Crane validate: mixed compatible and incompatible resources in live mode Signed-off-by: Nandini Chandra <nachandr@redhat.com> * Crane validate: mixed compatible and incompatible resources in live mode Signed-off-by: Nandini Chandra <nachandr@redhat.com> * Crane validate: mixed compatible and incompatible resources in live mode Signed-off-by: Nandini Chandra <nachandr@redhat.com> --------- Signed-off-by: Nandini Chandra <nachandr@redhat.com>
…gtools#528) * Return non zero exit code when ns not present / or insufficient permissions for user Signed-off-by: M Sajid Mansoori <mmansoor@redhat.com> * Add unit tests Signed-off-by: M Sajid Mansoori <mmansoor@redhat.com> * Update based on review suggestions Signed-off-by: M Sajid Mansoori <mmansoor@redhat.com> * Add single incompatible route test Signed-off-by: M Sajid Mansoori <mmansoor@redhat.com> * Add correct testcase id Signed-off-by: M Sajid Mansoori <mmansoor@redhat.com> --------- Signed-off-by: M Sajid Mansoori <mmansoor@redhat.com>
* Add InsecureSkipTLSVerify option to K8sDeployApp and related configurations - Introduced InsecureSkipTLSVerify boolean flag in the configuration to allow skipping TLS certificate verification for k8sdeploy connections. - Updated K8sDeployApp struct to include InsecureSkipTLS field. - Modified deployment, validation, and cleanup methods to utilize the new InsecureSkipTLS flag. - Enhanced migration scenario setup to pass the InsecureSkipTLSVerify configuration. - Updated e2e test suite initialization to include the new flag for better handling of OCP clusters with self-signed certificates. Signed-off-by: [Your Name] [your.email@example.com] Signed-off-by: midays <midays@redhat.com> * Remove GrantNamespaceAdminToUser and RevokeNamespaceAdminFromUser functions from KubectlRunner Signed-off-by: midays <midays@redhat.com> * Add reporting for E2E test results with MTA IDs - Introduced a new function to extract MTA IDs from test reports. - Enhanced the test suite to log the results of each test, indicating whether it passed or failed along with the corresponding MTA ID. - Updated both tier0 and tier1 test files to include the new reporting functionality. Signed-off-by: midays <midays@redhat.com> * Enhance E2E test result reporting to include skipped tests - Updated the E2E test suite in both tier0 and tier1 to utilize the Ginkgo types for improved reporting. - Added handling for skipped tests in the output, alongside passed and failed results. - This change enhances clarity in test results and provides better insights during test execution. Signed-off-by: midays <midays@redhat.com> * Add test helper functions for MTA ID extraction and reporting - Introduced a new file `test_helpers.go` containing functions to extract MTA IDs from test descriptions and register a reporter for logging test results. - Updated the E2E test suite in both tier0 and tier1 to utilize the new reporting functionality, enhancing clarity in test results by correlating them with MTA tickets. Signed-off-by: midays <midays@redhat.com> --------- Signed-off-by: [Your Name] [your.email@example.com] Signed-off-by: midays <midays@redhat.com>
* fix: resolve nil error in custom stage name validation * fix: resolve nil error and add regression test for custom stage validation * test: verify wrapped error and directory absence for invalid stage names * fix: remove personal files from gitignore
* Add end-to-end test for HPA migration (MTA-837) - Introduced a new test file `mta_837_hpa_migration_test.go` to validate the export, transformation, and application of Horizontal Pod Autoscaler (HPA) resources during migration. - The test verifies the existence of HPA on the source cluster, checks the correctness of the exported manifest, and ensures that the HPA is correctly applied and configured on the target cluster. - Included validation for HPA's scaleTargetRef and resource metrics to ensure they match the source values. Signed-off-by: midays <midays@redhat.com> * Refactor HPA migration test and utility functions - Moved the `toInt64` and `extractCPUAverageUtilization` functions from the HPA migration test file to the utils package for better reusability. - Cleaned up the HPA migration test file by removing the now-externalized functions, improving readability and maintainability. Signed-off-by: midays <midays@redhat.com> * Refactor utility functions for HPA migration test - Updated the HPA migration test to use the newly exported utility functions `ToInt64` and `ExtractCPUAverageUtilization` from the utils package, enhancing code clarity and reusability. - Renamed utility functions to follow Go naming conventions, improving consistency across the codebase. Signed-off-by: midays <midays@redhat.com> --------- Signed-off-by: midays <midays@redhat.com>
* Deprecate plugin optionals The original crane plugin system supports optional flags to be passed to plugins. Continuation of these flags was described in migtools#437 with aim to remove it. However to stay on safe side, pending crane plugin system update (partially related to migtools#415) and some gut feeling about possible need for args for each transform stage, I chose the way to mark these flags as deprecated, but keeping them for v0.10 release. Change 1. Marked related flags and subcommands as deprecated 2. Added small test to check they work (so, we'll see a failure after removing them) Fixes: migtools#437 Signed-off-by: Marek Aufart <maufart@redhat.com> * Update optionals test Signed-off-by: Marek Aufart <maufart@redhat.com> --------- Signed-off-by: Marek Aufart <maufart@redhat.com>
Signed-off-by: M Sajid Mansoori <mmansoor@redhat.com>
There was a recent change in KubernetesPlugin regarding to Job resource cleanup in migtools/crane-lib#138 Updating crane-lib dependency to get this update into crane. Related to migtools/crane-lib#138 Signed-off-by: Marek Aufart <maufart@redhat.com>
…ndard resources (migtools#394) * Crane validate: All compatible standard resources in live mode Signed-off-by: Nandini Chandra <nachandr@redhat.com> * Crane validate live mode : App with all compatible standard resources Signed-off-by: Nandini Chandra <nachandr@redhat.com> * Crane validate live mode : Manifests with all compatible standard resources Signed-off-by: Nandini Chandra <nachandr@redhat.com> * Automate Crane validate live mode : Manifests with all compatible standard resources Signed-off-by: Nandini Chandra <nachandr@redhat.com> --------- Signed-off-by: Nandini Chandra <nachandr@redhat.com>
* Refactor transform dir structure Implementation of https://github.com/aufi/move-crane/blob/main/drafts/STAGE_DIRECTORY_REFACTOR_PLAN.md Signed-off-by: Marek Aufart <maufart@redhat.com> * Fix unit tests, address feedback Signed-off-by: Marek Aufart <maufart@redhat.com> * Update E2E tests suite Signed-off-by: Marek Aufart <maufart@redhat.com> * Update docs and var names in test Signed-off-by: Marek Aufart <maufart@redhat.com> * Fix export test changes and .work cleanup Signed-off-by: Marek Aufart <maufart@redhat.com> * Update additional .work or transform resources/ occurences Signed-off-by: Marek Aufart <maufart@redhat.com> --------- Signed-off-by: Marek Aufart <maufart@redhat.com>
* improved log for custom stage * fixed comments
Signed-off-by: Nandini Chandra <nachandr@redhat.com>
…ls#566) * Add more assertions for transform directory structure changes Signed-off-by: M Sajid Mansoori <mmansoor@redhat.com> * Update based on code rabbit suggestions Signed-off-by: M Sajid Mansoori <mmansoor@redhat.com> --------- Signed-off-by: M Sajid Mansoori <mmansoor@redhat.com>
migtools#519) * refactor: replace positional args with typed option structs in e2e test framework Introduce ExportOptions, TransformOptions, and ApplyOptions structs in the e2e framework and refactor all test call sites to use them instead of passing individual string arguments. Signed-off-by: Ran Wurmbrand <rwurmbra@redhat.com> * addressed Code Rabit suggestion, and made a cosmetic change on function variables name) Signed-off-by: Ran Wurmbrand <rwurmbra@redhat.com> --------- Signed-off-by: Ran Wurmbrand <rwurmbra@redhat.com>
* add skip-ocp label to pvc-transfer tests * update label to transfer-pvc
…mn (migtools#565) * chore: update gitignore to track personal tools and scripts * fix: resolve duplicate suggestion text in validate output reason column * chore: restore original .gitignore to prevent blending personal tools
…nd crane / mta-ops (migtools#580) * feat: add PR ready-for-review Slack notification workflow * feat: refine PR ready-for-review Slack workflow Co-authored-by: Cursor <cursoragent@cursor.com> * feat: improve ready-for-review Slack workflow * Fixed mta 817 Signed-off-by: M Sajid Mansoori <mmansoor@redhat.com> * chore: remove pr-ready-for-review-slack workflow from PR * Fix 817 for mta-ops and crane Signed-off-by: M Sajid Mansoori <mmansoor@redhat.com> --------- Signed-off-by: M Sajid Mansoori <mmansoor@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
…#571) * crane validate --api-resources fails when KUBECONFIG is set Signed-off-by: Nandini Chandra <nachandr@redhat.com> * crane validate --api-resources fails when KUBECONFIG is set Signed-off-by: Nandini Chandra <nachandr@redhat.com> * crane validate --api-resources fails when KUBECONFIG is set Signed-off-by: Nandini Chandra <nachandr@redhat.com> --------- Signed-off-by: Nandini Chandra <nachandr@redhat.com>
* feat: add PR ready-for-review Slack notification workflow * feat: refine PR ready-for-review Slack workflow Co-authored-by: Cursor <cursoragent@cursor.com> * feat: improve ready-for-review Slack workflow * Fix test Signed-off-by: M Sajid Mansoori <mmansoor@redhat.com> * chore: remove pr-ready-for-review slack workflow from branch --------- Signed-off-by: M Sajid Mansoori <mmansoor@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
* feat: add PR ready-for-review Slack notification workflow * feat: refine PR ready-for-review Slack workflow Co-authored-by: Cursor <cursoragent@cursor.com> * feat: improve ready-for-review Slack workflow * Skip route test for ocp because ocp has route resource Signed-off-by: M Sajid Mansoori <mmansoor@redhat.com> * chore: remove pr-ready-for-review slack workflow from branch Co-authored-by: Cursor <cursoragent@cursor.com> --------- Signed-off-by: M Sajid Mansoori <mmansoor@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
Signed-off-by: M Sajid Mansoori <mmansoor@redhat.com>
|
Warning Review limit reached
More reviews will be available in 26 minutes and 4 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughStage paths now use ChangesRepository-wide pipeline and e2e updates
Sequence Diagram(s)Validation command flow sequenceDiagram
participant NewValidateCommand
participant ValidateOptions
participant ParseAPIResourcesJSON
participant MatchResultsFromIndex
NewValidateCommand->>ValidateOptions: Validate(c)
ValidateOptions->>ParseAPIResourcesJSON: parse api-resources JSON
ValidateOptions->>MatchResultsFromIndex: compare manifests to discovery index
MatchResultsFromIndex-->>ValidateOptions: ValidationReport
Typed Crane pipeline flow sequenceDiagram
participant RunCranePipelineWithChecks
participant RunCranePipeline
participant CraneRunner
RunCranePipelineWithChecks->>RunCranePipeline: ExportOptions / TransformOptions / ApplyOptions
RunCranePipeline->>CraneRunner: Export(opts)
RunCranePipeline->>CraneRunner: Transform(opts)
RunCranePipeline->>CraneRunner: Apply(opts)
Estimated code review effort🎯 5 (Critical) | ⏱️ ~180 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ 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 |
Test Coverage ReportTotal: 47.8% 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: 15
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (9)
cmd/validate/validate.go-146-151 (1)
146-151: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPopulate
report.ClusterContextfor default-context runs.When
--contextis omitted, this branch only logs the fallback path.internal/validate/report.go:24-31still rendersreport.ClusterContextfor live mode, so saved reports end up with an empty context string for the common "use current kubeconfig context" case.🤖 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/validate/validate.go` around lines 146 - 151, Populate report.ClusterContext in the live-mode fallback path when --context is omitted. In validate.go’s live validation branch, set report.ClusterContext to the current kubeconfig context before logging the default-context message, so internal/validate/report.go can render the correct context for saved reports. Keep the existing explicit --context handling in the same conditional around o.configFlags.Context and report.ClusterContext.internal/transform/writer.go-192-195 (1)
192-195: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUpdate the whiteout footer to match
input/.
kustomization.yamlnow emitsinput/...entries here, but the footer text built ingenerateKustomizationWithComments()still says whiteout resources are written toresources/. That leaves the generated file internally inconsistent when users debug stage output.🤖 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 192 - 195, The whiteout footer text in generateKustomizationWithComments is stale and still refers to resources/ even though the generated kustomization now uses input/ entries. Update the whiteout comment निर्माण in writer.go, near the whiteoutComments append logic in the kustomization writer, so the footer consistently references input/ and matches the emitted paths.internal/plugin/plugin_manager_helper_test.go-112-145 (1)
112-145: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMake the positive fixture include the current platform.
This test only seeds
linux/darwinbinaries, so it fails on any otherruntime.GOOS/runtime.GOARCHeven whenFilterPluginForOsArchworks. Seed one entry fromruntime.GOOS/runtime.GOARCHand keep the others as non-matches.Suggested fix
Versions: []PluginVersion{ { Name: "TestPlugin", Version: "0.0.1", Binaries: []Binary{ - {OS: "linux", Arch: "amd64", URI: "https://example.com/linux-amd64"}, - {OS: "darwin", Arch: "amd64", URI: "https://example.com/darwin-amd64"}, - {OS: "darwin", Arch: "arm64", URI: "https://example.com/darwin-arm64"}, + {OS: runtime.GOOS, Arch: runtime.GOARCH, URI: "https://example.com/current-platform"}, + {OS: "darwin", Arch: "amd64", URI: "https://example.com/darwin-amd64"}, + {OS: "plan9", Arch: "mips", URI: "https://example.com/plan9-mips"}, }, }, }, }As per coding guidelines,
All tests must pass: go test ./....🤖 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/plugin/plugin_manager_helper_test.go` around lines 112 - 145, The positive fixture in TestFilterPluginForOsArch_PersistsFilter only includes linux/darwin binaries, so it can fail on the current runner even when FilterPluginForOsArch behaves correctly. Update the test data in TestFilterPluginForOsArch_PersistsFilter to include one Binary using runtime.GOOS and runtime.GOARCH as the matching case, while keeping the other binaries as non-matches, so the post-filter assertions remain valid across platforms.Source: Coding guidelines
e2e-tests/tests/tier0/mta_828_instructions_file_migration_test.go-147-148 (1)
147-148: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDon't reject valid
input/resources/...entries.The new contract is that stage resources are rooted under
input/.ContainSubstring("resources/")also rejects legitimate paths likeinput/resources/<ns>/..., so this assertion can fail even when the transform output is correct.Suggested fix
- Expect(resourcePath).NotTo(ContainSubstring("resources/"), fmt.Sprintf("did not expect legacy resources/ path in resources[%d]=%q in %q", i, resourcePath, kustomizationPath)) + Expect(resourcePath).NotTo(HavePrefix("resources/"), fmt.Sprintf("did not expect legacy resources/ root in resources[%d]=%q in %q", i, resourcePath, kustomizationPath))🤖 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/tier0/mta_828_instructions_file_migration_test.go` around lines 147 - 148, The path assertion in the migration test is too broad: the check in the kustomization resource validation is rejecting valid stage paths like input/resources/... because it forbids any "resources/" substring. Update the test around the resourcePath expectations to only reject the legacy root-level resources/ prefix while still allowing input/resources/... entries, keeping the existing input/ requirement in the same validation logic.e2e-tests/tests/tier1/mta_850_validate_duplicate_gvk_test.go-42-45 (1)
42-45: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winResolve testdata relative to the source file, not the process CWD.
filepath.Abs("../../testdata/...")is still cwd-dependent. With the recursive E2E runner changes in this stack, that makes fixture lookup brittle; use the existing testdata helper (orruntime.Caller) instead.Suggested fix
import ( "encoding/json" "log" "os" "path/filepath" "github.com/konveyor/crane/e2e-tests/config" . "github.com/konveyor/crane/e2e-tests/framework" + "github.com/konveyor/crane/e2e-tests/utils" cranevalidate "github.com/konveyor/crane/internal/validate" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" ) @@ - sourcePath, err := filepath.Abs(filepath.Join("../../testdata", mf.testdataFile)) + sourcePath, err := utils.TestdataFilePath(mf.testdataFile)🤖 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/tier1/mta_850_validate_duplicate_gvk_test.go` around lines 42 - 45, The fixture lookup in the manifest loop is still dependent on the process working directory because it builds paths with filepath.Abs on a relative testdata path. Update the test to resolve testdata from the source file location instead, using the existing testdata helper or runtime.Caller, so the sourcePath computation in mta_850_validate_duplicate_gvk_test.go is stable regardless of how the E2E runner invokes the test.e2e-tests/utils/utils.go-1082-1094 (1)
1082-1094: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReject non-integral
float64inputs inToInt64.
int64(n)silently truncates values like49.9to49. That can turn malformed JSON data into a different integer instead of surfacing the bad input.Proposed fix
+// add: import "math" func ToInt64(v any) (int64, error) { switch n := v.(type) { case float64: + if math.Trunc(n) != n || n > math.MaxInt64 || n < math.MinInt64 { + return 0, fmt.Errorf("cannot convert non-integer float64 %v to int64", n) + } return int64(n), nil case json.Number: return n.Int64()🤖 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 1082 - 1094, Reject non-integral float64 values in ToInt64 instead of truncating them; update the float64 branch to verify the value is a whole number before converting. Use ToInt64 as the entry point and keep the existing handling for json.Number, int64, and string unchanged, but return an error for any float64 input with a fractional part so malformed JSON is surfaced.e2e-tests/tests/tier0/mta_831_validate_compatible_resources_offline_test.go-126-133 (1)
126-133: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAssert
report.Modeexplicitly.This new offline-mode test only logs
report.Mode, so it would still pass if the report started emitting the wrong mode while keepingapiResourcesSourcepopulated. Add a directExpect(report.Mode).To(Equal("offline"))next to the other report assertions.As per coding guidelines,
**/*_test.go: All new features require tests.Also applies to: 202-213
🤖 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/tier0/mta_831_validate_compatible_resources_offline_test.go` around lines 126 - 133, The offline validation test currently verifies apiResourcesSource but only logs report.Mode, so it can miss a wrong mode being emitted. In the mta_831_validate_compatible_resources_offline_test.go assertions around the ValidationReport unmarshaling, add an explicit Expect on report.Mode to equal offline alongside the existing report.APIResourcesSource checks. Keep the assertion near the current report validation block so the test directly fails if ValidationReport.Mode changes unexpectedly.Source: Coding guidelines
e2e-tests/golden-manifests-ocp/simple-nginx-nopv/output/output.yaml-77-80 (1)
77-80: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winKeep the aggregate ConfigMap fixture semantically identical to the split resource fixture.
Lines 79-80 encode
service-ca.crtas a block scalar, which parses asREDACTED\n. The split fixture ate2e-tests/golden-manifests-ocp/simple-nginx-nopv/output/resources/simple-nginx-nopv/ConfigMap__v1_simple-nginx-nopv_openshift-service-ca.crt.yamlstoresREDACTEDwithout the newline, so these two goldens no longer describe the same object.Suggested fix
data: - service-ca.crt: | - REDACTED + service-ca.crt: REDACTED🤖 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/golden-manifests-ocp/simple-nginx-nopv/output/output.yaml` around lines 77 - 80, The aggregate ConfigMap fixture currently serializes service-ca.crt as a block scalar, which adds a trailing newline and makes it differ from the split ConfigMap fixture. Update the golden in output.yaml so the service-ca.crt value matches the split resource fixture exactly, and keep the aggregate and split representations semantically identical for the simple-nginx-nopv ConfigMap__v1_simple-nginx-nopv_openshift-service-ca.crt resource.e2e-tests/tests/tier0/mta_801_stateful_migration_test.go-15-15 (1)
15-15: 🩺 Stability & Availability | 🟡 MinorVerify CI contract for
pvc-transferlabel to prevent hard failures.The test unconditionally executes
runner.TransferPVCafter switching to thepvc-transferlabel. The previous logic protected against missing infrastructure by explicitly skipping; the new implementation relies entirely on the CI environment guaranteeing PVC transfer support.There is no evidence in
.github/workflows/e2e-pr-tester.yamlthat Ginkgo automatically excludespvc-transferspecs when infrastructure is unavailable. If these tests run in an environment without PVC transfer configured, they will fail rather than skip.Please confirm:
- All CI runners executing the
e2e-testssuite are provisioned with PVC transfer capabilities.- Alternatively, add a CI-level
--label-filterexclusion if PVC transfer is optional.- Ensure
e2e-tests/tests/tier0/mta_804_empty_pvc_migration_test.gofollows the same contract.🤖 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/tier0/mta_801_stateful_migration_test.go` at line 15, The `pvc-transfer`-labeled MTA stateful migration spec now assumes PVC transfer is always available, so confirm the CI contract or restore a skip path in the `It("[MTA-801]...")` test before calling `runner.TransferPVC`. Update the `mta_801_stateful_migration_test.go` flow to either explicitly skip when PVC-transfer infrastructure is absent or ensure the e2e CI workflow applies a label filter that excludes `pvc-transfer` specs unless support is provisioned. Apply the same treatment to `mta_804_empty_pvc_migration_test.go` so both tests follow the same availability contract.
🧹 Nitpick comments (6)
cmd/export/discover.go (1)
283-288: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winInclude
g.APIResource.Nameand the original error in these Kubernetes log branches.
kind+groupVersionalone still leaves discovery failures ambiguous for plural resources/subresources, and these branches now drop the server error text entirely. Add the API resource name anderrso the log is actionable without rerunning.Suggested change
- case apierrors.IsForbidden(err): - log.Debugf("access denied for groupVersion %s, kind: %s (expected for namespace-admin users)\n", g.APIGroupVersion, g.APIResource.Kind) - case apierrors.IsMethodNotSupported(err): - log.Warnf("list method not supported on the groupVersion %s, kind: %s\n", g.APIGroupVersion, g.APIResource.Kind) - case apierrors.IsNotFound(err): - log.Debugf("resource not found (virtual resource), groupVersion %s, kind: %s\n", g.APIGroupVersion, g.APIResource.Kind) + case apierrors.IsForbidden(err): + log.Debugf("access denied listing resource %s (groupVersion=%s, kind=%s): %v", g.APIResource.Name, g.APIGroupVersion, g.APIResource.Kind, err) + case apierrors.IsMethodNotSupported(err): + log.Warnf("list not supported for resource %s (groupVersion=%s, kind=%s): %v", g.APIResource.Name, g.APIGroupVersion, g.APIResource.Kind, err) + case apierrors.IsNotFound(err): + log.Debugf("resource %s not found or virtual (groupVersion=%s, kind=%s): %v", g.APIResource.Name, g.APIGroupVersion, g.APIResource.Kind, err)As per coding guidelines, "Include the actual resource type and API resource name in error messages when working with Kubernetes API objects" and "Do not skip error context - include resource names and types in error messages".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/export/discover.go` around lines 283 - 288, The Kubernetes discovery log branches in the `switch` over `apierrors.IsForbidden`, `IsMethodNotSupported`, and `IsNotFound` should include the API resource name and the original error text. Update the `log.Debugf`/`log.Warnf` calls in `discover.go` to reference `g.APIResource.Name` alongside `g.APIGroupVersion` and `g.APIResource.Kind`, and append `err` so the failure context is preserved. This applies to the discovery logging around the resource handling logic in `discover.go`.Source: Coding guidelines
e2e-tests/tests/tier0/mta_813_cronJob_PVC_test.go (1)
15-15: 🩺 Stability & Availability | 🔵 TrivialUpdate CI label filter to prevent hard-failure on generic tier0 jobs
The test
mta_813_cronJob_PVC_test.gonow carries thepvc-transferlabel and unconditionally callsrunner.TransferPVC. The current CI workflow.github/workflows/run-e2e-tier0-tests.ymlfilters with--label-filter="tier0". Since Ginkgo includes specs that match any part of the filter expression (unless negated), thetier0filter selects this spec even though thecranebinary in that job may lack thetransfer-pvcsubcommand.Update the workflow
label_filterto excludepvc-transfertests from the generic tier0 job (e.g.,tier0 && !pvc-transfer) or add exclusion logic to the specific job configuration. Alternatively, ensure the standard tier0 job environment includes thetransfer-pvccapable binary.Relevant code
File: .github/workflows/run-e2e-tier0-tests.yml ... label_filter: tier0 ...File: e2e-tests/tests/tier0/mta_813_cronJob_PVC_test.go ... It("[BUG `#330`][BUG `#408`][MTA-813] Should migrate a cronjob...", Label("BUG `#330`", "BUG `#408`", "tier0", "pvc-transfer"), func() { ...🤖 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/tier0/mta_813_cronJob_PVC_test.go` at line 15, The generic tier0 CI job is still picking up the PVC-transfer cronjob spec because the workflow label filter only matches tier0, while this test now has the pvc-transfer label and unconditionally calls runner.TransferPVC. Update the Ginkgo label filter in the tier0 workflow to exclude pvc-transfer specs, or move that exclusion into the specific job configuration so the cronjob PVC test does not run where the crane binary lacks transfer-pvc support. Use the workflow’s label_filter setting and the mta_813_cronJob_PVC_test.go spec label as the key points to update.e2e-tests/framework/pipeline.go (1)
18-21: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winInclude the mismatched directory values in the preflight error.
This helper now validates six caller-provided fields, but the error doesn’t say which value diverged.
Suggested fix
if (e.ExportDir != t.ExportDir) || (e.ExportDir != a.ExportDir) || (t.TransformDir != a.TransformDir) { - return fmt.Errorf("pipeline directory mismatch: export/transform/apply options must agree on shared directories (exportDir, transformDir)") + return fmt.Errorf( + "pipeline directory mismatch: export options exportDir=%q, transform options exportDir=%q transformDir=%q, apply options exportDir=%q transformDir=%q", + e.ExportDir, t.ExportDir, t.TransformDir, a.ExportDir, a.TransformDir, + ) }🤖 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/framework/pipeline.go` around lines 18 - 21, In RunCranePipeline, the preflight mismatch error is too generic and does not identify which directory values differ. Update the validation error to include the actual ExportDir, TransformDir, and ApplyDir values from the e, t, and a options so callers can see the divergence directly. Keep the check in RunCranePipeline and make the returned fmt.Errorf message report the mismatched directory values clearly.Source: Coding guidelines
e2e-tests/tests/tier0/olm_whiteout_base_test.go (1)
81-90: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winDrop the second apply step here as well.
RunCranePipelineWithChecksalready exercises the apply stage, so callingApplyOutputToTargetagain makes this spec validate a re-apply path rather than the pipeline run it just completed.🤖 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/tier0/olm_whiteout_base_test.go` around lines 81 - 90, The OLM whiteout base test is re-applying manifests after the pipeline already ran the apply stage, so it ends up validating a second apply instead of the pipeline itself. Remove the extra apply step in the test flow around RunCranePipelineWithChecks and ApplyOutputToTarget, keeping the checks that verify the output directory contents after the pipeline completes.e2e-tests/tests/tier0/mta_837_hpa_migration_test.go (2)
75-90: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAvoid reapplying the same output after
RunCranePipelineWithChecks.This helper is already the export/transform/apply path in the suite, so the extra
ApplyOutputToTargetmakes the spec depend on second-apply idempotency instead of validating the first pipeline run.Proposed fix
- By("Apply rendered manifests to target") - log.Printf("Applying rendered manifests on target namespace %s from %s\n", namespace, paths.OutputDir) - Expect(ApplyOutputToTarget(kubectlTgt, namespace, paths.OutputDir)).NotTo(HaveOccurred()) - By("Scale target deployment and validate app")🤖 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/tier0/mta_837_hpa_migration_test.go` around lines 75 - 90, The test is reapplying the rendered output after RunCranePipelineWithChecks, which makes the spec validate a second apply instead of the pipeline itself. Remove the extra ApplyOutputToTarget step and its related logging from mta_837_hpa_migration_test.go, and keep the assertion focused on the pipeline output already produced by RunCranePipelineWithChecks and the HPA manifest check.
24-28: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winUse the source HPA as the assertion source-of-truth.
The spec says the migrated values should match the source, but the expectations are duplicated in constants. Reading
minReplicas,maxReplicas, and CPU target fromhpaSrcwould keep the test aligned with the fixture and make the source fetch meaningful.Also applies to: 53-54, 119-135
🤖 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/tier0/mta_837_hpa_migration_test.go` around lines 24 - 28, The HPA migration test is duplicating expected values in constants instead of asserting against the source HPA fixture. Update the test logic in the migration flow to read minReplicas, maxReplicas, and CPU target directly from hpaSrc and use those values in the assertions so the source HPA becomes the single source of truth. Make the same change wherever those duplicated expectations are used in the test, including the later assertion block that compares the migrated HPA spec.
🤖 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/e2e-pr-tester.yaml:
- Around line 202-207: The e2e workflow’s `Run touched crane e2e tests` step is
inlining `needs.detect-changes.outputs.focus_file_regex` directly inside the
shell script, which can allow shell metacharacter injection. Move that value
into the step’s `env:` block and reference it as a shell variable in the `ginkgo
run` command, keeping the `--focus-file` argument literal and avoiding direct
GitHub expression interpolation inside `run:`.
In `@cmd/apply/apply.go`:
- Around line 146-153: The overwrite handling in apply should not delete the
transform source tree or wipe output before validation is complete. In the apply
flow around os.Stat, os.RemoveAll, and the outputDir/transformDir setup, first
resolve and compare the paths to reject any overlap between outputDir and
transformDir, then defer removing the destination until after all remaining
validation and stage resolution succeed. Keep the guard in the apply logic so
--overwrite only replaces a safe destination and never the input stages.
In `@cmd/export/export.go`:
- Around line 57-63: The export command’s kubeconfig handling is overwriting
config-derived values in `PreRun`, causing `o.configFlags.KubeConfig` to be
cleared whenever the Cobra flag was not explicitly changed. Update the `PreRun`
logic in the export flow to only set `KubeConfig` from the flag when the user
actually provided it, and otherwise preserve the value already unmarshaled from
config/Viper so `RawConfig()` and `Namespace()` can use it correctly.
In `@cmd/transform/transform.go`:
- Around line 63-77: `validateOptions` in `cmd/transform/transform.go` is
unconditionally requiring `--export-dir` to exist, which breaks reruns that rely
on `RunMultiStage()` reading from a prior stage output instead of the original
export tree. Update the `o.ExportDir` handling so the `filepath.Abs`
normalization stays, but defer the `os.Stat` existence/accessibility check until
you know the first selected stage actually needs export input, and skip that
check for reruns that source `transform/<prev>/output`.
In `@cmd/validate/validate_test.go`:
- Around line 412-519: Add test coverage in
TestValidate_HelpGroupsFlags/ValidateOptions-related tests for the new Run()
validate-dir overwrite behavior: verify the command fails when the validate
directory already exists and --overwrite is not set, and verify it succeeds by
removing/recreating the directory when --overwrite is enabled. Use the existing
ValidateOptions, NewValidateCommand, and Run/Complete flow to locate the branch
being exercised, and assert both the error path and the overwrite path directly.
- Around line 457-500: The invalid-context test is not wiring Cobra flags, so
Complete() may ignore the temp kubeconfig and fall back to the user’s ambient
config. Update TestComplete_InvalidContextFailsBeforeRun to register the
expected flags on the cobra.Command and set the kubeconfig flag explicitly
before calling ValidateOptions.Complete, so the existing kubeconfig override
logic in Complete() and ToDiscoveryClient uses the fixture instead of
~/.kube/config.
In `@e2e-tests/framework/client.go`:
- Around line 210-222: The SelfSubjectReview call in
resolveUsernameViaSelfSubjectReview uses an unbounded context, which can hang
the E2E suite if the API server stalls. Replace context.Background() with a
timeout-based context for
clientSet.AuthenticationV1().SelfSubjectReviews().Create, and ensure the
existing NewClientSetForContext setup error includes the contextName for easier
debugging.
In `@e2e-tests/framework/kubectl.go`:
- Around line 204-229: The dry-run noise filter in isDryRunOCPNoise is too
permissive because it returns true even when no error line matched any entry in
dryRunKnownOCPErrors, which can hide real failures. Update the logic so it only
suppresses the dry-run failure after at least one error line has been found and
every such error line matches a known OCP noise pattern; if there are no
error-prefixed lines or any unmatched error line, return false so
ValidateApplyDir does not continue to apply on genuine dry-run errors.
In
`@e2e-tests/golden-manifests-ocp/simple-nginx-nopv/export/resources/simple-nginx-nopv/EndpointSlice_discovery.k8s.io_v1_simple-nginx-nopv_my-simple-nginx-nopv-rf7lk.yaml`:
- Around line 7-48: Controller-generated EndpointSlice fixtures are using a
non-deterministic `-rf7lk` suffix in both the
`EndpointSlice_discovery.k8s.io_v1_simple-nginx-nopv_my-simple-nginx-nopv-rf7lk.yaml`
fixture name and the manifest’s `metadata.name`, which breaks exact golden-path
matching. Update the golden export/comparison flow for `simple-nginx-nopv` so
`EndpointSlice` resources are either omitted from export or normalized to a
stable synthetic name before writing and comparing fixtures, using the existing
EndpointSlice handling in the fixture resolver/export logic.
In
`@e2e-tests/golden-manifests-ocp/simple-nginx-nopv/export/resources/simple-nginx-nopv/ReplicaSet_apps_v1_simple-nginx-nopv_simple-nginx-nopv-deployment-866ff66bdd.yaml`:
- Around line 87-120: Remove the live ReplicaSet status fields from the golden
manifest so the export only captures stable desired-state data. Update the
fixture generation/assertion around the
ReplicaSet_apps_v1_simple-nginx-nopv_simple-nginx-nopv-deployment-866ff66bdd
output to ignore or strip the status block, especially the controller-populated
counters in the ReplicaSet manifest. Keep the focus on the spec/template content
and normalize server-populated fields before comparing fixtures.
In
`@e2e-tests/golden-manifests-ocp/simple-nginx-nopv/export/resources/simple-nginx-nopv/RoleBinding_authorization.openshift.io_v1_simple-nginx-nopv_system`:image-builders.yaml:
- Around line 8-25: Remove the cluster-volatile metadata from the golden
RoleBinding fixture: in the generated YAML for the system:image-builders
resource, drop creationTimestamp, managedFields, resourceVersion, and uid so the
export keeps only stable metadata like name and namespace. Keep the fixture
aligned with the export/golden generation path by updating the golden manifest
content rather than the runtime object construction.
In `@e2e-tests/testdata/test-850-duplicate-deployment-1.yaml`:
- Around line 17-18: The nginx container image in this duplicate-deployment
fixture is using a floating tag, which can make the E2E scenario drift over
time. Update the nginx image reference in this fixture to a fixed versioned tag,
matching the companion duplicate-deployment fixture so both inputs stay
consistent and deterministic.
In `@e2e-tests/tests/tier0/mta_807_data_validation_test.go`:
- Around line 142-149: The `has_scc` flag is being derived once from
`scenario.KubectlSrc.IsOpenShift()` and reused for both apps, which
misconfigures mixed-cluster scenarios. Update the setup in the tier0 test to
compute `has_scc` separately for `srcApp` and `tgtApp` using each side’s cluster
capability, and keep the `srcApp.ExtraVars` and `tgtApp.ExtraVars` assignments
aligned with their respective cluster checks. Reference the `isOCP` setup and
the `ExtraVars` maps in `mta_807_data_validation_test.go` to locate the change.
In `@e2e-tests/tests/tier0/mta_833_compatible_resources_live_test.go`:
- Around line 34-40: The test in mta_833_compatible_resources_live_test.go
creates a local tgtApp but still uses scenario.TgtApp in cleanup and validation,
which leaves tgtApp unused and breaks compilation. Update the test to
consistently use the local tgtApp variable throughout the affected
cleanup/validate paths, or remove the local if it is not needed, so the
non-admin target app selected in the setup is the one exercised by the live
test.
In `@internal/plugin/plugin_manager_helper.go`:
- Around line 106-110: The manifest filtering in `YamlToManifest` only trims
binaries for matching entries but still keeps versions that have no binary for
the current `runtime.GOOS/runtime.GOARCH`, which can cause
`cmd/plugin-manager/add/add.go` to select an incompatible newest semver first.
Update `YamlToManifest` (and the `isPluginAvailable` flow) so that any
`plugin.Versions[i]` without a matching current-platform `Binary` is excluded
entirely, ensuring only installable versions are returned.
---
Minor comments:
In `@cmd/validate/validate.go`:
- Around line 146-151: Populate report.ClusterContext in the live-mode fallback
path when --context is omitted. In validate.go’s live validation branch, set
report.ClusterContext to the current kubeconfig context before logging the
default-context message, so internal/validate/report.go can render the correct
context for saved reports. Keep the existing explicit --context handling in the
same conditional around o.configFlags.Context and report.ClusterContext.
In `@e2e-tests/golden-manifests-ocp/simple-nginx-nopv/output/output.yaml`:
- Around line 77-80: The aggregate ConfigMap fixture currently serializes
service-ca.crt as a block scalar, which adds a trailing newline and makes it
differ from the split ConfigMap fixture. Update the golden in output.yaml so the
service-ca.crt value matches the split resource fixture exactly, and keep the
aggregate and split representations semantically identical for the
simple-nginx-nopv ConfigMap__v1_simple-nginx-nopv_openshift-service-ca.crt
resource.
In `@e2e-tests/tests/tier0/mta_801_stateful_migration_test.go`:
- Line 15: The `pvc-transfer`-labeled MTA stateful migration spec now assumes
PVC transfer is always available, so confirm the CI contract or restore a skip
path in the `It("[MTA-801]...")` test before calling `runner.TransferPVC`.
Update the `mta_801_stateful_migration_test.go` flow to either explicitly skip
when PVC-transfer infrastructure is absent or ensure the e2e CI workflow applies
a label filter that excludes `pvc-transfer` specs unless support is provisioned.
Apply the same treatment to `mta_804_empty_pvc_migration_test.go` so both tests
follow the same availability contract.
In `@e2e-tests/tests/tier0/mta_828_instructions_file_migration_test.go`:
- Around line 147-148: The path assertion in the migration test is too broad:
the check in the kustomization resource validation is rejecting valid stage
paths like input/resources/... because it forbids any "resources/" substring.
Update the test around the resourcePath expectations to only reject the legacy
root-level resources/ prefix while still allowing input/resources/... entries,
keeping the existing input/ requirement in the same validation logic.
In `@e2e-tests/tests/tier0/mta_831_validate_compatible_resources_offline_test.go`:
- Around line 126-133: The offline validation test currently verifies
apiResourcesSource but only logs report.Mode, so it can miss a wrong mode being
emitted. In the mta_831_validate_compatible_resources_offline_test.go assertions
around the ValidationReport unmarshaling, add an explicit Expect on report.Mode
to equal offline alongside the existing report.APIResourcesSource checks. Keep
the assertion near the current report validation block so the test directly
fails if ValidationReport.Mode changes unexpectedly.
In `@e2e-tests/tests/tier1/mta_850_validate_duplicate_gvk_test.go`:
- Around line 42-45: The fixture lookup in the manifest loop is still dependent
on the process working directory because it builds paths with filepath.Abs on a
relative testdata path. Update the test to resolve testdata from the source file
location instead, using the existing testdata helper or runtime.Caller, so the
sourcePath computation in mta_850_validate_duplicate_gvk_test.go is stable
regardless of how the E2E runner invokes the test.
In `@e2e-tests/utils/utils.go`:
- Around line 1082-1094: Reject non-integral float64 values in ToInt64 instead
of truncating them; update the float64 branch to verify the value is a whole
number before converting. Use ToInt64 as the entry point and keep the existing
handling for json.Number, int64, and string unchanged, but return an error for
any float64 input with a fractional part so malformed JSON is surfaced.
In `@internal/plugin/plugin_manager_helper_test.go`:
- Around line 112-145: The positive fixture in
TestFilterPluginForOsArch_PersistsFilter only includes linux/darwin binaries, so
it can fail on the current runner even when FilterPluginForOsArch behaves
correctly. Update the test data in TestFilterPluginForOsArch_PersistsFilter to
include one Binary using runtime.GOOS and runtime.GOARCH as the matching case,
while keeping the other binaries as non-matches, so the post-filter assertions
remain valid across platforms.
In `@internal/transform/writer.go`:
- Around line 192-195: The whiteout footer text in
generateKustomizationWithComments is stale and still refers to resources/ even
though the generated kustomization now uses input/ entries. Update the whiteout
comment निर्माण in writer.go, near the whiteoutComments append logic in the
kustomization writer, so the footer consistently references input/ and matches
the emitted paths.
---
Nitpick comments:
In `@cmd/export/discover.go`:
- Around line 283-288: The Kubernetes discovery log branches in the `switch`
over `apierrors.IsForbidden`, `IsMethodNotSupported`, and `IsNotFound` should
include the API resource name and the original error text. Update the
`log.Debugf`/`log.Warnf` calls in `discover.go` to reference
`g.APIResource.Name` alongside `g.APIGroupVersion` and `g.APIResource.Kind`, and
append `err` so the failure context is preserved. This applies to the discovery
logging around the resource handling logic in `discover.go`.
In `@e2e-tests/framework/pipeline.go`:
- Around line 18-21: In RunCranePipeline, the preflight mismatch error is too
generic and does not identify which directory values differ. Update the
validation error to include the actual ExportDir, TransformDir, and ApplyDir
values from the e, t, and a options so callers can see the divergence directly.
Keep the check in RunCranePipeline and make the returned fmt.Errorf message
report the mismatched directory values clearly.
In `@e2e-tests/tests/tier0/mta_813_cronJob_PVC_test.go`:
- Line 15: The generic tier0 CI job is still picking up the PVC-transfer cronjob
spec because the workflow label filter only matches tier0, while this test now
has the pvc-transfer label and unconditionally calls runner.TransferPVC. Update
the Ginkgo label filter in the tier0 workflow to exclude pvc-transfer specs, or
move that exclusion into the specific job configuration so the cronjob PVC test
does not run where the crane binary lacks transfer-pvc support. Use the
workflow’s label_filter setting and the mta_813_cronJob_PVC_test.go spec label
as the key points to update.
In `@e2e-tests/tests/tier0/mta_837_hpa_migration_test.go`:
- Around line 75-90: The test is reapplying the rendered output after
RunCranePipelineWithChecks, which makes the spec validate a second apply instead
of the pipeline itself. Remove the extra ApplyOutputToTarget step and its
related logging from mta_837_hpa_migration_test.go, and keep the assertion
focused on the pipeline output already produced by RunCranePipelineWithChecks
and the HPA manifest check.
- Around line 24-28: The HPA migration test is duplicating expected values in
constants instead of asserting against the source HPA fixture. Update the test
logic in the migration flow to read minReplicas, maxReplicas, and CPU target
directly from hpaSrc and use those values in the assertions so the source HPA
becomes the single source of truth. Make the same change wherever those
duplicated expectations are used in the test, including the later assertion
block that compares the migrated HPA spec.
In `@e2e-tests/tests/tier0/olm_whiteout_base_test.go`:
- Around line 81-90: The OLM whiteout base test is re-applying manifests after
the pipeline already ran the apply stage, so it ends up validating a second
apply instead of the pipeline itself. Remove the extra apply step in the test
flow around RunCranePipelineWithChecks and ApplyOutputToTarget, keeping the
checks that verify the output directory contents after the pipeline completes.
🪄 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: 099347f7-8073-4250-8842-52e5293a826f
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (112)
.github/actions/run-crane-e2e-tier/action.yml.github/workflows/build-release-binaries.yml.github/workflows/e2e-pr-tester.yaml.gitignoreREADME.mdcmd/apply/apply.gocmd/apply/apply_test.gocmd/export/discover.gocmd/export/discover_test.gocmd/export/export.gocmd/export/export_test.gocmd/plugin-manager/add/add.gocmd/transform/optionals/optionals.gocmd/transform/optionals_test.gocmd/transform/transform.gocmd/transform/transform_test.gocmd/validate/validate.gocmd/validate/validate_test.godocs/kustomize-multistage.mddocs/transform.mde2e-tests/README.mde2e-tests/config/config.goe2e-tests/framework/app.goe2e-tests/framework/client.goe2e-tests/framework/crane.goe2e-tests/framework/kubectl.goe2e-tests/framework/pipeline.goe2e-tests/framework/scenario.goe2e-tests/framework/test_helpers.goe2e-tests/golden-manifests-ocp/simple-nginx-nopv/export/resources/simple-nginx-nopv/ConfigMap__v1_simple-nginx-nopv_kube-root-ca.crt.yamle2e-tests/golden-manifests-ocp/simple-nginx-nopv/export/resources/simple-nginx-nopv/ConfigMap__v1_simple-nginx-nopv_openshift-service-ca.crt.yamle2e-tests/golden-manifests-ocp/simple-nginx-nopv/export/resources/simple-nginx-nopv/Deployment_apps_v1_simple-nginx-nopv_simple-nginx-nopv-deployment.yamle2e-tests/golden-manifests-ocp/simple-nginx-nopv/export/resources/simple-nginx-nopv/EndpointSlice_discovery.k8s.io_v1_simple-nginx-nopv_my-simple-nginx-nopv-rf7lk.yamle2e-tests/golden-manifests-ocp/simple-nginx-nopv/export/resources/simple-nginx-nopv/Endpoints__v1_simple-nginx-nopv_my-simple-nginx-nopv.yamle2e-tests/golden-manifests-ocp/simple-nginx-nopv/export/resources/simple-nginx-nopv/ReplicaSet_apps_v1_simple-nginx-nopv_simple-nginx-nopv-deployment-866ff66bdd.yamle2e-tests/golden-manifests-ocp/simple-nginx-nopv/export/resources/simple-nginx-nopv/RoleBinding_authorization.openshift.io_v1_simple-nginx-nopv_system:deployers.yamle2e-tests/golden-manifests-ocp/simple-nginx-nopv/export/resources/simple-nginx-nopv/RoleBinding_authorization.openshift.io_v1_simple-nginx-nopv_system:image-builders.yamle2e-tests/golden-manifests-ocp/simple-nginx-nopv/export/resources/simple-nginx-nopv/RoleBinding_authorization.openshift.io_v1_simple-nginx-nopv_system:image-pullers.yamle2e-tests/golden-manifests-ocp/simple-nginx-nopv/export/resources/simple-nginx-nopv/RoleBinding_rbac.authorization.k8s.io_v1_simple-nginx-nopv_system:deployers.yamle2e-tests/golden-manifests-ocp/simple-nginx-nopv/export/resources/simple-nginx-nopv/RoleBinding_rbac.authorization.k8s.io_v1_simple-nginx-nopv_system:image-builders.yamle2e-tests/golden-manifests-ocp/simple-nginx-nopv/export/resources/simple-nginx-nopv/RoleBinding_rbac.authorization.k8s.io_v1_simple-nginx-nopv_system:image-pullers.yamle2e-tests/golden-manifests-ocp/simple-nginx-nopv/export/resources/simple-nginx-nopv/Secret__v1_simple-nginx-nopv_builder-dockercfg-n5zzt.yamle2e-tests/golden-manifests-ocp/simple-nginx-nopv/export/resources/simple-nginx-nopv/Secret__v1_simple-nginx-nopv_default-dockercfg-w99v8.yamle2e-tests/golden-manifests-ocp/simple-nginx-nopv/export/resources/simple-nginx-nopv/Secret__v1_simple-nginx-nopv_deployer-dockercfg-rgp2c.yamle2e-tests/golden-manifests-ocp/simple-nginx-nopv/export/resources/simple-nginx-nopv/ServiceAccount__v1_simple-nginx-nopv_builder.yamle2e-tests/golden-manifests-ocp/simple-nginx-nopv/export/resources/simple-nginx-nopv/ServiceAccount__v1_simple-nginx-nopv_default.yamle2e-tests/golden-manifests-ocp/simple-nginx-nopv/export/resources/simple-nginx-nopv/ServiceAccount__v1_simple-nginx-nopv_deployer.yamle2e-tests/golden-manifests-ocp/simple-nginx-nopv/export/resources/simple-nginx-nopv/Service__v1_simple-nginx-nopv_my-simple-nginx-nopv.yamle2e-tests/golden-manifests-ocp/simple-nginx-nopv/output/output.yamle2e-tests/golden-manifests-ocp/simple-nginx-nopv/output/resources/simple-nginx-nopv/ConfigMap__v1_simple-nginx-nopv_openshift-service-ca.crt.yamle2e-tests/golden-manifests-ocp/simple-nginx-nopv/output/resources/simple-nginx-nopv/Deployment_apps_v1_simple-nginx-nopv_simple-nginx-nopv-deployment.yamle2e-tests/golden-manifests-ocp/simple-nginx-nopv/output/resources/simple-nginx-nopv/RoleBinding_rbac.authorization.k8s.io_v1_simple-nginx-nopv_system:deployers.yamle2e-tests/golden-manifests-ocp/simple-nginx-nopv/output/resources/simple-nginx-nopv/RoleBinding_rbac.authorization.k8s.io_v1_simple-nginx-nopv_system:image-builders.yamle2e-tests/golden-manifests-ocp/simple-nginx-nopv/output/resources/simple-nginx-nopv/RoleBinding_rbac.authorization.k8s.io_v1_simple-nginx-nopv_system:image-pullers.yamle2e-tests/golden-manifests-ocp/simple-nginx-nopv/output/resources/simple-nginx-nopv/ServiceAccount__v1_simple-nginx-nopv_builder.yamle2e-tests/golden-manifests-ocp/simple-nginx-nopv/output/resources/simple-nginx-nopv/ServiceAccount__v1_simple-nginx-nopv_deployer.yamle2e-tests/golden-manifests-ocp/simple-nginx-nopv/output/resources/simple-nginx-nopv/Service__v1_simple-nginx-nopv_my-simple-nginx-nopv.yamle2e-tests/testdata/test-850-duplicate-deployment-1.yamle2e-tests/testdata/test-850-duplicate-deployment-2.yamle2e-tests/tests/tier0/e2e_suite_test.goe2e-tests/tests/tier0/mta_801_stateful_migration_test.goe2e-tests/tests/tier0/mta_802_ignored_resources_test.goe2e-tests/tests/tier0/mta_804_empty_pvc_migration_test.goe2e-tests/tests/tier0/mta_805_sets_test.goe2e-tests/tests/tier0/mta_806_pvc_data_integrity_test.goe2e-tests/tests/tier0/mta_807_data_validation_test.goe2e-tests/tests/tier0/mta_808_cronjob_quiesced_test.goe2e-tests/tests/tier0/mta_809_initcontainer_test.goe2e-tests/tests/tier0/mta_810_configmap_test.goe2e-tests/tests/tier0/mta_811_mongodb_non_admin_test.goe2e-tests/tests/tier0/mta_812_role_migration_test.goe2e-tests/tests/tier0/mta_813_cronJob_PVC_test.goe2e-tests/tests/tier0/mta_817_stateless_migration_test.goe2e-tests/tests/tier0/mta_827_custom_transformation_stage_test.goe2e-tests/tests/tier0/mta_828_instructions_file_migration_test.goe2e-tests/tests/tier0/mta_831_validate_compatible_resources_offline_test.goe2e-tests/tests/tier0/mta_833_compatible_resources_live_test.goe2e-tests/tests/tier0/mta_837_hpa_migration_test.goe2e-tests/tests/tier0/mta_844_validate_mixed_resources_live_test.goe2e-tests/tests/tier0/mta_845_validate_mixed_resources_offline_test.goe2e-tests/tests/tier0/mta_846_validate_single_incompatible_route_live_test.goe2e-tests/tests/tier0/olm_whiteout_base_test.goe2e-tests/tests/tier1/e2e_suite_test.goe2e-tests/tests/tier1/mta_829_validate_alternative_gv_suggestion_test.goe2e-tests/tests/tier1/mta_830_instructions_file_force_reconcile_test.goe2e-tests/tests/tier1/mta_832_validate_alternative_gv_suggestion_offline_test.goe2e-tests/tests/tier1/mta_836_validate_core_group_omitted_offline_test.goe2e-tests/tests/tier1/mta_849_validate_cluster_scoped_namespace_live_test.goe2e-tests/tests/tier1/mta_850_validate_duplicate_gvk_test.goe2e-tests/tests/tier1/olm_whiteout_auditability_test.goe2e-tests/utils/utils.goe2e-tests/utils/utils_test.gogo.modinternal/apply/kustomize.gointernal/apply/ordering_test.gointernal/buildinfo/buildinfo.gointernal/file/file_helper.gointernal/file/file_helper_test.gointernal/file/ordering.gointernal/file/ordering_test.gointernal/flags/help_groups.gointernal/plugin/plugin_manager_helper.gointernal/plugin/plugin_manager_helper_test.gointernal/transform/orchestrator.gointernal/transform/orchestrator_test.gointernal/transform/writer.gointernal/transform/writer_integration_test.gointernal/validate/api_resources.gointernal/validate/api_resources_test.gointernal/validate/matcher.gointernal/validate/matcher_test.gointernal/validate/scanner.go
Comment on lines
+202
to
+207
| - name: Run touched crane e2e tests | ||
| run: | | ||
| if [ "${{ needs.detect-changes.outputs.run_mode }}" = "all" ]; then | ||
| ginkgo run -v "$E2E_SUITE_DIR" -- $GINKGO_COMMON_ARGS | ||
| ginkgo run -v -r "$E2E_SUITE_DIR" -- $GINKGO_COMMON_ARGS | ||
| else | ||
| ginkgo run -v --focus-file="${{ needs.detect-changes.outputs.focus_file_regex }}" "$E2E_SUITE_DIR" -- $GINKGO_COMMON_ARGS | ||
| ginkgo run -v -r --focus-file="${{ needs.detect-changes.outputs.focus_file_regex }}" "$E2E_SUITE_DIR" -- $GINKGO_COMMON_ARGS |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
rg -n -B 5 -A 5 "focus_file_regex" .github/workflows/e2e-pr-tester.yamlRepository: migtools/mta-crane
Length of output: 3754
Stop inlining focus_file_regex into the shell script.
Line 207 interpolates ${{ needs.detect-changes.outputs.focus_file_regex }} directly inside a run: block. While Line 105 escapes regex metacharacters, it does not escape shell metacharacters like " or backticks. A crafted filename (e.g., file" && rm -rf / #) can break the quoted argument --focus-file="..." and execute arbitrary shell commands before Ginkgo starts.
Pass the value through the env: context to ensure it is treated as a literal variable by the shell.
Hardening diff
- name: Run touched crane e2e tests
+ env:
+ FOCUS_FILE_REGEX: ${{ needs.detect-changes.outputs.focus_file_regex }}
run: |
if [ "${{ needs.detect-changes.outputs.run_mode }}" = "all" ]; then
ginkgo run -v -r "$E2E_SUITE_DIR" -- $GINKGO_COMMON_ARGS
else
- ginkgo run -v -r --focus-file="${{ needs.detect-changes.outputs.focus_file_regex }}" "$E2E_SUITE_DIR" -- $GINKGO_COMMON_ARGS
+ ginkgo run -v -r --focus-file="$FOCUS_FILE_REGEX" "$E2E_SUITE_DIR" -- $GINKGO_COMMON_ARGS
fi📝 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
| - name: Run touched crane e2e tests | |
| run: | | |
| if [ "${{ needs.detect-changes.outputs.run_mode }}" = "all" ]; then | |
| ginkgo run -v "$E2E_SUITE_DIR" -- $GINKGO_COMMON_ARGS | |
| ginkgo run -v -r "$E2E_SUITE_DIR" -- $GINKGO_COMMON_ARGS | |
| else | |
| ginkgo run -v --focus-file="${{ needs.detect-changes.outputs.focus_file_regex }}" "$E2E_SUITE_DIR" -- $GINKGO_COMMON_ARGS | |
| ginkgo run -v -r --focus-file="${{ needs.detect-changes.outputs.focus_file_regex }}" "$E2E_SUITE_DIR" -- $GINKGO_COMMON_ARGS | |
| - name: Run touched crane e2e tests | |
| env: | |
| FOCUS_FILE_REGEX: ${{ needs.detect-changes.outputs.focus_file_regex }} | |
| run: | | |
| if [ "${{ needs.detect-changes.outputs.run_mode }}" = "all" ]; then | |
| ginkgo run -v -r "$E2E_SUITE_DIR" -- $GINKGO_COMMON_ARGS | |
| else | |
| ginkgo run -v -r --focus-file="$FOCUS_FILE_REGEX" "$E2E_SUITE_DIR" -- $GINKGO_COMMON_ARGS |
🧰 Tools
🪛 zizmor (1.26.1)
[info] 204-204: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
[info] 207-207: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
🤖 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/e2e-pr-tester.yaml around lines 202 - 207, The e2e
workflow’s `Run touched crane e2e tests` step is inlining
`needs.detect-changes.outputs.focus_file_regex` directly inside the shell
script, which can allow shell metacharacter injection. Move that value into the
step’s `env:` block and reference it as a shell variable in the `ginkgo run`
command, keeping the `--focus-file` argument literal and avoiding direct GitHub
expression interpolation inside `run:`.
Source: Linters/SAST tools
Comment on lines
+146
to
+153
| if _, err := os.Stat(outputDir); err == nil { | ||
| if !o.Overwrite { | ||
| return fmt.Errorf("output directory %q already exists; use --overwrite to replace it", outputDir) | ||
| } | ||
| if err := os.RemoveAll(outputDir); err != nil { | ||
| return fmt.Errorf("failed to clear output directory: %w", err) | ||
| } | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick win
Guard --overwrite from deleting the source transform tree.
This RemoveAll runs before stage resolution and without checking whether outputDir overlaps transformDir. A call like --output-dir ./transform --overwrite will recursively delete the input stages, and even ordinary validation failures after this point leave the previous output wiped. Reject overlapping paths and only replace the destination after the remaining validation succeeds.
🤖 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 146 - 153, The overwrite handling in apply
should not delete the transform source tree or wipe output before validation is
complete. In the apply flow around os.Stat, os.RemoveAll, and the
outputDir/transformDir setup, first resolve and compare the paths to reject any
overlap between outputDir and transformDir, then defer removing the destination
until after all remaining validation and stage resolution succeed. Keep the
guard in the apply logic so --overwrite only replaces a safe destination and
never the input stages.
Comment on lines
+57
to
+63
| if c != nil { | ||
| kubeconfigFlag := c.Flags().Lookup("kubeconfig") | ||
| if kubeconfigFlag == nil || !kubeconfigFlag.Changed { | ||
| emptyStr := "" | ||
| o.configFlags.KubeConfig = &emptyStr | ||
| } | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Don't discard kubeconfig values coming from config/Viper.
PreRun already unmarshals config into o.configFlags, but this block clears KubeConfig whenever the Cobra flag itself was not changed. That means a kubeconfig path supplied via the config file is silently ignored before RawConfig() and Namespace() run.
🤖 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/export/export.go` around lines 57 - 63, The export command’s kubeconfig
handling is overwriting config-derived values in `PreRun`, causing
`o.configFlags.KubeConfig` to be cleared whenever the Cobra flag was not
explicitly changed. Update the `PreRun` logic in the export flow to only set
`KubeConfig` from the flag when the user actually provided it, and otherwise
preserve the value already unmarshaled from config/Viper so `RawConfig()` and
`Namespace()` can use it correctly.
Comment on lines
+63
to
+77
| exportDir, err := filepath.Abs(o.ExportDir) | ||
| if err != nil { | ||
| return fmt.Errorf("resolving export-dir %q: %w", o.ExportDir, err) | ||
| } | ||
| o.ExportDir = exportDir | ||
| info, err := os.Stat(o.ExportDir) | ||
| if err != nil { | ||
| if os.IsNotExist(err) { | ||
| return fmt.Errorf("export-dir %q does not exist", o.ExportDir) | ||
| } | ||
| return fmt.Errorf("export-dir %q is not accessible: %v", o.ExportDir, err) | ||
| } | ||
| if !info.IsDir() { | ||
| return fmt.Errorf("export-dir %q is not a directory", o.ExportDir) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Don't require --export-dir for reruns that read from prior stage output.
RunMultiStage() can source a selected non-first stage from transform/<prev>/output, so this unconditional os.Stat now rejects valid reruns once the original export tree has been cleaned up. Please defer the existence check until you know the first selected stage actually needs export input.
🤖 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 63 - 77, `validateOptions` in
`cmd/transform/transform.go` is unconditionally requiring `--export-dir` to
exist, which breaks reruns that rely on `RunMultiStage()` reading from a prior
stage output instead of the original export tree. Update the `o.ExportDir`
handling so the `filepath.Abs` normalization stays, but defer the `os.Stat`
existence/accessibility check until you know the first selected stage actually
needs export input, and skip that check for reruns that source
`transform/<prev>/output`.
Comment on lines
+412
to
+519
| func TestValidate_HelpGroupsFlags(t *testing.T) { | ||
| streams, _, outBuf, _ := genericclioptions.NewTestIOStreams() | ||
| cmd := NewValidateCommand(streams, nil) | ||
| cmd.SetOut(outBuf) | ||
| cmd.SetErr(outBuf) | ||
| cmd.SetArgs([]string{"--help"}) | ||
| if err := cmd.Execute(); err != nil { | ||
| t.Fatalf("unexpected error running --help: %v", err) | ||
| } | ||
|
|
||
| help := outBuf.String() | ||
|
|
||
| commandSpecificHeader := "Command-specific Flags:" | ||
| kubeHeader := "Inherited Kubernetes Client Flags:" | ||
|
|
||
| commandSpecificIdx := strings.Index(help, commandSpecificHeader) | ||
| if commandSpecificIdx == -1 { | ||
| t.Fatalf("missing %q section in help output:\n%s", commandSpecificHeader, help) | ||
| } | ||
|
|
||
| kubeIdx := strings.Index(help, kubeHeader) | ||
| if kubeIdx == -1 { | ||
| t.Fatalf("missing %q section in help output:\n%s", kubeHeader, help) | ||
| } | ||
|
|
||
| if commandSpecificIdx > kubeIdx { | ||
| t.Fatalf("expected command-specific section before inherited kube/client section") | ||
| } | ||
|
|
||
| commandSpecificSection := help[commandSpecificIdx:kubeIdx] | ||
| kubeSection := help[kubeIdx:] | ||
|
|
||
| if !strings.Contains(commandSpecificSection, "--validate-dir") { | ||
| t.Fatalf("expected --validate-dir in command-specific section, got:\n%s", commandSpecificSection) | ||
| } | ||
|
|
||
| if strings.Contains(commandSpecificSection, "--as-uid") { | ||
| t.Fatalf("did not expect --as-uid in command-specific section, got:\n%s", commandSpecificSection) | ||
| } | ||
|
|
||
| if !strings.Contains(kubeSection, "--as-uid") { | ||
| t.Fatalf("expected --as-uid in inherited kube/client section, got:\n%s", kubeSection) | ||
| } | ||
| } | ||
|
|
||
| func TestComplete_InvalidContextFailsBeforeRun(t *testing.T) { | ||
| ctx := "nonexistent-context-that-does-not-exist" | ||
| cf := genericclioptions.NewConfigFlags(true) | ||
| cf.Context = &ctx | ||
|
|
||
| // Use a temp kubeconfig so the test is environment-independent | ||
| kc := filepath.Join(t.TempDir(), "kubeconfig") | ||
| kubeconfig := `apiVersion: v1 | ||
| kind: Config | ||
| clusters: | ||
| - name: local | ||
| cluster: | ||
| server: https://127.0.0.1:6443 | ||
| users: | ||
| - name: local-user | ||
| user: | ||
| token: fake | ||
| contexts: | ||
| - name: existing-context | ||
| context: | ||
| cluster: local | ||
| user: local-user | ||
| current-context: existing-context | ||
| ` | ||
| if err := os.WriteFile(kc, []byte(kubeconfig), 0600); err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| cf.KubeConfig = &kc | ||
|
|
||
| o := &ValidateOptions{ | ||
| configFlags: cf, | ||
| globalFlags: &flags.GlobalFlags{}, | ||
| inputDir: t.TempDir(), | ||
| outputFormat: "json", | ||
| } | ||
|
|
||
| cmd := &cobra.Command{} | ||
| err := o.Complete(cmd, nil) | ||
|
|
||
| if err == nil { | ||
| t.Fatal("Complete() should fail for nonexistent context, but got nil") | ||
| } | ||
| if !strings.Contains(err.Error(), "nonexistent-context-that-does-not-exist") { | ||
| t.Fatalf("error should mention the invalid context name, got: %v", err) | ||
| } | ||
| } | ||
|
|
||
| func TestComplete_SkippedInOfflineMode(t *testing.T) { | ||
| o := &ValidateOptions{ | ||
| configFlags: genericclioptions.NewConfigFlags(true), | ||
| globalFlags: &flags.GlobalFlags{}, | ||
| inputDir: t.TempDir(), | ||
| outputFormat: "json", | ||
| apiResourcesFile: "/some/file.json", | ||
| } | ||
|
|
||
| cmd := &cobra.Command{} | ||
| err := o.Complete(cmd, nil) | ||
|
|
||
| if err != nil { | ||
| t.Fatalf("Complete() should skip in offline mode, got: %v", err) | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Add direct coverage for --overwrite validate-dir handling.
This file exercises the new flag parsing and Complete() behavior, but it still doesn't cover the new Run() branch that errors on an existing validate directory unless --overwrite is set, nor the removal/recreate path when it is set. That's the highest-risk part of this CLI change.
As per coding guidelines, "All new features require tests".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmd/validate/validate_test.go` around lines 412 - 519, Add test coverage in
TestValidate_HelpGroupsFlags/ValidateOptions-related tests for the new Run()
validate-dir overwrite behavior: verify the command fails when the validate
directory already exists and --overwrite is not set, and verify it succeeds by
removing/recreating the directory when --overwrite is enabled. Use the existing
ValidateOptions, NewValidateCommand, and Run/Complete flow to locate the branch
being exercised, and assert both the error path and the overwrite path directly.
Source: Coding guidelines
Comment on lines
+8
to
+25
| creationTimestamp: "2026-06-25T05:47:04Z" | ||
| managedFields: | ||
| - apiVersion: rbac.authorization.k8s.io/v1 | ||
| fieldsType: FieldsV1 | ||
| fieldsV1: | ||
| f:metadata: | ||
| f:annotations: | ||
| .: {} | ||
| f:openshift.io/description: {} | ||
| f:roleRef: {} | ||
| f:subjects: {} | ||
| manager: openshift-controller-manager | ||
| operation: Update | ||
| time: "2026-06-25T05:47:03Z" | ||
| name: system:image-builders | ||
| namespace: simple-nginx-nopv | ||
| resourceVersion: "44847" | ||
| uid: 2ec850be-14c2-43e7-a412-997d8d96e598 |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Remove cluster-volatile metadata from the golden fixture.
creationTimestamp, managedFields, resourceVersion, and uid are cluster/run-specific, so raw golden comparisons can fail on the next OpenShift export. Keep only stable metadata needed for the fixture.
Suggested fix
- creationTimestamp: "2026-06-25T05:47:04Z"
- managedFields:
- - apiVersion: rbac.authorization.k8s.io/v1
- fieldsType: FieldsV1
- fieldsV1:
- f:metadata:
- f:annotations:
- .: {}
- f:openshift.io/description: {}
- f:roleRef: {}
- f:subjects: {}
- manager: openshift-controller-manager
- operation: Update
- time: "2026-06-25T05:47:03Z"
name: system:image-builders
namespace: simple-nginx-nopv
- resourceVersion: "44847"
- uid: 2ec850be-14c2-43e7-a412-997d8d96e598📝 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
| creationTimestamp: "2026-06-25T05:47:04Z" | |
| managedFields: | |
| - apiVersion: rbac.authorization.k8s.io/v1 | |
| fieldsType: FieldsV1 | |
| fieldsV1: | |
| f:metadata: | |
| f:annotations: | |
| .: {} | |
| f:openshift.io/description: {} | |
| f:roleRef: {} | |
| f:subjects: {} | |
| manager: openshift-controller-manager | |
| operation: Update | |
| time: "2026-06-25T05:47:03Z" | |
| name: system:image-builders | |
| namespace: simple-nginx-nopv | |
| resourceVersion: "44847" | |
| uid: 2ec850be-14c2-43e7-a412-997d8d96e598 | |
| name: system:image-builders | |
| namespace: simple-nginx-nopv |
🤖 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/golden-manifests-ocp/simple-nginx-nopv/export/resources/simple-nginx-nopv/RoleBinding_authorization.openshift.io_v1_simple-nginx-nopv_system`:image-builders.yaml
around lines 8 - 25, Remove the cluster-volatile metadata from the golden
RoleBinding fixture: in the generated YAML for the system:image-builders
resource, drop creationTimestamp, managedFields, resourceVersion, and uid so the
export keeps only stable metadata like name and namespace. Keep the fixture
aligned with the export/golden generation path by updating the golden manifest
content rather than the runtime object construction.
Comment on lines
+17
to
+18
| - name: nginx | ||
| image: nginx:latest |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Pin the image tag in this fixture.
nginx:latest makes this E2E input drift as upstream images change, so the duplicate-resource test can fail for reasons unrelated to the scenario. Use a fixed tag here, ideally the same one as the companion fixture.
🤖 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/testdata/test-850-duplicate-deployment-1.yaml` around lines 17 -
18, The nginx container image in this duplicate-deployment fixture is using a
floating tag, which can make the E2E scenario drift over time. Update the nginx
image reference in this fixture to a fixed versioned tag, matching the companion
duplicate-deployment fixture so both inputs stay consistent and deterministic.
Comment on lines
+142
to
+149
| isOCP := scenario.KubectlSrc.IsOpenShift() | ||
| srcApp.ExtraVars = map[string]any{ | ||
| "non_admin_user": "true", | ||
| "has_scc": isOCP, | ||
| } | ||
| tgtApp.ExtraVars = map[string]any{ | ||
| "non_admin_user": "true", | ||
| "has_scc": isOCP, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Compute has_scc per cluster instead of reusing the source value.
Line 142 reads OpenShift capability from scenario.KubectlSrc, then applies that same value to both srcApp and tgtApp. In mixed-cluster runs this misconfigures one side: Kubernetes→OpenShift never enables SCC handling on the target, and OpenShift→Kubernetes enables it when it should stay off.
Suggested fix
- isOCP := scenario.KubectlSrc.IsOpenShift()
+ srcIsOCP := scenario.KubectlSrc.IsOpenShift()
+ tgtIsOCP := scenario.KubectlTgt.IsOpenShift()
srcApp.ExtraVars = map[string]any{
"non_admin_user": "true",
- "has_scc": isOCP,
+ "has_scc": srcIsOCP,
}
tgtApp.ExtraVars = map[string]any{
"non_admin_user": "true",
- "has_scc": isOCP,
+ "has_scc": tgtIsOCP,
}📝 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
| isOCP := scenario.KubectlSrc.IsOpenShift() | |
| srcApp.ExtraVars = map[string]any{ | |
| "non_admin_user": "true", | |
| "has_scc": isOCP, | |
| } | |
| tgtApp.ExtraVars = map[string]any{ | |
| "non_admin_user": "true", | |
| "has_scc": isOCP, | |
| srcIsOCP := scenario.KubectlSrc.IsOpenShift() | |
| tgtIsOCP := scenario.KubectlTgt.IsOpenShift() | |
| srcApp.ExtraVars = map[string]any{ | |
| "non_admin_user": "true", | |
| "has_scc": srcIsOCP, | |
| } | |
| tgtApp.ExtraVars = map[string]any{ | |
| "non_admin_user": "true", | |
| "has_scc": tgtIsOCP, | |
| } |
🤖 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/tier0/mta_807_data_validation_test.go` around lines 142 -
149, The `has_scc` flag is being derived once from
`scenario.KubectlSrc.IsOpenShift()` and reused for both apps, which
misconfigures mixed-cluster scenarios. Update the setup in the tier0 test to
compute `has_scc` separately for `srcApp` and `tgtApp` using each side’s cluster
capability, and keep the `srcApp.ExtraVars` and `tgtApp.ExtraVars` assignments
aligned with their respective cluster checks. Reference the `isOCP` setup and
the `ExtraVars` maps in `mta_807_data_validation_test.go` to locate the change.
Comment on lines
+34
to
+40
| srcApp := scenario.SrcAppNonAdmin | ||
| tgtApp := scenario.TgtAppNonAdmin | ||
| runner := scenario.CraneNonAdmin | ||
| srcApp.ExtraVars = map[string]any{ | ||
| "non_admin_user": "true", | ||
| } | ||
| tgtApp.ExtraVars = srcApp.ExtraVars |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Use tgtApp consistently or drop it; this file does not compile.
Line 35 introduces tgtApp, but the test still uses scenario.TgtApp for cleanup and validate, so the local is unused and go build ./... fails. That also means the live validation path is still pointed at the admin target app/context instead of the non-admin target selected above.
Suggested fix
- tgtApp := scenario.TgtAppNonAdmin
+ tgtApp := scenario.TgtAppNonAdmin
...
- if err := CleanupScenario(paths.TempDir, srcApp, scenario.TgtApp); err != nil {
+ if err := CleanupScenario(paths.TempDir, srcApp, tgtApp); err != nil {
log.Printf("cleanup: %v", err)
}
})
...
stdout, err := runner.Validate(ValidateOptions{
- Context: scenario.TgtApp.Context,
+ Context: tgtApp.Context,
InputDir: paths.OutputDir,
ValidateDir: validateDir,
})As per coding guidelines, **/*.go: Code must compile successfully: go build ./....
Also applies to: 66-71, 97-101
🤖 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/tier0/mta_833_compatible_resources_live_test.go` around lines
34 - 40, The test in mta_833_compatible_resources_live_test.go creates a local
tgtApp but still uses scenario.TgtApp in cleanup and validation, which leaves
tgtApp unused and breaks compilation. Update the test to consistently use the
local tgtApp variable throughout the affected cleanup/validate paths, or remove
the local if it is not needed, so the non-admin target app selected in the setup
is the one exercised by the live test.
Source: Coding guidelines
Comment on lines
+106
to
+110
| for i := range plugin.Versions { | ||
| for _, binary := range plugin.Versions[i].Binaries { | ||
| if binary.OS == runtime.GOOS && binary.Arch == runtime.GOARCH { | ||
| isPluginAvailable = true | ||
| version.Binaries = []Binary{ | ||
| binary, | ||
| } | ||
| plugin.Versions[i].Binaries = []Binary{binary} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Drop versions that have no current-platform binary.
Line 106 only narrows Binaries on matching entries; incompatible versions are still returned from YamlToManifest. That leaks into cmd/plugin-manager/add/add.go, where the no---version path picks the highest semver first, so a newer release without a runtime.GOOS/runtime.GOARCH binary can now block installing an older compatible release.
Suggested fix
func FilterPluginForOsArch(plugin *Plugin) bool {
- isPluginAvailable := false
- for i := range plugin.Versions {
- for _, binary := range plugin.Versions[i].Binaries {
- if binary.OS == runtime.GOOS && binary.Arch == runtime.GOARCH {
- isPluginAvailable = true
- plugin.Versions[i].Binaries = []Binary{binary}
- break
- }
- }
- }
- return isPluginAvailable
+ filtered := plugin.Versions[:0]
+ for _, version := range plugin.Versions {
+ for _, binary := range version.Binaries {
+ if binary.OS == runtime.GOOS && binary.Arch == runtime.GOARCH {
+ version.Binaries = []Binary{binary}
+ filtered = append(filtered, version)
+ break
+ }
+ }
+ }
+ plugin.Versions = filtered
+ return len(filtered) > 0
}📝 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 i := range plugin.Versions { | |
| for _, binary := range plugin.Versions[i].Binaries { | |
| if binary.OS == runtime.GOOS && binary.Arch == runtime.GOARCH { | |
| isPluginAvailable = true | |
| version.Binaries = []Binary{ | |
| binary, | |
| } | |
| plugin.Versions[i].Binaries = []Binary{binary} | |
| func FilterPluginForOsArch(plugin *Plugin) bool { | |
| filtered := plugin.Versions[:0] | |
| for _, version := range plugin.Versions { | |
| for _, binary := range version.Binaries { | |
| if binary.OS == runtime.GOOS && binary.Arch == runtime.GOARCH { | |
| version.Binaries = []Binary{binary} | |
| filtered = append(filtered, version) | |
| break | |
| } | |
| } | |
| } | |
| plugin.Versions = filtered | |
| return len(filtered) > 0 | |
| } |
🤖 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/plugin/plugin_manager_helper.go` around lines 106 - 110, The
manifest filtering in `YamlToManifest` only trims binaries for matching entries
but still keeps versions that have no binary for the current
`runtime.GOOS/runtime.GOARCH`, which can cause `cmd/plugin-manager/add/add.go`
to select an incompatible newest semver first. Update `YamlToManifest` (and the
`isPluginAvailable` flow) so that any `plugin.Versions[i]` without a matching
current-platform `Binary` is excluded entirely, ensuring only installable
versions are returned.
Signed-off-by: M Sajid Mansoori <mmansoor@redhat.com>
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
7 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
input/andoutput/stage layout.