Fix: namespace restricted mode issues - #7281
Conversation
59606ae to
aae691a
Compare
bf4547a to
99a73f5
Compare
…espaced is activated Signed-off-by: François Rigaut <francois.rigaut@orange.com>
Signed-off-by: RIGAUT Francois DTOF/PFC <francois.rigaut@orange.com>
99a73f5 to
071c621
Compare
📝 WalkthroughWalkthroughChangesThe change corrects restricted event filtering and adds namespace-restricted E2E coverage. The E2E matrix now runs the new area, with manifests for resources in separate namespaces. Namespace restriction
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant E2ETest
participant Helm
participant Crossplane
participant KubernetesAPI
E2ETest->>Helm: install with --watch-cache-namespaced
Helm->>Crossplane: configure namespace-restricted mode
E2ETest->>KubernetesAPI: apply namespace and composition resources
KubernetesAPI->>Crossplane: reconcile namespaced XR
Crossplane-->>KubernetesAPI: update readiness and status.coolerField
E2ETest->>KubernetesAPI: delete XR and prerequisites
Suggested reviewers: 🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 passed)
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 |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
test/e2e/namespace_restricted_test.go (1)
58-92: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the required table-driven test form.
Please represent the scenario as a named test case with
args,want, andreasonfields. Keep the feature construction inside the case loop. This structure will support additional namespace-restricted scenarios without duplicating the lifecycle flow.As per path instructions,
**/*_test.go: “Enforce table-driven test structure: PascalCase test names (no underscores), args/want pattern, use cmp.Diff with cmpopts.EquateErrors() for error testing.”🤖 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 `@test/e2e/namespace_restricted_test.go` around lines 58 - 92, Refactor the TestNamespaceRestrictedBasicCompositionNamespaced function into a table-driven test structure with a slice of test cases, where each case contains args, want, and reason fields representing different namespace-restricted scenarios. Move the feature construction and lifecycle flow (WithSetup, Assess, WithTeardown) inside a loop that iterates over the test cases, parameterizing the manifests and resource paths to support multiple scenarios without duplicating the shared lifecycle logic. Use PascalCase naming for individual test case names without underscores.Source: Path instructions
cmd/crossplane/core/core.go (1)
265-277: 🎯 Functional Correctness | 🔵 Trivial | 🏗️ Heavy liftAdd a regression test for all filter outcomes.
Thank you for correcting the predicate. The new E2E suite enables only
--watch-cache-namespacedattest/e2e/namespace_restricted_test.go, Line 50. It does not execute thisRestrictNamespacedEventsfilter.Add a table-driven test for metadata-access failure, cluster-scoped objects,
default-namespace objects, and other namespaced objects. Assert the expectedevent.FilterFnresult for each case.As per path instructions,
**/*_test.go: “Enforce table-driven test structure: PascalCase test names (no underscores), args/want pattern, use cmp.Diff with cmpopts.EquateErrors() for error testing.”🤖 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/crossplane/core/core.go` around lines 265 - 277, Create a table-driven unit test for the RestrictNamespacedEvents filter predicate that covers all filter outcomes: metadata-access failures, cluster-scoped objects (empty namespace), default-namespace objects, and other namespaced objects. Use PascalCase test names without underscores, an args/want pattern for test cases, and assert the expected boolean result returned by the event.FilterFn for each scenario. For metadata-access failure cases, use cmp.Diff with cmpopts.EquateErrors() to validate error behavior.Source: Path instructions
🤖 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 `@cmd/crossplane/core/core.go`:
- Around line 265-277: Create a table-driven unit test for the
RestrictNamespacedEvents filter predicate that covers all filter outcomes:
metadata-access failures, cluster-scoped objects (empty namespace),
default-namespace objects, and other namespaced objects. Use PascalCase test
names without underscores, an args/want pattern for test cases, and assert the
expected boolean result returned by the event.FilterFn for each scenario. For
metadata-access failure cases, use cmp.Diff with cmpopts.EquateErrors() to
validate error behavior.
In `@test/e2e/namespace_restricted_test.go`:
- Around line 58-92: Refactor the
TestNamespaceRestrictedBasicCompositionNamespaced function into a table-driven
test structure with a slice of test cases, where each case contains args, want,
and reason fields representing different namespace-restricted scenarios. Move
the feature construction and lifecycle flow (WithSetup, Assess, WithTeardown)
inside a loop that iterates over the test cases, parameterizing the manifests
and resource paths to support multiple scenarios without duplicating the shared
lifecycle logic. Use PascalCase naming for individual test case names without
underscores.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 93ea1be9-500b-4e9e-89bf-876d3e777427
📒 Files selected for processing (6)
.github/workflows/ci.ymlcmd/crossplane/core/core.gotest/e2e/manifests/namespace-restricted/other-namespace.yamltest/e2e/manifests/namespace-restricted/xr.yamltest/e2e/namespace_restricted_test.gotest/e2e/ops_operations_test.go
Description of your changes
Fixes #7354 and #7353
Depends on crossplane/crossplane-runtime#946
I have:
./nix.sh flake checkto ensure this PR is ready for review.backport release-x.ylabels to auto-backport this PR.Need help with this checklist? See the cheat sheet.