Define Provider interface and ProviderRegistry#32
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request lays the groundwork for a flexible database infrastructure deployment system by introducing a Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a Provider interface and a ProviderRegistry, which is a solid step towards abstracting database deployment logic. The code is well-structured and includes tests. My review focuses on improving the robustness and concurrency safety of the new Registry, which is crucial given the global DefaultRegistry. I have also provided suggestions to enhance the test suite by making it more idiomatic and improving error handling.
| type Registry struct { | ||
| providers map[string]Provider | ||
| } |
There was a problem hiding this comment.
The Registry is not safe for concurrent use. The DefaultRegistry is a global variable, which makes concurrent access from multiple goroutines likely as the application grows. This can lead to race conditions when reading from and writing to the providers map.
To fix this, you should add a sync.RWMutex to the Registry struct and use it to protect access to the providers map in Register, Get, and List methods.
For example:
// In Register
r.mu.Lock()
defer r.mu.Unlock()
// In Get and List
r.mu.RLock()
defer r.mu.RUnlock()You will also need to add "sync" to your imports.
| type Registry struct { | |
| providers map[string]Provider | |
| } | |
| type Registry struct { | |
| mu sync.RWMutex | |
| providers map[string]Provider | |
| } |
| func TestRegistryDuplicateRegister(t *testing.T) { | ||
| reg := NewRegistry() | ||
| mock := &mockProvider{name: "test"} | ||
| _ = reg.Register(mock) |
There was a problem hiding this comment.
It's a good practice to check for errors even on the first registration call. While it's unlikely to fail in this test, explicitly checking the error makes the test more robust and clearly states the assumption that the first registration should succeed.
if err := reg.Register(mock); err != nil {
t.Fatalf("initial register failed: %v", err)
}| _ = reg.Register(&mockProvider{name: "b"}) | ||
| _ = reg.Register(&mockProvider{name: "a"}) |
There was a problem hiding this comment.
It's a good practice to check for errors on registration calls. While it's unlikely to fail in this test, explicitly checking the errors makes the test more robust and clearly states the assumption that these registrations should succeed.
if err := reg.Register(&mockProvider{name: "b"}); err != nil {
t.Fatalf("register 'b' failed: %v", err)
}
if err := reg.Register(&mockProvider{name: "a"}); err != nil {
t.Fatalf("register 'a' failed: %v", err)
}| if len(names) != 2 { | ||
| t.Errorf("expected 2 providers, got %d", len(names)) | ||
| } | ||
| if names[0] != "a" || names[1] != "b" { | ||
| t.Errorf("expected sorted [a b], got %v", names) | ||
| } |
There was a problem hiding this comment.
For comparing slices, it's more idiomatic and scalable to use reflect.DeepEqual. This also checks the length, so you can combine the length and content checks into one. This makes the test easier to read and maintain.
You will need to add "reflect" to your imports.
expected := []string{"a", "b"}
if !reflect.DeepEqual(names, expected) {
t.Errorf("expected sorted %v, got %v", expected, names)
}There was a problem hiding this comment.
Pull request overview
Introduces the initial providers/ package to establish a minimal Provider abstraction and a registry for looking up providers by name (foundation for Phase 2a).
Changes:
- Added
Providerinterface andPortRangedefinition. - Added a provider registry with
Register,Get, andList, plus a global default registry. - Added unit tests covering basic registry behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
providers/provider.go |
Defines the Provider interface and implements a registry for provider discovery. |
providers/provider_test.go |
Adds unit tests for registry registration, lookup, duplicate handling, and listing order. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func (r *Registry) Register(p Provider) error { | ||
| name := p.Name() | ||
| if _, exists := r.providers[name]; exists { | ||
| return fmt.Errorf("provider %q already registered", name) | ||
| } | ||
| r.providers[name] = p | ||
| return nil |
There was a problem hiding this comment.
Register can panic in two common cases: (1) callers may use the exported Registry zero value (so r.providers is nil and assignment panics), and (2) callers may pass a nil Provider (so p.Name() panics). Consider making the zero value usable by initializing r.providers on first use, and validating inputs (nil provider / empty provider name) with a returned error instead of panicking.
| type Registry struct { | ||
| providers map[string]Provider | ||
| } | ||
|
|
||
| func NewRegistry() *Registry { | ||
| return &Registry{providers: make(map[string]Provider)} | ||
| } |
There was a problem hiding this comment.
The PR/plan refer to a ProviderRegistry, but the exported type is currently named just Registry. Since this is a public API and will be imported from other packages, the generic name can be ambiguous (e.g., providers.Registry). Consider renaming to ProviderRegistry (and NewProviderRegistry, etc.) or aligning the PR title/docs to match the chosen public type name.
| t.Fatal("expected error on duplicate register") | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Registry tests cover the happy path/duplicate/not-found/list, but they don't cover invalid inputs that are currently possible via the public API (e.g., Register(nil) and registering a provider with an empty name). Adding tests for these cases will lock in the intended behavior once Register is hardened to return errors instead of panicking.
| func TestRegistryRegisterNilProvider(t *testing.T) { | |
| reg := NewRegistry() | |
| if err := reg.Register(nil); err == nil { | |
| t.Fatal("expected error when registering nil provider") | |
| } | |
| if names := reg.List(); len(names) != 0 { | |
| t.Fatalf("expected no providers to be registered, got %v", names) | |
| } | |
| } | |
| func TestRegistryRegisterEmptyName(t *testing.T) { | |
| reg := NewRegistry() | |
| mock := &mockProvider{name: ""} | |
| if err := reg.Register(mock); err == nil { | |
| t.Fatal("expected error when registering provider with empty name") | |
| } | |
| if names := reg.List(); len(names) != 0 { | |
| t.Fatalf("expected no providers to be registered, got %v", names) | |
| } | |
| } |
Closes #25