🧪 [fix] mta_853 test logic so it will fit the test plan more accuratly#636
Open
RanWurmbrand wants to merge 2 commits into
Open
🧪 [fix] mta_853 test logic so it will fit the test plan more accuratly#636RanWurmbrand wants to merge 2 commits into
RanWurmbrand wants to merge 2 commits into
Conversation
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe Crane test wrappers add overwrite support to export and apply commands. The MTA-853 split-apply test adds RBAC setup, executes namespace-admin and cluster-admin pipeline phases, validates outputs, and applies namespace and cluster resources separately. ChangesSplit apply overwrite flow
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant NamespaceAdmin as namespace-admin
participant CraneRunner
participant ClusterAdmin as cluster-admin
participant Target as target
NamespaceAdmin->>CraneRunner: export, transform, and apply without overwrite
CraneRunner-->>NamespaceAdmin: namespace output and denied cluster-resource failures
NamespaceAdmin->>Target: apply generated namespace resources
ClusterAdmin->>CraneRunner: export, transform, and apply with overwrite
CraneRunner-->>ClusterAdmin: cluster resources in _cluster
ClusterAdmin->>Target: apply generated cluster resources
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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.2% 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: 2
🤖 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/tests/tier0/mta_853_split_apply_test.go`:
- Line 113: Correct the misleading By messages in the cluster-admin phase:
update the message near the cluster-admin export/transform/apply steps to say
“as cluster-admin,” and update the application message near the cluster-resource
operation to describe applying cluster resources from _cluster with the admin
runner. Leave the execution logic unchanged.
- Line 95: Correct the typo in the Ginkgo By message by changing “Relevent” to
“Relevant” while preserving the rest of the message unchanged.
🪄 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: 71bc5d07-2bc7-4032-8362-7a4616280f4c
📒 Files selected for processing (2)
e2e-tests/framework/crane.goe2e-tests/tests/tier0/mta_853_split_apply_test.go
| By("Namespace admin phase: Applying namespace resources to target as namespace-admin") | ||
| Expect(kubectlTgtNonAdmin.ApplyDir(filepath.Join(paths.OutputDir, "resources", namespace))).NotTo(HaveOccurred()) | ||
|
|
||
| By("Cluster admin phase: Running crane export, transform, apply as namespace-admin") |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct misleading By messages in the cluster-admin phase.
Both messages are copy-pasted from the namespace-admin phase and incorrectly describe the cluster-admin phase:
- Line 113 says "as namespace-admin" but uses
adminRunner(cluster-admin). - Line 123 says "Applying namespace resources to target as namespace-admin" but applies cluster resources from
_clusterusingkubectlTgt(admin).
✏️ Proposed fixes
- By("Cluster admin phase: Running crane export, transform, apply as namespace-admin")
+ By("Cluster admin phase: Running crane export, transform, apply as cluster-admin")- By("Cluster admin phase: Applying namespace resources to target as namespace-admin")
+ By("Cluster admin phase: Applying cluster resources to target as cluster-admin")Also applies to: 123-124
🤖 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_853_split_apply_test.go` at line 113, Correct the
misleading By messages in the cluster-admin phase: update the message near the
cluster-admin export/transform/apply steps to say “as cluster-admin,” and update
the application message near the cluster-resource operation to describe applying
cluster resources from _cluster with the admin runner. Leave the execution logic
unchanged.
Signed-off-by: Ran Wurmbrand <rwurmbra@redhat.com>
Member
|
Two minor findings:
|
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.
Summary
Overwritefield toExportOptionsandApplyOptionsso the pipeline can re-run with--overwriteWhy
The previous MTA-853 test only ran the namespace-admin phase — it verified that cluster resources failed for non-admin and applied namespace resources, but never completed the workflow by having a cluster-admin apply the
_cluster/resources. This didn't match test plan NA-1 which requires both phases with Pod Ready at the end.Test plan
_cluster/, apply themSummary by CodeRabbit