SANDBOX1318: update filter function to accept runtimeclient object/runtimeobject#489
SANDBOX1318: update filter function to accept runtimeclient object/runtimeobject#489fbm3307 wants to merge 3 commits intocodeready-toolchain:masterfrom
Conversation
…ntimeobject Signed-off-by: Feny Mehta <fbm3307@gmail.com>
WalkthroughFilter now accepts interface{} and normalizes supported slice types ([]runtime.RawExtension, []runtime.Object, []runtimeclient.Object) into []runtime.RawExtension, applies FilterFunc(s) uniformly, and panics for unsupported input types. Tests expanded to cover all three input shapes and panic behavior. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller
participant Filter as template.Filter
participant TS as TypeSwitch/Normalizer
participant FL as FilterFuncs
participant Result as ResultSlice
Caller->>Filter: Filter(objs:any, filters...)
Filter->>TS: Inspect and normalize objs
alt Supported type -> normalized []RawExtension
TS-->>Filter: normalized slice
loop per object
Filter->>FL: Apply FilterFunc(s)
alt any returns false
Note right of Filter: continue to next object
else all true
Filter->>Result: append object
end
end
Filter-->>Caller: return []runtime.RawExtension
else Unsupported type
TS-->>Filter: unsupported
Filter-->>Caller: panic(fmt.Sprintf(...))
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #489 +/- ##
==========================================
+ Coverage 78.62% 78.82% +0.20%
==========================================
Files 52 52
Lines 2638 2663 +25
==========================================
+ Hits 2074 2099 +25
Misses 502 502
Partials 62 62
🚀 New features to boost your workflow:
|
Signed-off-by: Feny Mehta <fbm3307@gmail.com>
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
pkg/template/filter.go (3)
27-29: Document the panic behavior in the exported function commentSince the function now panics on unsupported input types, the doc should state this explicitly to set caller expectations and improve godoc.
-// Filter filters the given objs to return only those matching the given filters (if any) -// Accepts []runtime.RawExtension, []runtime.Object, or []runtimeclient.Object +// Filter filters the given objs to return only those matching the given filters (if any). +// Accepts []runtime.RawExtension, []runtime.Object, or []runtimeclient.Object. +// Panics on unsupported input types. func Filter(objs interface{}, filters ...FilterFunc) []runtime.RawExtension {
30-32: Handle nil input gracefully to avoid surprising panic on Filter(nil)With the new interface{} signature, it’s easy to accidentally pass a nil interface (e.g., Filter(nil)). Today that will fall through to default and panic. Returning an empty slice is safer and aligns with common Go conventions.
// Convert input to []runtime.RawExtension var rawExtensions []runtime.RawExtension + // Gracefully handle a nil interface input + if objs == nil { + return nil + }
33-57: Remove redundant type assertions and an extra allocationYou can split the combined case into two distinct cases. This removes the second type assertion and the temporary []runtime.Object allocation for the []runtimeclient.Object path, improving clarity and shaving an allocation.
- switch v := objs.(type) { - case []runtime.RawExtension: - rawExtensions = v - case []runtime.Object, []runtimeclient.Object: - // Handle both runtime.Object and runtimeclient.Object the same way - // since runtimeclient.Object embeds runtime.Object - var objects []runtime.Object - if runtimeObjs, ok := v.([]runtime.Object); ok { - objects = runtimeObjs - } else { - // Convert []runtimeclient.Object to []runtime.Object - clientObjs := v.([]runtimeclient.Object) - objects = make([]runtime.Object, len(clientObjs)) - for i, obj := range clientObjs { - objects[i] = obj - } - } - - rawExtensions = make([]runtime.RawExtension, len(objects)) - for i, obj := range objects { - rawExtensions[i] = runtime.RawExtension{Object: obj} - } - default: - panic(fmt.Sprintf("unsupported type %T for Filter function. Supported types: []runtime.RawExtension, []runtime.Object, []runtimeclient.Object", objs)) - } + switch v := objs.(type) { + case []runtime.RawExtension: + rawExtensions = v + case []runtime.Object: + rawExtensions = make([]runtime.RawExtension, len(v)) + for i, obj := range v { + rawExtensions[i] = runtime.RawExtension{Object: obj} + } + case []runtimeclient.Object: + rawExtensions = make([]runtime.RawExtension, len(v)) + for i, obj := range v { + rawExtensions[i] = runtime.RawExtension{Object: obj} + } + default: + panic(fmt.Sprintf("unsupported type %T for Filter function. Supported types: []runtime.RawExtension, []runtime.Object, []runtimeclient.Object", objs)) + }pkg/template/filter_test.go (1)
373-383: Optionally assert the panic message to harden the contractCurrent test asserts a panic occurs, which is fine. If you want to pin the contract further, assert the message (or at least that it contains "unsupported type"). This is optional but can prevent accidental behavior changes.
- assert.Panics(t, func() { - template.Filter(unsupportedSlice) - }) + assert.PanicsWithValue(t, + "unsupported type []string for Filter function. Supported types: []runtime.RawExtension, []runtime.Object, []runtimeclient.Object", + func() { + template.Filter(unsupportedSlice) + }, + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
pkg/template/filter.go(2 hunks)pkg/template/filter_test.go(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
pkg/template/filter_test.go (2)
pkg/test/testing_t.go (1)
T(4-11)pkg/template/filter.go (3)
Filter(29-72)RetainNamespaces(12-15)RetainAllButNamespaces(18-21)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Verify Dependencies
🔇 Additional comments (6)
pkg/template/filter.go (3)
3-8: Imports look goodNew imports are minimal and justified by the added functionality.
59-70: Filtering loop is clear and preserves orderLabel-based early-continue is succinct, avoids extra flags, and keeps the original order intact. Capacity preallocation is a nice touch.
28-29: Alltemplate.Filtercall sites use only supported slice types
I’ve reviewed everytemplate.Filterinvocation across the codebase—all are passed one of the three allowed slice types ([]runtime.RawExtension,[]runtime.Object, or[]runtimeclient.Object), and there’s even a dedicated test that panics on an unsupported slice. No unexpected argument types found; this change is safe as-is.pkg/template/filter_test.go (3)
12-12: Import for runtimeclient is appropriateNeeded for exercising the []runtimeclient.Object path.
221-295: Good coverage for []runtime.Object inputValidates no-filter behavior and both filter predicates with order preservation. Solid.
297-371: Good coverage for []runtimeclient.Object inputMirrors runtime.Object tests and confirms the normalized path behaves identically.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
pkg/template/filter_test.go (5)
17-49: Set apiVersion in unstructured fixtures to ensure robust kind/GVK detectionRight now the fixtures only set "kind". If any filtering logic consults GroupVersionKind (common via GetObjectKind().GroupVersionKind()), Namespaces and RoleBindings may resolve to zero values and cause false negatives. Add apiVersion to each object to avoid brittle behavior.
Apply this diff to include apiVersion in the fixtures:
ns1 = &unstructured.Unstructured{ Object: map[string]interface{}{ - "kind": "Namespace", + "apiVersion": "v1", + "kind": "Namespace", "metadata": map[string]interface{}{ "name": "ns1", }, }, } ns2 = &unstructured.Unstructured{ Object: map[string]interface{}{ - "kind": "Namespace", + "apiVersion": "v1", + "kind": "Namespace", "metadata": map[string]interface{}{ "name": "ns2", }, }, } rb1 = &unstructured.Unstructured{ Object: map[string]interface{}{ - "kind": "RoleBinding", + "apiVersion": "rbac.authorization.k8s.io/v1", + "kind": "RoleBinding", "metadata": map[string]interface{}{ "name": "rb1", }, }, } rb2 = &unstructured.Unstructured{ Object: map[string]interface{}{ - "kind": "RoleBinding", + "apiVersion": "rbac.authorization.k8s.io/v1", + "kind": "RoleBinding", "metadata": map[string]interface{}{ "name": "rb2", }, }, }
51-58: Avoid duplicating expectedCount; derive it from expectedObjectsexpectedCount duplicates len(expectedObjects) and risks drift. Consider removing expectedCount and using len(expectedObjects) in assertions to keep tests DRY.
I can provide a follow-up diff to remove the field and update assertions if you want to simplify in this PR.
91-94: Guard the type assertion to runtimeclient.Object to avoid incidental panicsIf a future test case includes an object that doesn’t implement runtimeclient.Object, this will panic before the filter is even exercised. Use a checked assertion and fail fast with a clear message.
Apply this diff:
- clientObjects := make([]runtimeclient.Object, len(tc.objects)) - for i, obj := range tc.objects { - clientObjects[i] = obj.(runtimeclient.Object) - } + clientObjects := make([]runtimeclient.Object, len(tc.objects)) + for i, obj := range tc.objects { + co, ok := obj.(runtimeclient.Object) + require.True(t, ok, "object at index %d does not implement runtimeclient.Object", i) + clientObjects[i] = co + }
73-76: Consider asserting on stable identifiers (kind/name) versus whole-object equalityAsserting deep equality of entire objects tightly couples tests to representation details. Comparing Kind and metadata.name reduces brittleness while still validating correctness.
Example within each loop:
- Cast both expected and actual to *unstructured.Unstructured and compare GetKind()/GetName().
If you want, I can push a concrete diff that switches these three loops to compare Kind/Name and adds a small helper.
Also applies to: 83-86, 98-101
170-180: Unsupported type panic test is spot onThis guards the new interface{} entry-point well. Optional: assert the panic message to lock down error clarity if you standardize it in implementation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
pkg/template/filter_test.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Verify Dependencies
🔇 Additional comments (3)
pkg/template/filter_test.go (3)
12-13: Importing controller-runtime client is appropriateGood call adding the controller-runtime client import to exercise the new []runtimeclient.Object input shape.
60-106: Table-driven harness exercising all supported input shapes — niceThe helper cleanly validates behavior across []runtime.RawExtension, []runtime.Object, and []runtimeclient.Object while preserving order. Good structure and reuse.
107-168: Good coverage of filter semantics and ordering
- Verifies no-op with no filters.
- Conflicting filters produce empty set (AND semantics).
- Covers both RetainNamespaces and RetainAllButNamespaces with single/multiple/empty results.
fbm3307
changed the title
| // Accepts []runtime.RawExtension, []runtime.Object, or []runtimeclient.Object | ||
| func Filter(objs interface{}, filters ...FilterFunc) []runtime.RawExtension { | ||
| // Convert input to []runtime.RawExtension | ||
| var rawExtensions []runtime.RawExtension | ||
|
|
||
| switch v := objs.(type) { | ||
| case []runtime.RawExtension: | ||
| rawExtensions = v | ||
| case []runtime.Object, []runtimeclient.Object: | ||
| // Handle both runtime.Object and runtimeclient.Object the same way | ||
| // since runtimeclient.Object embeds runtime.Object | ||
| var objects []runtime.Object | ||
| if runtimeObjs, ok := v.([]runtime.Object); ok { | ||
| objects = runtimeObjs |
There was a problem hiding this comment.
Using interface{} looks like an overkill and similar to a python code - let's stay with Golang code and the standard approach to keep the code sane.
Also, based on the latest changes changes in the other PR, do we actually need to support the Object types?
There was a problem hiding this comment.
yea i was actually thinking the same, this PR is now not needed , closing it
|
closing this PR since now the support for different object types is not required |
2 participants
This PR is related to the discussion codeready-toolchain/member-operator#680 (comment)
to improve the filter function
related PR
Member-operator: codeready-toolchain/member-operator#692
Summary by CodeRabbit
New Features
Tests