Add provider validation to deployment commands#35
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 (3)
✨ 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 enhances 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 adds provider version validation to the deploy commands for single, multiple, and replication sandboxes. The implementation correctly integrates with the new provider registry. However, the validation logic has been duplicated across three different files. My review includes suggestions to refactor this duplicated code into a shared helper function to improve code maintainability and adhere to the DRY (Don't Repeat Yourself) principle.
| // Validate version with provider | ||
| // 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.
This provider validation logic is duplicated in cmd/replication.go and cmd/single.go. To improve maintainability and follow the DRY (Don't Repeat Yourself) principle, this logic should be extracted into a common helper function. For example, you could create a validateProvider(sd sandbox.SandboxDef) function in cmd/single.go (alongside fillSandboxDefinition) and call it from all three command files.
| // Validate version with provider | ||
| // 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.
This provider validation logic is duplicated in cmd/multiple.go and cmd/single.go. To improve maintainability and follow the DRY (Don't Repeat Yourself) principle, this logic should be extracted into a common helper function. For example, you could create a validateProvider(sd sandbox.SandboxDef) function in cmd/single.go (alongside fillSandboxDefinition) and call it from all three command files.
| // Validate version with provider | ||
| // 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.
This provider validation logic is duplicated in cmd/multiple.go and cmd/replication.go. To improve maintainability and follow the DRY (Don't Repeat Yourself) principle, this logic should be extracted into a common helper function. For example, you could create a validateProvider(sd sandbox.SandboxDef) function in this file (alongside fillSandboxDefinition) and call it from all three command files.
There was a problem hiding this comment.
Pull request overview
Adds a provider-backed version validation step to the CLI deployment flows so that sd.Version is checked via providers.DefaultRegistry before sandbox creation, aligning command behavior with the emerging provider abstraction.
Changes:
- Add MySQL provider lookup +
ValidateVersion(sd.Version)todeploy singlebeforeCreateStandaloneSandbox. - Add the same provider validation step to
deploy replicationbeforeCreateReplicationSandbox. - Add the same provider validation step to
deploy multiplebeforeCreateMultipleSandbox.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| cmd/single.go | Performs provider version validation after fillSandboxDefinition() and before standalone sandbox creation. |
| cmd/replication.go | Performs provider version validation before replication sandbox creation. |
| cmd/multiple.go | Performs provider version validation before multiple sandbox creation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Validate version with provider | ||
| // 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 + version validation logic is duplicated across multiple deployment commands (single/replication/multiple). Consider extracting this into a shared helper (e.g., validateProviderVersion(sd) in cmd) to reduce drift and make Phase 2b (provider-from-flavor) a smaller change.
| 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 failure message version validation failed doesn’t include which provider and version were being validated. Including both (e.g., provider name and sd.Version) would make CLI failures actionable, especially once multiple providers are supported.
| common.Exitf(1, "version validation failed: %s", provErr) | |
| common.Exitf(1, "version validation failed for provider %s, version %s: %s", "mysql", sd.Version, provErr) |
| // Validate version with provider | ||
| // 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 + version validation logic is duplicated across multiple deployment commands (single/replication/multiple). Consider extracting this into a shared helper (e.g., validateProviderVersion(sd) in cmd) to reduce drift and make Phase 2b (provider-from-flavor) a smaller change.
| 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 failure message version validation failed doesn’t include which provider and version were being validated. Including both (e.g., provider name and sd.Version) would make CLI failures actionable, especially once multiple providers are supported.
| 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) | |
| providerName := "mysql" | |
| p, provErr := providers.DefaultRegistry.Get(providerName) | |
| if provErr != nil { | |
| common.Exitf(1, "provider error: %s", provErr) | |
| } | |
| if provErr = p.ValidateVersion(sd.Version); provErr != nil { | |
| common.Exitf(1, "version validation failed for provider %q and version %q: %s", providerName, sd.Version, provErr) |
| // Validate version with provider | ||
| // 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 + version validation logic is duplicated across multiple deployment commands (single/replication/multiple). Consider extracting this into a shared helper (e.g., validateProviderVersion(sd) in cmd) to reduce drift and make Phase 2b (provider-from-flavor) a smaller change.
| 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 failure message version validation failed doesn’t include which provider and version were being validated. Including both (e.g., provider name and sd.Version) would make CLI failures actionable, especially once multiple providers are supported.
| common.Exitf(1, "version validation failed: %s", provErr) | |
| common.Exitf(1, "version validation failed for provider %s, version %s: %s", "mysql", sd.Version, provErr) |
Closes #28
Closes #29
Summary
cmd/single.goafterfillSandboxDefinition()and beforeCreateStandaloneSandbox()cmd/replication.go(beforeCreateReplicationSandbox) andcmd/multiple.go(beforeCreateMultipleSandbox)sd.Versionagainst the MySQL provider viaproviders.DefaultRegistryTest plan
go buildsucceedsdbdeployer deploy single 8.4.4deploys successfullydbdeployer delete all --skip-confirmcleans up