🌱 Remove defunct --export-dir flag from apply command - #692
Merged
Tamar-Dinavetsky merged 3 commits intoJul 27, 2026
Merged
Conversation
|
Warning Review limit reached
Next review available in: 36 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?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 reviews. How do review 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 refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (46)
📝 WalkthroughWalkthroughThe apply command no longer accepts ChangesApply and transform option wiring
Same-cluster PVC transfer handling
Deterministic end-to-end fixtures
Changed-test workflow
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
e2e-tests/framework/pipeline.go (1)
21-22: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd regression coverage for the independent directory checks.
Please verify tests cover both rejected mismatches and the valid case where export and apply use different directory fields. As per coding guidelines, “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/framework/pipeline.go` around lines 21 - 22, Expand regression coverage for the directory validation around the pipeline check: add tests that reject an export-directory mismatch and a transform-directory mismatch, plus a valid case where export and apply intentionally use different directory fields. Anchor the tests to the pipeline validation entry point and assert both the error and successful result.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@e2e-tests/framework/pipeline.go`:
- Around line 21-22: Expand regression coverage for the directory validation
around the pipeline check: add tests that reject an export-directory mismatch
and a transform-directory mismatch, plus a valid case where export and apply
intentionally use different directory fields. Anchor the tests to the pipeline
validation entry point and assert both the error and successful result.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b0a8e302-2122-41ff-af8b-3b2c7ba30f96
📒 Files selected for processing (45)
cmd/apply/apply.goe2e-tests/framework/crane.goe2e-tests/framework/pipeline.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_838_multi_container_stateless_migration_test.goe2e-tests/tests/tier0/mta_839_networkpolicy_migration_test.goe2e-tests/tests/tier0/mta_840_same_cluster_migration_test.goe2e-tests/tests/tier0/mta_843_pipeline_determinism_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_851_no_cluster_resources_test.goe2e-tests/tests/tier0/mta_852_minimal_rbac_test.goe2e-tests/tests/tier0/mta_853_split_apply_test.goe2e-tests/tests/tier0/mta_862_ordered_apply_test.goe2e-tests/tests/tier0/olm_whiteout_base_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_841_missing_configmap_ref_test.goe2e-tests/tests/tier1/mta_842_secrets_migration_test.goe2e-tests/tests/tier1/mta_854_two_clusterroles_test.goe2e-tests/tests/tier1/mta_855_crd_custom_resource_test.goe2e-tests/tests/tier1/mta_857_service_account_subject_test.goe2e-tests/tests/tier1/mta_858_user_subject_test.goe2e-tests/tests/tier1/mta_859_group_subject_test.goe2e-tests/tests/tier1/mta_860_validate_malformed_api_surface_test.goe2e-tests/tests/tier1/olm_whiteout_auditability_test.go
💤 Files with no reviewable changes (4)
- cmd/apply/apply.go
- e2e-tests/tests/tier0/mta_862_ordered_apply_test.go
- e2e-tests/tests/tier1/mta_860_validate_malformed_api_surface_test.go
- e2e-tests/framework/crane.go
Contributor
Author
|
/rfr |
aufi
reviewed
Jul 27, 2026
aufi
left a comment
aufi
left a comment
Contributor
There was a problem hiding this comment.
Overall looks good, thanks for the PR! Please update also docs e.g. https://github.com/migtools/crane/blob/main/docs/commands/apply.md
Tamar-Dinavetsky
force-pushed
the
refactor/remove-export-dir-flag-from-apply
branch
2 times, most recently
from
July 27, 2026 10:35
f3a3528 to
7720bdd
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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:
- Line 207: Update the ginkgo invocation in the E2E workflow to stop
interpolating the PR-controlled focus_file_regex GitHub expression directly into
the shell command. Pass the value through the step’s env configuration and
reference it only via a properly quoted shell variable, preserving the existing
--focus-file behavior.
In `@cmd/transfer-pvc/transfer-pvc.go`:
- Around line 373-375: Update the source certificate secret creation flow around
srcClient.Create to handle an existing secret by retrieving and updating it, or
otherwise using the repository’s established upsert behavior, instead of
aborting on AlreadyExists. Preserve creation for missing secrets, and add a
fake-client regression test covering a pre-existing source secret and successful
retry.
- Around line 233-234: Update the same-cluster validation error in the transfer
flow to use grammatically correct wording and include both context names plus
t.sourceContext.Cluster. Preserve the existing validation condition and error
behavior while making the message clearly identify the conflicting source and
destination configuration.
In `@cmd/transform/transform.go`:
- Line 46: Add tests for the new IgnoredPatchesDir configuration and
--ignored-patches-dir CLI parsing, then cover the conflict path to verify
ignored patches are written beneath the configured directory using the ignored-
filename prefix. Reuse the existing transform and conflict-output test helpers
and preserve current behavior when the option is unset.
🪄 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 Plus
Run ID: 5f94ecbf-4422-4fd1-9df5-b48fe1ebfe81
📒 Files selected for processing (61)
.github/workflows/e2e-pr-tester.yamlcmd/apply/apply.gocmd/transfer-pvc/transfer-pvc.gocmd/transfer-pvc/transfer-pvc_test.gocmd/transform/transform.godocs/commands/apply.mde2e-tests/framework/crane.goe2e-tests/framework/pipeline.goe2e-tests/golden-manifests-ocp/simple-nginx-nopv/export/resources/simple-nginx-nopv/Route_route.openshift.io_v1_simple-nginx-nopv_my-simple-nginx-nopv.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/Route_route.openshift.io_v1_simple-nginx-nopv_my-simple-nginx-nopv.yamle2e-tests/golden-manifests-ocp/simple-nginx-nopv/output/resources/simple-nginx-nopv/Service__v1_simple-nginx-nopv_my-simple-nginx-nopv.yamle2e-tests/golden-manifests/simple-nginx-nopv/export/resources/simple-nginx-nopv/Service__v1_simple-nginx-nopv_my-simple-nginx-nopv.yamle2e-tests/golden-manifests/simple-nginx-nopv/output/output.yamle2e-tests/golden-manifests/simple-nginx-nopv/output/resources/simple-nginx-nopv/Service__v1_simple-nginx-nopv_my-simple-nginx-nopv.yamle2e-tests/testdata/basic-instructions-file-ocp.yamle2e-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_838_multi_container_stateless_migration_test.goe2e-tests/tests/tier0/mta_839_networkpolicy_migration_test.goe2e-tests/tests/tier0/mta_840_same_cluster_migration_test.goe2e-tests/tests/tier0/mta_843_pipeline_determinism_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_851_no_cluster_resources_test.goe2e-tests/tests/tier0/mta_852_minimal_rbac_test.goe2e-tests/tests/tier0/mta_853_split_apply_test.goe2e-tests/tests/tier0/mta_862_ordered_apply_test.goe2e-tests/tests/tier0/olm_whiteout_base_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_841_missing_configmap_ref_test.goe2e-tests/tests/tier1/mta_842_secrets_migration_test.goe2e-tests/tests/tier1/mta_854_two_clusterroles_test.goe2e-tests/tests/tier1/mta_855_crd_custom_resource_test.goe2e-tests/tests/tier1/mta_857_service_account_subject_test.goe2e-tests/tests/tier1/mta_858_user_subject_test.goe2e-tests/tests/tier1/mta_859_group_subject_test.goe2e-tests/tests/tier1/mta_860_validate_malformed_api_surface_test.goe2e-tests/tests/tier1/olm_whiteout_auditability_test.goe2e-tests/utils/utils.gointernal/file/file_helper.go
💤 Files with no reviewable changes (8)
- e2e-tests/testdata/basic-instructions-file-ocp.yaml
- e2e-tests/golden-manifests-ocp/simple-nginx-nopv/output/resources/simple-nginx-nopv/Route_route.openshift.io_v1_simple-nginx-nopv_my-simple-nginx-nopv.yaml
- e2e-tests/golden-manifests-ocp/simple-nginx-nopv/export/resources/simple-nginx-nopv/Route_route.openshift.io_v1_simple-nginx-nopv_my-simple-nginx-nopv.yaml
- cmd/apply/apply.go
- e2e-tests/tests/tier0/mta_862_ordered_apply_test.go
- e2e-tests/tests/tier1/mta_860_validate_malformed_api_surface_test.go
- cmd/transfer-pvc/transfer-pvc_test.go
- e2e-tests/utils/utils.go
🚧 Files skipped from review as they are similar to previous changes (33)
- e2e-tests/tests/tier0/mta_852_minimal_rbac_test.go
- e2e-tests/tests/tier1/mta_854_two_clusterroles_test.go
- e2e-tests/tests/tier1/mta_841_missing_configmap_ref_test.go
- e2e-tests/tests/tier0/mta_801_stateful_migration_test.go
- e2e-tests/tests/tier0/mta_827_custom_transformation_stage_test.go
- e2e-tests/tests/tier0/mta_851_no_cluster_resources_test.go
- e2e-tests/tests/tier1/mta_836_validate_core_group_omitted_offline_test.go
- e2e-tests/tests/tier0/mta_802_ignored_resources_test.go
- e2e-tests/tests/tier0/mta_853_split_apply_test.go
- e2e-tests/tests/tier1/mta_857_service_account_subject_test.go
- e2e-tests/tests/tier0/mta_845_validate_mixed_resources_offline_test.go
- e2e-tests/tests/tier1/mta_858_user_subject_test.go
- e2e-tests/tests/tier1/mta_829_validate_alternative_gv_suggestion_test.go
- e2e-tests/tests/tier1/mta_859_group_subject_test.go
- e2e-tests/tests/tier0/mta_809_initcontainer_test.go
- e2e-tests/tests/tier0/mta_817_stateless_migration_test.go
- e2e-tests/tests/tier0/olm_whiteout_base_test.go
- e2e-tests/tests/tier0/mta_807_data_validation_test.go
- e2e-tests/tests/tier0/mta_804_empty_pvc_migration_test.go
- e2e-tests/tests/tier1/mta_842_secrets_migration_test.go
- e2e-tests/tests/tier0/mta_806_pvc_data_integrity_test.go
- e2e-tests/tests/tier0/mta_840_same_cluster_migration_test.go
- e2e-tests/tests/tier1/mta_832_validate_alternative_gv_suggestion_offline_test.go
- e2e-tests/framework/pipeline.go
- e2e-tests/tests/tier0/mta_808_cronjob_quiesced_test.go
- e2e-tests/tests/tier0/mta_812_role_migration_test.go
- e2e-tests/tests/tier1/olm_whiteout_auditability_test.go
- e2e-tests/tests/tier0/mta_813_cronJob_PVC_test.go
- e2e-tests/tests/tier0/mta_838_multi_container_stateless_migration_test.go
- e2e-tests/tests/tier0/mta_837_hpa_migration_test.go
- e2e-tests/tests/tier0/mta_843_pipeline_determinism_test.go
- e2e-tests/framework/crane.go
- e2e-tests/tests/tier0/mta_811_mongodb_non_admin_test.go
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 4
🤖 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:
- Line 207: Update the ginkgo invocation in the E2E workflow to stop
interpolating the PR-controlled focus_file_regex GitHub expression directly into
the shell command. Pass the value through the step’s env configuration and
reference it only via a properly quoted shell variable, preserving the existing
--focus-file behavior.
In `@cmd/transfer-pvc/transfer-pvc.go`:
- Around line 373-375: Update the source certificate secret creation flow around
srcClient.Create to handle an existing secret by retrieving and updating it, or
otherwise using the repository’s established upsert behavior, instead of
aborting on AlreadyExists. Preserve creation for missing secrets, and add a
fake-client regression test covering a pre-existing source secret and successful
retry.
- Around line 233-234: Update the same-cluster validation error in the transfer
flow to use grammatically correct wording and include both context names plus
t.sourceContext.Cluster. Preserve the existing validation condition and error
behavior while making the message clearly identify the conflicting source and
destination configuration.
In `@cmd/transform/transform.go`:
- Line 46: Add tests for the new IgnoredPatchesDir configuration and
--ignored-patches-dir CLI parsing, then cover the conflict path to verify
ignored patches are written beneath the configured directory using the ignored-
filename prefix. Reuse the existing transform and conflict-output test helpers
and preserve current behavior when the option is unset.
🪄 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 Plus
Run ID: 5f94ecbf-4422-4fd1-9df5-b48fe1ebfe81
📒 Files selected for processing (61)
.github/workflows/e2e-pr-tester.yamlcmd/apply/apply.gocmd/transfer-pvc/transfer-pvc.gocmd/transfer-pvc/transfer-pvc_test.gocmd/transform/transform.godocs/commands/apply.mde2e-tests/framework/crane.goe2e-tests/framework/pipeline.goe2e-tests/golden-manifests-ocp/simple-nginx-nopv/export/resources/simple-nginx-nopv/Route_route.openshift.io_v1_simple-nginx-nopv_my-simple-nginx-nopv.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/Route_route.openshift.io_v1_simple-nginx-nopv_my-simple-nginx-nopv.yamle2e-tests/golden-manifests-ocp/simple-nginx-nopv/output/resources/simple-nginx-nopv/Service__v1_simple-nginx-nopv_my-simple-nginx-nopv.yamle2e-tests/golden-manifests/simple-nginx-nopv/export/resources/simple-nginx-nopv/Service__v1_simple-nginx-nopv_my-simple-nginx-nopv.yamle2e-tests/golden-manifests/simple-nginx-nopv/output/output.yamle2e-tests/golden-manifests/simple-nginx-nopv/output/resources/simple-nginx-nopv/Service__v1_simple-nginx-nopv_my-simple-nginx-nopv.yamle2e-tests/testdata/basic-instructions-file-ocp.yamle2e-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_838_multi_container_stateless_migration_test.goe2e-tests/tests/tier0/mta_839_networkpolicy_migration_test.goe2e-tests/tests/tier0/mta_840_same_cluster_migration_test.goe2e-tests/tests/tier0/mta_843_pipeline_determinism_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_851_no_cluster_resources_test.goe2e-tests/tests/tier0/mta_852_minimal_rbac_test.goe2e-tests/tests/tier0/mta_853_split_apply_test.goe2e-tests/tests/tier0/mta_862_ordered_apply_test.goe2e-tests/tests/tier0/olm_whiteout_base_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_841_missing_configmap_ref_test.goe2e-tests/tests/tier1/mta_842_secrets_migration_test.goe2e-tests/tests/tier1/mta_854_two_clusterroles_test.goe2e-tests/tests/tier1/mta_855_crd_custom_resource_test.goe2e-tests/tests/tier1/mta_857_service_account_subject_test.goe2e-tests/tests/tier1/mta_858_user_subject_test.goe2e-tests/tests/tier1/mta_859_group_subject_test.goe2e-tests/tests/tier1/mta_860_validate_malformed_api_surface_test.goe2e-tests/tests/tier1/olm_whiteout_auditability_test.goe2e-tests/utils/utils.gointernal/file/file_helper.go
💤 Files with no reviewable changes (8)
- e2e-tests/testdata/basic-instructions-file-ocp.yaml
- e2e-tests/golden-manifests-ocp/simple-nginx-nopv/output/resources/simple-nginx-nopv/Route_route.openshift.io_v1_simple-nginx-nopv_my-simple-nginx-nopv.yaml
- e2e-tests/golden-manifests-ocp/simple-nginx-nopv/export/resources/simple-nginx-nopv/Route_route.openshift.io_v1_simple-nginx-nopv_my-simple-nginx-nopv.yaml
- cmd/apply/apply.go
- e2e-tests/tests/tier0/mta_862_ordered_apply_test.go
- e2e-tests/tests/tier1/mta_860_validate_malformed_api_surface_test.go
- cmd/transfer-pvc/transfer-pvc_test.go
- e2e-tests/utils/utils.go
🚧 Files skipped from review as they are similar to previous changes (33)
- e2e-tests/tests/tier0/mta_852_minimal_rbac_test.go
- e2e-tests/tests/tier1/mta_854_two_clusterroles_test.go
- e2e-tests/tests/tier1/mta_841_missing_configmap_ref_test.go
- e2e-tests/tests/tier0/mta_801_stateful_migration_test.go
- e2e-tests/tests/tier0/mta_827_custom_transformation_stage_test.go
- e2e-tests/tests/tier0/mta_851_no_cluster_resources_test.go
- e2e-tests/tests/tier1/mta_836_validate_core_group_omitted_offline_test.go
- e2e-tests/tests/tier0/mta_802_ignored_resources_test.go
- e2e-tests/tests/tier0/mta_853_split_apply_test.go
- e2e-tests/tests/tier1/mta_857_service_account_subject_test.go
- e2e-tests/tests/tier0/mta_845_validate_mixed_resources_offline_test.go
- e2e-tests/tests/tier1/mta_858_user_subject_test.go
- e2e-tests/tests/tier1/mta_829_validate_alternative_gv_suggestion_test.go
- e2e-tests/tests/tier1/mta_859_group_subject_test.go
- e2e-tests/tests/tier0/mta_809_initcontainer_test.go
- e2e-tests/tests/tier0/mta_817_stateless_migration_test.go
- e2e-tests/tests/tier0/olm_whiteout_base_test.go
- e2e-tests/tests/tier0/mta_807_data_validation_test.go
- e2e-tests/tests/tier0/mta_804_empty_pvc_migration_test.go
- e2e-tests/tests/tier1/mta_842_secrets_migration_test.go
- e2e-tests/tests/tier0/mta_806_pvc_data_integrity_test.go
- e2e-tests/tests/tier0/mta_840_same_cluster_migration_test.go
- e2e-tests/tests/tier1/mta_832_validate_alternative_gv_suggestion_offline_test.go
- e2e-tests/framework/pipeline.go
- e2e-tests/tests/tier0/mta_808_cronjob_quiesced_test.go
- e2e-tests/tests/tier0/mta_812_role_migration_test.go
- e2e-tests/tests/tier1/olm_whiteout_auditability_test.go
- e2e-tests/tests/tier0/mta_813_cronJob_PVC_test.go
- e2e-tests/tests/tier0/mta_838_multi_container_stateless_migration_test.go
- e2e-tests/tests/tier0/mta_837_hpa_migration_test.go
- e2e-tests/tests/tier0/mta_843_pipeline_determinism_test.go
- e2e-tests/framework/crane.go
- e2e-tests/tests/tier0/mta_811_mongodb_non_admin_test.go
🛑 Comments failed to post (4)
.github/workflows/e2e-pr-tester.yaml (1)
207-207: 🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win
Do not interpolate the PR-controlled regex directly into the shell script.
focus_file_regexis derived from changed filenames, so a malicious PR can inject shell syntax through this GitHub expression. Pass it viaenvand reference the quoted shell variable instead.Proposed fix
+ 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.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 fi🧰 Tools
🪛 zizmor (1.26.1)
[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 at line 207, Update the ginkgo invocation in the E2E workflow to stop interpolating the PR-controlled focus_file_regex GitHub expression directly into the shell command. Pass the value through the step’s env configuration and reference it only via a properly quoted shell variable, preserving the existing --focus-file behavior.Source: Linters/SAST tools
cmd/transfer-pvc/transfer-pvc.go (2)
233-234: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Include the conflicting contexts and cluster in this validation error.
This message is grammatically incorrect and does not identify what must change. Include both context names and
t.sourceContext.Cluster.Proposed fix
if t.sourceContext.Cluster == t.destinationContext.Cluster { - დაბრუნ return fmt.Errorf("both source and destination cluster are the same, this is not support right now, coming soon") + վերադարձ return fmt.Errorf( + "source context %q and destination context %q resolve to the same cluster %q; PVC transfer requires distinct clusters", + t.Flags.SourceContext, + t.Flags.DestinationContext, + t.sourceContext.Cluster, + ) }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/transfer-pvc/transfer-pvc.go` around lines 233 - 234, Update the same-cluster validation error in the transfer flow to use grammatically correct wording and include both context names plus t.sourceContext.Cluster. Preserve the existing validation condition and error behavior while making the message clearly identify the conflicting source and destination configuration.Source: Coding guidelines
373-375: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Preserve retryability when the source certificate secret already exists.
A run failing after this
Createbut beforegarbageCollectleaves the secret behind; the next run reaches this loop and aborts onAlreadyExists. Restore get-and-update/upsert handling and add a fake-client regression test for a pre-existing source secret.🤖 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/transfer-pvc/transfer-pvc.go` around lines 373 - 375, Update the source certificate secret creation flow around srcClient.Create to handle an existing secret by retrieving and updating it, or otherwise using the repository’s established upsert behavior, instead of aborting on AlreadyExists. Preserve creation for missing secrets, and add a fake-client regression test covering a pre-existing source secret and successful retry.Source: Coding guidelines
cmd/transform/transform.go (1)
46-46: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add coverage for
--ignored-patches-dir.No test in this change exercises the new flag or verifies ignored patches are emitted under the configured directory with the
ignored-prefix. Add coverage for both configuration/CLI parsing and conflict-path output behavior. As per coding guidelines, “All new features require tests.”Also applies to: 160-160
🤖 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` at line 46, Add tests for the new IgnoredPatchesDir configuration and --ignored-patches-dir CLI parsing, then cover the conflict path to verify ignored patches are written beneath the configured directory using the ignored- filename prefix. Reuse the existing transform and conflict-output test helpers and preserve current behavior when the option is unset.Source: Coding guidelines
Tamar-Dinavetsky
force-pushed
the
refactor/remove-export-dir-flag-from-apply
branch
from
July 27, 2026 10:45
7720bdd to
735fa41
Compare
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
3 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.
#665
Summary
Removes the
--export-dirflag from theapplycommand - the flag was registered but never used. It was aleftover from when
applywas intended to run the full pipeline automatically, but that was never fullyimplemented.
ExportDirfield and flag registration fromcmd/apply/apply.goExportDirfield and args injection frome2e-tests/framework/crane.goe2e-tests/framework/pipeline.goto remove the now-irrelevanta.ExportDircomparisonExportDirfromApplyOptionsin all test files (~40 files, bulk search & replace)Test plan
go build ./...passesgo test ./internal/... ./cmd/...passesSummary by CodeRabbit
--export-diroption from theapplycommand; apply now uses only the transform and output directories.--ignored-patches-dirto thetransformcommand.crane applycommand docs to remove the--export-dirflag.