-
Notifications
You must be signed in to change notification settings - Fork 4
Filter child resource if resource type not present in filter #607
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
WalkthroughIntroduces client-side filtering for sub-resources during traversal in the syncer. A syncResourceTypeMap caches configured sync resource types, and ChildResourceType annotation parsing now conditionally skips pushing sub-resource actions unless the child resource type is in the configured filter. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
pkg/sync/syncer.go (1)
851-854: Consider caching the resource type map to avoid repeated allocations.The pattern of creating
syncResourceTypeMapfroms.syncResourceTypesnow appears three times in this file (lines 374-379, 786-789, and 851-854). SincegetSubResourcescan be called multiple times during a sync, recreating the map on each invocation is inefficient.🔎 Proposed refactor to cache the map
Add a cached map field to the syncer struct and initialize it once:
type syncer struct { c1zManager manager.Manager c1zPath string externalResourceC1ZPath string externalResourceEntitlementIdFilter string store connectorstore.Writer externalResourceReader connectorstore.Reader connector types.ConnectorClient state State runDuration time.Duration transitionHandler func(s Action) progressHandler func(p *Progress) tmpDir string skipFullSync bool lastCheckPointTime time.Time counts *ProgressCounts targetedSyncResources []*v2.Resource onlyExpandGrants bool dontExpandGrants bool syncID string skipEGForResourceType map[string]bool skipEntitlementsForResourceType map[string]bool skipEntitlementsAndGrants bool skipGrants bool resourceTypeTraits map[string][]v2.ResourceType_Trait syncType connectorstore.SyncType injectSyncIDAnnotation bool setSessionStore sessions.SetSessionStore syncResourceTypes []string + syncResourceTypeMap map[string]bool }Then initialize it once in the
Syncmethod after lines 374-379 (or inNewSyncer):syncResourceTypeMap := make(map[string]bool) if len(s.syncResourceTypes) > 0 { for _, rt := range s.syncResourceTypes { syncResourceTypeMap[rt] = true } } + s.syncResourceTypeMap = syncResourceTypeMapAnd replace all three map creation sites with:
- syncResourceTypeMap := make(map[string]bool) - for _, rt := range s.syncResourceTypes { - syncResourceTypeMap[rt] = true - } + // Use cached map from s.syncResourceTypeMap
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pkg/sync/syncer.go
🧰 Additional context used
🧬 Code graph analysis (1)
pkg/sync/syncer.go (2)
pb/c1/connector/v2/annotation_resource_tree.pb.go (2)
ChildResourceType(25-30)ChildResourceType(43-43)pb/c1/connector/v2/annotation_resource_tree_protoopaque.pb.go (2)
ChildResourceType(25-30)ChildResourceType(43-43)
⏰ 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: go-test (1.25.2, windows-latest)
🔇 Additional comments (1)
pkg/sync/syncer.go (1)
863-867: Filtering logic correctly implements the PR objective.The implementation properly checks if a resource type filter is configured and skips child resource actions for types not in the filter. This is consistent with the filtering approach used elsewhere in the codebase (e.g., lines 784-797 in
SyncResourceTypes).
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.