Add 'dbdeployer providers' CLI command#34
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 introduces a new command to the 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 new dbdeployer providers command to list available deployment providers, which is a useful addition for discoverability. The implementation is clean. I've found one potential issue in the new cmd/providers.go file regarding error handling that could lead to a panic. My specific comment provides a suggestion to fix it. The changes in cmd/single.go for version validation are a good step towards more robust deployments.
| Long: "Lists all registered providers that can be used for sandbox deployment", | ||
| Run: func(cmd *cobra.Command, args []string) { | ||
| for _, name := range providers.DefaultRegistry.List() { | ||
| p, _ := providers.DefaultRegistry.Get(name) |
There was a problem hiding this comment.
The error returned from providers.DefaultRegistry.Get(name) is ignored. While it's unlikely to fail in the current logic (as you're iterating over a list from the same registry), not handling the error could lead to a panic on the next line if p is nil. It's safer to handle the error.
You will also need to import the os package to use os.Stderr.
p, err := providers.DefaultRegistry.Get(name)
if err != nil {
fmt.Fprintf(os.Stderr, "error getting provider %q: %v\n", name, err)
continue
}There was a problem hiding this comment.
Pull request overview
Adds CLI support for enumerating registered deployment providers and begins integrating the provider abstraction into the single-sandbox deployment flow.
Changes:
- Introduces
dbdeployer providerscommand to list registered providers and their default port ranges. - Adds provider-based version validation step to
deploy single. - Wires the command into the Cobra root command.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| cmd/single.go | Adds provider lookup and ValidateVersion call before creating a single sandbox. |
| cmd/providers.go | New providers root command that lists entries from providers.DefaultRegistry. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // TODO: Phase 2b — determine provider from sd.Flavor instead of hardcoding "mysql" | ||
| p, provErr := providers.DefaultRegistry.Get("mysql") | ||
| if provErr != nil { | ||
| common.Exitf(1, "provider error: %s", provErr) | ||
| } | ||
| if provErr = p.ValidateVersion(sd.Version); provErr != nil { | ||
| common.Exitf(1, "version validation failed: %s", provErr) | ||
| } |
There was a problem hiding this comment.
The provider lookup is hardcoded to "mysql" even though fillSandboxDefinition detects/sets sd.Flavor. This will validate versions against the wrong provider for non-MySQL flavors (e.g. MariaDB/Percona) and will likely break once providers implement flavor-specific ValidateVersion logic. Please select the provider based on sd.Flavor (or skip provider validation when no matching provider is registered) instead of hardcoding "mysql".
| // TODO: Phase 2b — determine provider from sd.Flavor instead of hardcoding "mysql" | |
| p, provErr := providers.DefaultRegistry.Get("mysql") | |
| if provErr != nil { | |
| common.Exitf(1, "provider error: %s", provErr) | |
| } | |
| if provErr = p.ValidateVersion(sd.Version); provErr != nil { | |
| common.Exitf(1, "version validation failed: %s", provErr) | |
| } | |
| // Determine provider from sd.Flavor, defaulting to "mysql" for backward compatibility | |
| providerName := sd.Flavor | |
| if providerName == "" { | |
| providerName = "mysql" | |
| } | |
| p, provErr := providers.DefaultRegistry.Get(providerName) | |
| if provErr != nil { | |
| // For non-MySQL flavors, skip provider validation if no matching provider is registered | |
| if providerName == "mysql" { | |
| common.Exitf(1, "provider error: %s", provErr) | |
| } | |
| } else { | |
| if err := p.ValidateVersion(sd.Version); err != nil { | |
| common.Exitf(1, "version validation failed: %s", err) | |
| } | |
| } |
| package cmd | ||
|
|
||
| import ( |
There was a problem hiding this comment.
This new command file is missing the standard DBDeployer Apache 2.0 license header comment that appears at the top of the other cmd/*.go files (e.g. cmd/root.go:1-15, cmd/versions.go:1-15). Please add the same header here for consistency and to avoid licensing ambiguity.
| Long: "Lists all registered providers that can be used for sandbox deployment", | ||
| Run: func(cmd *cobra.Command, args []string) { | ||
| for _, name := range providers.DefaultRegistry.List() { | ||
| p, _ := providers.DefaultRegistry.Get(name) |
There was a problem hiding this comment.
providers.DefaultRegistry.Get(name) errors are currently ignored, which would lead to a nil dereference on p.DefaultPorts() if the registry ever becomes inconsistent. Even if this is unlikely given name comes from List(), it’s safer to handle the error (e.g., exit or print a clear message and continue).
| p, _ := providers.DefaultRegistry.Get(name) | |
| p, err := providers.DefaultRegistry.Get(name) | |
| if err != nil { | |
| fmt.Fprintf(cmd.ErrOrStderr(), "failed to get provider %q: %v\n", name, err) | |
| continue | |
| } |
Summary
dbdeployer providerscommand that lists all registered deployment providers with their port configurationDefaultRegistryfrom theproviderspackage to enumerate and display provider detailsTest plan
go buildsucceeds./dbdeployer providersoutputs:mysql (base port: 3306, ports per instance: 3)./dbdeployer --helplists theproviderscommandCloses #30