🧪 Add e2e tests for cluster-level export filtering: mta868,869,870#646
Open
RanWurmbrand wants to merge 5 commits into
Open
🧪 Add e2e tests for cluster-level export filtering: mta868,869,870#646RanWurmbrand wants to merge 5 commits into
RanWurmbrand wants to merge 5 commits into
Conversation
…s on ressources.go Signed-off-by: Ran Wurmbrand <rwurmbra@redhat.com>
Signed-off-by: Ran Wurmbrand <rwurmbra@redhat.com>
Signed-off-by: Ran Wurmbrand <rwurmbra@redhat.com>
📝 WalkthroughWalkthroughAdds resource filename assertion utilities, optional ServiceAccount labeling, and end-to-end coverage for label-scoped cluster exports, unrelated ClusterRoleBindings, and CRD include/skip group behavior. ChangesExport filtering validation
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant SourceCluster
participant CranePipeline
participant ExportDirectory
participant TargetCluster
SourceCluster->>CranePipeline: Create Gadget CRD and custom resource
CranePipeline->>ExportDirectory: Export CRD and custom resource
CranePipeline->>TargetCluster: Apply exported resources
TargetCluster-->>CranePipeline: Confirm CRD establishment and Gadget validation
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 |
Test Coverage ReportTotal: 49.6% 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: 6
🤖 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 136-141: Add tests for the ServiceAccount labeling logic in the
resource creation flow around the sa.Label branch: verify empty labels skip
k.Run, non-empty labels invoke the expected label command successfully, and
label command errors are propagated with the ServiceAccount context. Use the
existing test patterns and helpers in the resources tests.
- Line 139: Update the error returned by the ServiceAccount labeling operation
to include the ServiceAccount namespace and the label being applied, while
preserving the existing resource name and wrapped error. Ensure the message
identifies the full attempted mutation for parallel E2E debugging.
In `@e2e-tests/testdata/gadget_cr.yaml`:
- Around line 1-7: Rename e2e-tests/testdata/gadget_cr.yaml to
Gadget_crane-e2e.openshift.io_v1_simple-nginx-nopv_test-gadget.yaml and
e2e-tests/testdata/gadget_crd.yaml to
CustomResourceDefinition_apiextensions.k8s.io_v1_clusterscoped_gadgets.crane-e2e.openshift.io.yaml;
update both ReadTestdataFile arguments in ca10_crd_flags_test.go to reference
the new filenames.
In `@e2e-tests/tests/tier0/ca8_label_scoped_export_test.go`:
- Around line 99-108: Update the CA-8 test assertions around
AssertResourcesExist so every outOfScopeResources item is checked individually
and each must be absent, rather than relying on the aggregate result. Add
equivalent per-resource assertions for in-scope and out-of-scope ServiceAccounts
in the namespace export directory, while preserving the existing ClusterRole and
ClusterRoleBinding checks.
In `@e2e-tests/utils/utils_test.go`:
- Around line 1484-1513: Extend the resource-matching table around the existing
ClusterRole and ClusterRoleBinding cases with regression tests for filename
component collisions: distinguish ClusterRole from ClusterRoleBinding, reject
partial Group and Version values, and verify Version matching without Group. Use
representative fixture filenames and set wantFound to reflect exact component
matching.
In `@e2e-tests/utils/utils.go`:
- Around line 1250-1268: Update the resource filename matching logic in the
relevant utility function so every specified prefix component, including Kind,
Group, and Version, is terminated with an underscore before matching, and reject
resources that specify Version without Group. In e2e-tests/utils/utils_test.go
lines 1484-1513, add regression cases covering kind, group, and version prefix
collisions plus the invalid version-only resource case.
🪄 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: aeb8335e-df0f-408d-a1ec-fe6eca8958e1
📒 Files selected for processing (8)
e2e-tests/framework/resources.goe2e-tests/testdata/gadget_cr.yamle2e-tests/testdata/gadget_crd.yamle2e-tests/tests/tier0/ca10_crd_flags_test.goe2e-tests/tests/tier0/ca8_label_scoped_export_test.goe2e-tests/tests/tier1/ca7_unrelated_crb_test.goe2e-tests/utils/utils.goe2e-tests/utils/utils_test.go
Comment on lines
+136
to
+141
| if sa.Label != "" { | ||
| _, err = k.Run("label", "serviceaccount", sa.Name, "-n", sa.Namespace, sa.Label) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to label ServiceAccount %s: %w", sa.Name, err) | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Add tests for optional ServiceAccount labeling.
Cover the empty-label path, successful label invocation, and propagated label failure. 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 `@e2e-tests/framework/resources.go` around lines 136 - 141, Add tests for the
ServiceAccount labeling logic in the resource creation flow around the sa.Label
branch: verify empty labels skip k.Run, non-empty labels invoke the expected
label command successfully, and label command errors are propagated with the
ServiceAccount context. Use the existing test patterns and helpers in the
resources tests.
Source: Coding guidelines
Comment on lines
+1
to
+7
| apiVersion: crane-e2e.openshift.io/v1 | ||
| kind: Gadget | ||
| metadata: | ||
| name: test-gadget | ||
| spec: | ||
| color: red | ||
| size: 3 |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Rename the new YAML fixtures consistently with the repository convention.
e2e-tests/testdata/gadget_cr.yaml#L1-L7: rename toGadget_crane-e2e.openshift.io_v1_simple-nginx-nopv_test-gadget.yaml.e2e-tests/testdata/gadget_crd.yaml#L1-L27: rename toCustomResourceDefinition_apiextensions.k8s.io_v1_clusterscoped_gadgets.crane-e2e.openshift.io.yaml.e2e-tests/tests/tier0/ca10_crd_flags_test.go#L120-L123: update bothReadTestdataFilearguments.
As per coding guidelines, YAML resource names should follow Kind_group_version_namespace_name.yaml.
📍 Affects 3 files
e2e-tests/testdata/gadget_cr.yaml#L1-L7(this comment)e2e-tests/testdata/gadget_crd.yaml#L1-L27e2e-tests/tests/tier0/ca10_crd_flags_test.go#L120-L123
🤖 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/gadget_cr.yaml` around lines 1 - 7, Rename
e2e-tests/testdata/gadget_cr.yaml to
Gadget_crane-e2e.openshift.io_v1_simple-nginx-nopv_test-gadget.yaml and
e2e-tests/testdata/gadget_crd.yaml to
CustomResourceDefinition_apiextensions.k8s.io_v1_clusterscoped_gadgets.crane-e2e.openshift.io.yaml;
update both ReadTestdataFile arguments in ca10_crd_flags_test.go to reference
the new filenames.
Source: Coding guidelines
…no file exist Signed-off-by: Ran Wurmbrand <rwurmbra@redhat.com>
Signed-off-by: Ran Wurmbrand <rwurmbra@redhat.com>
There was a problem hiding this comment.
🧹 Nitpick comments (6)
e2e-tests/tests/tier0/mta_868_label_scoped_export_test.go (4)
42-61: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winIn-scope/out-of-scope ServiceAccounts are created but never asserted on.
inScopeSAandoutOfScopeSAare set up specifically to exercise--label-selectoron a namespaced workload, but only ClusterRole/ClusterRoleBinding are checked afterward (Lines 99-108). Since the PR objective explicitly calls out validating "labeled workloads" (not just RBAC), addResourceMatchentries for the SAs and assert their presence/absence in the namespace export directory to actually cover this.♻️ Suggested addition
outOfScopeResources := []utils.ResourceMatch{ {Kind: "ClusterRoleBinding", Name: outOfScopeBinding.Name}, {Kind: "ClusterRole", Name: outOfScopeCR.Name}, + {Kind: "ServiceAccount", Name: outOfScopeSA.Name, Scope: namespace}, } inScopeResources := []utils.ResourceMatch{ {Kind: "ClusterRoleBinding", Name: inScopeBinding.Name}, {Kind: "ClusterRole", Name: inScopeCR.Name}, + {Kind: "ServiceAccount", Name: inScopeSA.Name, Scope: namespace}, }🤖 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_868_label_scoped_export_test.go` around lines 42 - 61, Add ServiceAccount ResourceMatch entries using inScopeSA and outOfScopeSA to the corresponding inScopeResources and outOfScopeResources collections, then ensure the export assertions verify the in-scope account is present and the out-of-scope account is absent in the namespace export directory.
1-1: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winBoth tests claim to verify export/transform/output stages but only assert against the export directory. The shared root cause:
By()messages describe checking all three pipeline stages, while the actualAssertResourcesExistcalls only targetpaths.ExportDir-derived paths, leaving transform/apply-stage regressions uncaught.
e2e-tests/tests/tier0/mta_868_label_scoped_export_test.go#L105-108: either narrow the By() text to "export directory" or extend the assertions to also checkpaths.TransformDirandpaths.OutputDirequivalents for the in-scope ClusterRole/ClusterRoleBinding.e2e-tests/tests/tier1/mta_870_unrelated_crb_test.go#L96-96: same fix — narrow the By() text or add corresponding transform/output directory assertions fortestCRB/cr.🤖 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_868_label_scoped_export_test.go` at line 1, The test step descriptions overstate coverage because both tests only assert resources under paths.ExportDir. Update the By() messages in the tier0 and tier1 test cases to say “export directory,” or extend their AssertResourcesExist calls to validate the corresponding paths.TransformDir and paths.OutputDir resources for the in-scope ClusterRole and ClusterRoleBinding objects.
105-108: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winVerification scope narrower than the
By()description claims.The message says resources are checked "in export, transform, and output," but
AssertResourcesExistis only called againstexportClusterPath(the export stage). Transform/output stage regressions (e.g., a plugin accidentally dropping the CRB during transform) wouldn't be caught here. This same pattern repeats ine2e-tests/tests/tier1/mta_870_unrelated_crb_test.go— see the consolidated comment.🤖 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_868_label_scoped_export_test.go` around lines 105 - 108, The verification around AssertResourcesExist must cover export, transform, and output stages as stated by the By() message. Invoke the appropriate existing stage paths for inScopeResources and assert each result succeeds, applying the same update to the corresponding verification in mta_870_unrelated_crb_test.go.
1-1: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTest titles and temp-dir prefixes still reference the old CA-7/8/10 tags after the MTA rename. The commit renamed the files to
mta_868/mta_869/mta_870, but theIt()description strings andNewScenarioPathsprefixes were never updated, so Ginkgo reports/CI logs will still show[CA-8],[CA-7],[CA-10a]/[CA-10b]instead of the new MTA identifiers, making it harder to trace failures to the renamed Jira/MTA tickets.
e2e-tests/tests/tier0/mta_868_label_scoped_export_test.go#L15-15: update the title to referenceMTA-868(and the"crane-ca8-*"prefix on Line 33 to a matchingmta-868prefix).e2e-tests/tests/tier1/mta_870_unrelated_crb_test.go#L15-15: update the title to referenceMTA-870(and the"crane-ca7-*"prefix on Line 32 to a matchingmta-870prefix).e2e-tests/tests/tier0/mta_869_crd_flags_test.go#L18-18,103-103: update both titles to referenceMTA-869(and the"crane-ca10a-*"/"crane-ca10b-*"prefixes on Lines 33/118 to matchingmta-869prefixes).🤖 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_868_label_scoped_export_test.go` at line 1, Update the It() descriptions and NewScenarioPaths prefixes in the MTA-868, MTA-869, and MTA-870 tests to use the renamed MTA identifiers: MTA-868 with mta-868, MTA-869 for both cases with mta-869 prefixes, and MTA-870 with mta-870. Replace the old CA references and crane-ca prefixes while preserving the existing test behavior.e2e-tests/tests/tier0/mta_869_crd_flags_test.go (1)
185-187: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove leftover commented-out verification code.
This dead block (By/path/Expect) should be deleted rather than left commented out.
🧹 Suggested cleanup
- // By("Verifying CRD exists in output _cluster directory") - // outputClusterPath := filepath.Join(paths.OutputDir, "resources", "_cluster") - // Expect(ValidateDirResources(outputClusterPath, crdPatterns)).NotTo(HaveOccurred()) -🤖 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_869_crd_flags_test.go` around lines 185 - 187, Remove the leftover commented-out By, outputClusterPath, and Expect verification block from the test, leaving the surrounding test logic unchanged.e2e-tests/tests/tier1/mta_870_unrelated_crb_test.go (1)
90-94: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPrefer
AssertResourcesDontExistfor exclusion checks, matching the sibling test's idiom.
mta_868_label_scoped_export_test.go(Line 101) usesutils.AssertResourcesDontExistfor the same "resource must be absent" assertion. UsingAssertResourcesExist(...)+BeFalse()here works but is a less direct, inconsistent idiom for the same helper API.♻️ Suggested fix
- found, err := utils.AssertResourcesExist(exportClusterPath, []utils.ResourceMatch{ - {Kind: "ClusterRoleBinding", Name: unrelatedCRB.Name}, - }) - Expect(err).NotTo(HaveOccurred()) - Expect(found).To(BeFalse()) + excluded, err := utils.AssertResourcesDontExist(exportClusterPath, []utils.ResourceMatch{ + {Kind: "ClusterRoleBinding", Name: unrelatedCRB.Name}, + }) + Expect(err).NotTo(HaveOccurred()) + Expect(excluded).To(BeTrue())🤖 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_870_unrelated_crb_test.go` around lines 90 - 94, Replace the exclusion assertion in the test with utils.AssertResourcesDontExist, passing exportClusterPath and the ClusterRoleBinding match for unrelatedCRB.Name; retain the existing error assertion and remove the AssertResourcesExist/BeFalse pattern.
🤖 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/tests/tier0/mta_868_label_scoped_export_test.go`:
- Around line 42-61: Add ServiceAccount ResourceMatch entries using inScopeSA
and outOfScopeSA to the corresponding inScopeResources and outOfScopeResources
collections, then ensure the export assertions verify the in-scope account is
present and the out-of-scope account is absent in the namespace export
directory.
- Line 1: The test step descriptions overstate coverage because both tests only
assert resources under paths.ExportDir. Update the By() messages in the tier0
and tier1 test cases to say “export directory,” or extend their
AssertResourcesExist calls to validate the corresponding paths.TransformDir and
paths.OutputDir resources for the in-scope ClusterRole and ClusterRoleBinding
objects.
- Around line 105-108: The verification around AssertResourcesExist must cover
export, transform, and output stages as stated by the By() message. Invoke the
appropriate existing stage paths for inScopeResources and assert each result
succeeds, applying the same update to the corresponding verification in
mta_870_unrelated_crb_test.go.
- Line 1: Update the It() descriptions and NewScenarioPaths prefixes in the
MTA-868, MTA-869, and MTA-870 tests to use the renamed MTA identifiers: MTA-868
with mta-868, MTA-869 for both cases with mta-869 prefixes, and MTA-870 with
mta-870. Replace the old CA references and crane-ca prefixes while preserving
the existing test behavior.
In `@e2e-tests/tests/tier0/mta_869_crd_flags_test.go`:
- Around line 185-187: Remove the leftover commented-out By, outputClusterPath,
and Expect verification block from the test, leaving the surrounding test logic
unchanged.
In `@e2e-tests/tests/tier1/mta_870_unrelated_crb_test.go`:
- Around line 90-94: Replace the exclusion assertion in the test with
utils.AssertResourcesDontExist, passing exportClusterPath and the
ClusterRoleBinding match for unrelatedCRB.Name; retain the existing error
assertion and remove the AssertResourcesExist/BeFalse pattern.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 73c0f852-e8b1-4425-a5f0-d7d125d17831
📒 Files selected for processing (3)
e2e-tests/tests/tier0/mta_868_label_scoped_export_test.goe2e-tests/tests/tier0/mta_869_crd_flags_test.goe2e-tests/tests/tier1/mta_870_unrelated_crb_test.go
Contributor
Author
|
these tests passed on ocp |
Contributor
Author
|
/rfr |
RanWurmbrand
changed the title
Open
4 tasks
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
1 participant
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: #645
Summary
Add e2e tests for cluster-level export filtering (CA-7, CA-8, CA-10) and a reusable resource assertion utility.
Changes
--label-selectorscopes export to labeled workloads and their associated RBAC--crd-skip-groupand--crd-include-groupflags control CRD export behaviorResourceMatchstruct withAssertResourcesExist()andAssertResourcesDontExist()for verifying exported resource files by kind, name, and scopeLabelfield toServiceAccountstructgadget_crd.yamlandgadget_cr.yamlfor CA-10b testTest plan
ginkgo run -v -r --focus="CA-7" e2e-tests/testsginkgo run -v -r --focus="CA-8" e2e-tests/testsginkgo run -v -r --focus="CA-10" e2e-tests/testsgo test ./e2e-tests/utils -vSummary by CodeRabbit
New Features
Bug Fixes
Tests