refactor(detector): Attempt to add WithStoreProvider, including interface definitions#19927
Conversation
|
Skipping CI for Draft Pull Request. |
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The new
WithStoreProviderondetector.Buildernow overlaps with the existingWithXStore/With*LabelProvidermethods; consider either deprecating the more granular setters or documenting/preventing mixed usage so it’s clear which values win if both are called. - There are now two different
StoreProvidertypes (the newdetector.StoreProviderinterface and the existing KubernetesStoreProviderstruct) with the same name in different packages; consider renaming one (e.g.DetectorStoreProviderorReadOnlyStoreProvider) to reduce ambiguity when reading or searching the codebase.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new `WithStoreProvider` on `detector.Builder` now overlaps with the existing `WithXStore`/`With*LabelProvider` methods; consider either deprecating the more granular setters or documenting/preventing mixed usage so it’s clear which values win if both are called.
- There are now two different `StoreProvider` types (the new `detector.StoreProvider` interface and the existing Kubernetes `StoreProvider` struct) with the same name in different packages; consider renaming one (e.g. `DetectorStoreProvider` or `ReadOnlyStoreProvider`) to reduce ambiguity when reading or searching the codebase.
## Individual Comments
### Comment 1
<location path="sensor/common/detector/builder.go" line_range="26-34" />
<code_context>
"github.com/stackrox/rox/sensor/common/updater"
)
+// StoreProvider provides access to the stores needed by the detector.
+type StoreProvider interface {
+ Deployments() store.DeploymentStore
+ ServiceAccounts() store.ServiceAccountStore
+ NetworkPolicies() store.NetworkPolicyStore
+ Nodes() store.NodeStore
+ Registries() registry.Provider
+ ClusterLabels() scopecomp.ClusterLabelProvider
+ NamespaceLabels() scopecomp.NamespaceLabelProvider
+}
+
</code_context>
<issue_to_address>
**suggestion:** Consider reusing or embedding the existing store.Provider to avoid overlapping store-provider abstractions.
This new StoreProvider overlaps with sensor/common/store.Provider (Deployments, ServiceAccounts, NetworkPolicies, Nodes, Registries). To avoid drift between two similar interfaces, consider either accepting store.Provider here and defining a separate interface only for the label providers, or embedding store.Provider in StoreProvider and adding ClusterLabels/NamespaceLabels. That keeps the API smaller and reduces divergence risk.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| // StoreProvider provides access to the stores needed by the detector. | ||
| type StoreProvider interface { | ||
| Deployments() store.DeploymentStore | ||
| ServiceAccounts() store.ServiceAccountStore | ||
| NetworkPolicies() store.NetworkPolicyStore | ||
| Nodes() store.NodeStore | ||
| Registries() registry.Provider | ||
| ClusterLabels() scopecomp.ClusterLabelProvider | ||
| NamespaceLabels() scopecomp.NamespaceLabelProvider |
There was a problem hiding this comment.
suggestion: Consider reusing or embedding the existing store.Provider to avoid overlapping store-provider abstractions.
This new StoreProvider overlaps with sensor/common/store.Provider (Deployments, ServiceAccounts, NetworkPolicies, Nodes, Registries). To avoid drift between two similar interfaces, consider either accepting store.Provider here and defining a separate interface only for the label providers, or embedding store.Provider in StoreProvider and adding ClusterLabels/NamespaceLabels. That keeps the API smaller and reduces divergence risk.
🚀 Build Images ReadyImages are ready for commit ba301fe. To use with deploy scripts: export MAIN_IMAGE_TAG=4.11.x-578-gba301fe9a6 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## giles/ROX-33799-refactor-detector-constructor #19927 +/- ##
=================================================================================
- Coverage 49.58% 49.57% -0.02%
=================================================================================
Files 2764 2764
Lines 208402 208414 +12
=================================================================================
- Hits 103343 103326 -17
- Misses 97394 97426 +32
+ Partials 7665 7662 -3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Description
change me!
User-facing documentation
Testing and quality
Automated testing
How I validated my change
change me!