Add typed K8s resource helpers and cluster-level export e2e tests (MTA-851–855)#576
Merged
RanWurmbrand merged 3 commits intoJun 29, 2026
Merged
Conversation
- Resource interface with Create/Delete for consistent lifecycle management
- Typed helpers: ClusterRole, ClusterRoleBinding, ServiceAccount, CRD,
CustomResource, Namespace
- ResourceCleanup for multi-cluster test teardown
- E2E tests MTA-851–855 covering RBAC and CRD export scenarios
Signed-off-by: Ran Wurmbrand <rwurmbra@redhat.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
🚧 Files skipped from review as they are similar to previous changes (6)
📝 WalkthroughWalkthroughAdds Kubernetes resource helpers and RBAC validation utilities for e2e tests, plus Widget CRD/CR testdata and five new migration scenarios covering namespace-only, RBAC, split-apply, dual-ClusterRole, and CRD/custom-resource export flows. ChangesCluster-level resource export e2e test suite
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Test Coverage ReportTotal: 46.7% 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: 11
🤖 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 `@e2e-tests/framework/test_helpers.go`:
- Around line 74-80: The subject check in the helper that uses kubectl.Run and
compares against b.SubjectName is too strict because
jsonpath={.subjects[*].name} can return multiple space-separated names. Update
the comparison logic in the cluster role binding verification helper to treat
the output as a list and verify that b.SubjectName is present, rather than
requiring exact string equality. Keep the existing error handling and adjust the
check around the subject comparison so it remains robust if a binding has
multiple subjects.
In `@e2e-tests/tests/tier0/mta_851_no_cluster_resources_test.go`:
- Around line 37-39: The cleanup wrapper currently ignores the error returned by
CleanupScenario, which triggers errcheck in CI. Update the DeferCleanup callback
to capture and handle the CleanupScenario return value in the
mta_851_no_cluster_resources_test.go test, using the existing CleanupScenario
call with paths.TempDir, srcApp, and tgtApp so the error is not silently
dropped.
In `@e2e-tests/tests/tier0/mta_852_minimal_rbac_test.go`:
- Around line 45-48: Handle the ignored error from CleanupScenario in the
DeferCleanup block to satisfy errcheck. Update the cleanup closure in the test
to capture and handle the return value from CleanupScenario alongside the
ResourceCleanup call, using the same cleanup symbols (DeferCleanup,
ResourceCleanup, CleanupScenario) so the failure is not dropped and CI no longer
flags the unchecked error.
In `@e2e-tests/tests/tier0/mta_853_split_apply_test.go`:
- Around line 48-55: Move the `Expect(err).NotTo(HaveOccurred())` check to
immediately follow `NewScenarioPaths("crane-na1-*")` in
`mta_853_split_apply_test.go` before `paths` is used to build `ExportOptions`,
`TransformOptions`, and `ApplyOptions`; this keeps `paths` from being
dereferenced after a failed `NewScenarioPaths` call and aligns the pattern with
the related MTA-851/852 tests.
- Around line 65-69: Handle the ignored error from CleanupScenario inside the
DeferCleanup callback to satisfy errcheck and avoid CI failures. In the
DeferCleanup function in mta_853_split_apply_test.go, capture and handle the
return value from CleanupScenario after ResourceCleanup, using the existing
cleanup variables like paths.TempDir, srcAppNonAdmin, and tgtAppNonAdmin, and
make sure any failure is surfaced or explicitly handled instead of being
dropped.
- Around line 9-11: The test file has a duplicate Gomega import causing
staticcheck ST1019. In the import block for mta_853_split_apply_test.go, remove
the named github.com/onsi/gomega import and keep the dot import already used
alongside ginkgo. Then ensure the assertions in the test body use Succeed()
directly via the existing dot import, including the check near the referenced
expectation in the test.
- Around line 87-88: The export verification is checking the wrong relative path
and the assertion is inverted, so it can pass without confirming the failed
export. Update the `AssertFilesExist` call in the `mta_853_split_apply_test.go`
check to use the namespace-specific `failures/<namespace>/` location for
`deniedResources`, and change the expectation so the helper is asserted to
succeed rather than expecting an error.
In `@e2e-tests/tests/tier1/mta_854_two_clusterroles_test.go`:
- Around line 53-55: The deferred teardown in the test ignores the error
returned by CleanupScenario, which can hide cleanup failures. Update the
DeferCleanup callback in the two-clusterroles test to capture and handle the
CleanupScenario return value from the srcApp/tgtApp teardown path, using the
existing paths.TempDir context and reporting any error so teardown failures are
visible.
In `@e2e-tests/tests/tier1/mta_855_crd_custom_resource_test.go`:
- Around line 44-49: The CustomResource setup in the tier1 MTA CRD test is
missing the Resource field, which causes cleanup to fail when ResourceCleanup
calls CustomResource.Delete. Update the cr initialization so the Widget custom
resource includes the correct API resource name via the Resource field,
alongside Name, Namespace, Kind, and YAML. Use the CustomResource and
ResourceCleanup symbols in this test to locate the setup that must be fixed.
- Around line 55-58: The DeferCleanup block in
mta_855_crd_custom_resource_test.go ignores the return value from
CleanupScenario, triggering errcheck. Update the cleanup closure to capture and
assert the CleanupScenario result with Gomega (similar to how other cleanup
failures are surfaced), while keeping ResourceCleanup and the existing cleanup
flow in place. Use the CleanupScenario call inside DeferCleanup as the target to
fix so the failure is reported instead of dropped.
In `@e2e-tests/utils/utils.go`:
- Around line 958-979: `AssertFilesExist` is comparing the returned recursive
paths from `ListFilesRecursivelyAsList` against bare expected names, so it
misses files written under the namespace-specific failures directory. Update the
matching logic in `AssertFilesExist` to compare against `filepath.Base` (or
otherwise normalize both sides to the actual failure filenames) and ensure the
caller path is the failures dir used by export, so the check targets the real
files produced by the export flow.
🪄 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: c1d49c35-d809-40b8-94dd-e06dfb5e001f
📒 Files selected for processing (10)
e2e-tests/framework/resources.goe2e-tests/framework/test_helpers.goe2e-tests/testdata/widget_cr.yamle2e-tests/testdata/widget_crd.yamle2e-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/tier1/mta_854_two_clusterroles_test.goe2e-tests/tests/tier1/mta_855_crd_custom_resource_test.goe2e-tests/utils/utils.go
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@e2e-tests/framework/resources.go`:
- Around line 14-19: ResourceCleanup is aborting on the first Delete failure,
which leaves later resources and clusters uncleared. Update ResourceCleanup to
keep iterating through all clusters and resources, collect every error instead
of returning immediately, and then return a single aggregated error at the end.
Include cluster and resource context from ResourceCleanup, KubectlRunner, and
Resource.Delete in each failure so callers of DeferCleanup still get useful
cleanup diagnostics.
🪄 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: 548682d1-834d-4494-a333-2015fdaff30a
📒 Files selected for processing (7)
e2e-tests/framework/resources.goe2e-tests/framework/test_helpers.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/tier1/mta_854_two_clusterroles_test.goe2e-tests/tests/tier1/mta_855_crd_custom_resource_test.go
✅ Files skipped from review due to trivial changes (1)
- e2e-tests/tests/tier1/mta_854_two_clusterroles_test.go
🚧 Files skipped from review as they are similar to previous changes (5)
- e2e-tests/tests/tier0/mta_851_no_cluster_resources_test.go
- e2e-tests/tests/tier0/mta_852_minimal_rbac_test.go
- e2e-tests/tests/tier1/mta_855_crd_custom_resource_test.go
- e2e-tests/framework/test_helpers.go
- e2e-tests/tests/tier0/mta_853_split_apply_test.go
Signed-off-by: Ran Wurmbrand <rwurmbra@redhat.com>
RanWurmbrand
force-pushed
the
infra/resources-and-tests
branch
from
2eaebe7 to
0335919
Compare
Signed-off-by: Ran Wurmbrand <rwurmbra@redhat.com>
2 tasks
msajidmansoori12
pushed a commit
to msajidmansoori12/crane
that referenced
this pull request
Jul 1, 2026
…A-851–855) (migtools#576) * Add typed K8s resource helpers and cluster-level export tests - Resource interface with Create/Delete for consistent lifecycle management - Typed helpers: ClusterRole, ClusterRoleBinding, ServiceAccount, CRD, CustomResource, Namespace - ResourceCleanup for multi-cluster test teardown - E2E tests MTA-851–855 covering RBAC and CRD export scenarios Signed-off-by: Ran Wurmbrand <rwurmbra@redhat.com> * addresed code rabbit Signed-off-by: Ran Wurmbrand <rwurmbra@redhat.com> * addressed comments Signed-off-by: Ran Wurmbrand <rwurmbra@redhat.com> --------- Signed-off-by: Ran Wurmbrand <rwurmbra@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
2 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.
closes: #579, closes: #578, closes:#334, closes:#333, closes:#332
Summary
Resourceinterface withCreate/Deletemethods for consistent K8s resource lifecycle management in testsClusterRole,ClusterRoleBinding,ServiceAccount,CustomResourceDefinition,CustomResource,NamespaceResourceCleanupfor multi-cluster test teardownValidateClusterRBAChelper for verifying cluster role bindings on targetReadTestdataFileutility for loading test fixturesNew Tests
_clusterdirectory for namespace-only workloads_cluster, custom resource to namespace directoryTest plan
ginkgo -v --label-filter="tier0" ./e2e-tests/...ginkgo -v --label-filter="tier1" ./e2e-tests/...Summary by CodeRabbit
Summary