This repository was archived by the owner on Jun 2, 2026. It is now read-only.
refactor: Split component manager registry wiring#530
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (36)
💤 Files with no reviewable changes (8)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (24)
Summary by CodeRabbit
WalkthroughIntroduce a normalized component-manager catalog and typed errors, split static descriptors from runtime FactorySpec/factories, add a concurrency-safe Registry, wire builtin provider/manager initialization (config, decoder registry, provider registry, factory specs), update implementations and tests, and adjust serve initialization to use the new builtin provider registry. ChangesComponent Manager Catalog and Runtime Wiring
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
flow/internal/task/componentmanager/builtin/builtin_test.go (1)
96-105: ⚡ Quick winPrefer implementation constants for component-manager assertions.
These assertions validate manager implementation selection but compare against provider-name constants. Using implementation constants keeps the test aligned with the actual contract (
ComponentManagersmaps component type → implementation name).Proposed refactor
- assert.Equal(t, nicoprovider.ProviderName, componentManagers[devicetypes.ComponentTypeCompute]) - assert.Equal(t, nicoprovider.ProviderName, componentManagers[devicetypes.ComponentTypeNVLSwitch]) - assert.Equal(t, nicoprovider.ProviderName, componentManagers[devicetypes.ComponentTypePowerShelf]) + assert.Equal(t, computenico.ImplementationName, componentManagers[devicetypes.ComponentTypeCompute]) + assert.Equal(t, nvlswitchnico.ImplementationName, componentManagers[devicetypes.ComponentTypeNVLSwitch]) + assert.Equal(t, powershelfnico.ImplementationName, componentManagers[devicetypes.ComponentTypePowerShelf]) componentManagers[devicetypes.ComponentTypeCompute] = "mutated" assert.Equal( t, - nicoprovider.ProviderName, + computenico.ImplementationName, defaultServiceComponentManagers()[devicetypes.ComponentTypeCompute], )- assert.Equal(t, nicoprovider.ProviderName, config.ComponentManagers[devicetypes.ComponentTypeCompute]) + assert.Equal(t, computenico.ImplementationName, config.ComponentManagers[devicetypes.ComponentTypeCompute])Also applies to: 167-167
🤖 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 `@flow/internal/task/componentmanager/builtin/builtin_test.go` around lines 96 - 105, The test is asserting component manager selections against provider-name constants (nicoprovider.ProviderName); change those assertions to use the provider's implementation constant instead (the implementation-name constant exported by nicoprovider) so the test verifies the ComponentManagers mapping maps component types to implementation names; update all assertions referencing nicoprovider.ProviderName (including the mutation/assertion of defaultServiceComponentManagers()) to use the implementation constant from nicoprovider (e.g., the exported implementation constant) so they reflect the contract exposed by componentManagers and defaultServiceComponentManagers().
🤖 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 `@flow/docs/component-manager-architecture.md`:
- Around line 340-359: The example registration snippet is missing imports for
the packages that define cmconfig.Config and componentmanager.FactorySpec;
update the import block to include the correct package paths that provide
cmconfig (symbol: cmconfig.Config) and componentmanager (symbol:
componentmanager.FactorySpec) so the functions serviceFactorySpecs and
serviceDescriptors compile; specifically add the import entries for the packages
that declare componentmanager and cmconfig, then ensure the import aliases (if
any) match the usage in serviceFactorySpecs and serviceDescriptors.
In `@flow/internal/task/componentmanager/catalog/catalog.go`:
- Around line 77-88: The Catalog.Get method currently returns a Descriptor that
contains the RequiredProviders slice by reference, risking external mutation of
internal state; modify Catalog.Get to defensively clone the slice before
returning (use slices.Clone on Descriptor.RequiredProviders) so the returned
Descriptor contains an independent copy, ensuring callers cannot mutate the
catalog's internal backing array; update the return path in Catalog.Get to
create a copy of the descriptor with RequiredProviders set to
slices.Clone(descriptor.RequiredProviders) and then return that copy along with
the ok flag.
In `@flow/internal/task/componentmanager/registry.go`:
- Around line 168-177: The GetAllManagers method currently dereferences r
without checking for a nil receiver, causing a panic on nil calls; modify
Registry.GetAllManagers to first check if r == nil and immediately return an
empty []ComponentManager (zero-length slice) in that case, otherwise proceed to
acquire r.mu.RLock(), defer r.mu.RUnlock(), iterate over r.active and append
managers as before; this keeps behavior consistent with other read APIs and
avoids locking a nil receiver.
---
Nitpick comments:
In `@flow/internal/task/componentmanager/builtin/builtin_test.go`:
- Around line 96-105: The test is asserting component manager selections against
provider-name constants (nicoprovider.ProviderName); change those assertions to
use the provider's implementation constant instead (the implementation-name
constant exported by nicoprovider) so the test verifies the ComponentManagers
mapping maps component types to implementation names; update all assertions
referencing nicoprovider.ProviderName (including the mutation/assertion of
defaultServiceComponentManagers()) to use the implementation constant from
nicoprovider (e.g., the exported implementation constant) so they reflect the
contract exposed by componentManagers and defaultServiceComponentManagers().
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 00c2e358-bcbf-402b-8f04-b30200220512
📒 Files selected for processing (36)
flow/cmd/serve.goflow/docs/component-manager-architecture.mdflow/docs/flow-architecture.mdflow/internal/task/componentmanager/builtin/builtin_test.goflow/internal/task/componentmanager/builtin/component_manager_factories.goflow/internal/task/componentmanager/builtin/config.goflow/internal/task/componentmanager/builtin/config_test.goflow/internal/task/componentmanager/builtin/helpers.goflow/internal/task/componentmanager/builtin/manifest.goflow/internal/task/componentmanager/builtin/provider_config_decoders.goflow/internal/task/componentmanager/builtin/provider_config_decoders_test.goflow/internal/task/componentmanager/builtin/setup.goflow/internal/task/componentmanager/catalog/catalog.goflow/internal/task/componentmanager/catalog/catalog_test.goflow/internal/task/componentmanager/catalog/errors.goflow/internal/task/componentmanager/componentmanager.goflow/internal/task/componentmanager/componentmanager_test.goflow/internal/task/componentmanager/compute/nico/nico.goflow/internal/task/componentmanager/config/config.goflow/internal/task/componentmanager/config/config_test.goflow/internal/task/componentmanager/config/doc.goflow/internal/task/componentmanager/config/errors.goflow/internal/task/componentmanager/config/yaml.goflow/internal/task/componentmanager/errors.goflow/internal/task/componentmanager/factory_spec.goflow/internal/task/componentmanager/factory_spec_test.goflow/internal/task/componentmanager/manager.goflow/internal/task/componentmanager/mock/mock.goflow/internal/task/componentmanager/nvlswitch/nico/nico.goflow/internal/task/componentmanager/nvlswitch/nvswitchmanager/nvswitchmanager.goflow/internal/task/componentmanager/powershelf/nico/nico.goflow/internal/task/componentmanager/powershelf/psm/psm.goflow/internal/task/componentmanager/registry.goflow/internal/task/componentmanager/registry_test.goflow/internal/task/componentmanager/test_helpers_test.goflow/internal/task/executor/temporalworkflow/activity/activity_test.go
💤 Files with no reviewable changes (8)
- flow/internal/task/componentmanager/builtin/config.go
- flow/internal/task/componentmanager/builtin/config_test.go
- flow/internal/task/componentmanager/builtin/provider_config_decoders_test.go
- flow/internal/task/componentmanager/componentmanager_test.go
- flow/internal/task/componentmanager/config/doc.go
- flow/internal/task/componentmanager/builtin/provider_config_decoders.go
- flow/internal/task/componentmanager/componentmanager.go
- flow/internal/task/componentmanager/builtin/component_manager_factories.go
jw-nvidia
force-pushed
the
refactor/cm-providers
branch
from
ae2e5b2 to
1c6e1f2
Compare
Contributor
Author
|
/ok to test 1c6e1f2 |
🔐 TruffleHog Secret Scan✅ No secrets or credentials found! Your code has been scanned for 700+ types of secrets and credentials. All clear! 🎉 🕐 Last updated: 2026-05-14 21:22:04 UTC | Commit: 1c6e1f2 |
🔍 Container Scan Summary
Per-CVE detail lives in the per-service |
kunzhao-nv
approved these changes
May 15, 2026
- Separate component manager descriptors into a catalog package and split registry construction into manager, factory spec, and registry files. - Consolidate builtin service setup around a manifest, provider registry construction, and focused tests. Signed-off-by: Jin Wang <jinwan@nvidia.com>
jw-nvidia
force-pushed
the
refactor/cm-providers
branch
from
1c6e1f2 to
217bff5
Compare
Contributor
Author
|
/ok to test 217bff5 |
This was referenced May 18, 2026
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
2 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.
Description
Type of Change
Services Affected
Related Issues (Optional)
Breaking Changes
Testing
Additional Notes