-
Notifications
You must be signed in to change notification settings - Fork 2
feat: Phase 3 — PostgreSQL provider #54
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
Changes from all commits
8e0e623
aa7a81e
4be2b3b
65e5551
1c8d32b
ba2c992
fa6e1d8
a93fb3c
8038ebd
768d9c4
99dd2b9
4eb2fd3
9f74117
00451ca
d6ec6aa
cadfe55
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,92 @@ | ||
| package cmd | ||
|
|
||
| import ( | ||
| "fmt" | ||
| "path" | ||
|
|
||
| "github.com/ProxySQL/dbdeployer/common" | ||
| "github.com/ProxySQL/dbdeployer/defaults" | ||
| "github.com/ProxySQL/dbdeployer/providers" | ||
| "github.com/ProxySQL/dbdeployer/providers/postgresql" | ||
| "github.com/spf13/cobra" | ||
| ) | ||
|
|
||
| func deploySandboxPostgreSQL(cmd *cobra.Command, args []string) { | ||
| version := args[0] | ||
| flags := cmd.Flags() | ||
| skipStart, _ := flags.GetBool("skip-start") | ||
|
|
||
| p, err := providers.DefaultRegistry.Get("postgresql") | ||
| if err != nil { | ||
| common.Exitf(1, "PostgreSQL provider not available: %s", err) | ||
| } | ||
|
|
||
| if err := p.ValidateVersion(version); err != nil { | ||
| common.Exitf(1, "invalid version: %s", err) | ||
| } | ||
|
|
||
| if _, err := p.FindBinary(version); err != nil { | ||
| common.Exitf(1, "PostgreSQL binaries not found: %s\nRun: dbdeployer unpack --provider=postgresql <server.deb> <client.deb>", err) | ||
| } | ||
|
|
||
| port, err := postgresql.VersionToPort(version) | ||
| if err != nil { | ||
| common.Exitf(1, "error computing port: %s", err) | ||
| } | ||
| freePort, portErr := common.FindFreePort(port, []int{}, 1) | ||
| if portErr == nil { | ||
| port = freePort | ||
| } | ||
|
|
||
| sandboxHome := defaults.Defaults().SandboxHome | ||
| sandboxDir := path.Join(sandboxHome, fmt.Sprintf("pg_sandbox_%d", port)) | ||
|
|
||
| if common.DirExists(sandboxDir) { | ||
| common.Exitf(1, "sandbox directory %s already exists", sandboxDir) | ||
| } | ||
|
|
||
| config := providers.SandboxConfig{ | ||
| Version: version, | ||
| Dir: sandboxDir, | ||
| Port: port, | ||
| Host: "127.0.0.1", | ||
| DbUser: "postgres", | ||
| DbPassword: "", | ||
| Options: map[string]string{}, | ||
| } | ||
|
|
||
| if _, err := p.CreateSandbox(config); err != nil { | ||
| common.Exitf(1, "error creating PostgreSQL sandbox: %s", err) | ||
| } | ||
|
|
||
| if !skipStart { | ||
| if err := p.StartSandbox(sandboxDir); err != nil { | ||
| common.Exitf(1, "error starting PostgreSQL: %s", err) | ||
| } | ||
| } | ||
|
|
||
| fmt.Printf("PostgreSQL %s sandbox deployed in %s (port: %d)\n", version, sandboxDir, port) | ||
| } | ||
|
|
||
| var deployPostgreSQLCmd = &cobra.Command{ | ||
| Use: "postgresql version", | ||
| Short: "deploys a PostgreSQL sandbox", | ||
| Long: `postgresql deploys a standalone PostgreSQL instance as a sandbox. | ||
| It creates a sandbox directory with data, configuration, start/stop scripts, and a | ||
| psql client script. | ||
|
|
||
| Requires PostgreSQL binaries to be extracted first: | ||
| dbdeployer unpack --provider=postgresql postgresql-16_*.deb postgresql-client-16_*.deb | ||
|
|
||
| Example: | ||
| dbdeployer deploy postgresql 16.13 | ||
| dbdeployer deploy postgresql 17.1 --skip-start | ||
| `, | ||
| Args: cobra.ExactArgs(1), | ||
| Run: deploySandboxPostgreSQL, | ||
| } | ||
|
|
||
| func init() { | ||
| deployCmd.AddCommand(deployPostgreSQLCmd) | ||
| deployPostgreSQLCmd.Flags().Bool("skip-start", false, "Do not start PostgreSQL after deployment") | ||
| } | ||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -16,17 +16,106 @@ | |||||||||||||||||||
| package cmd | ||||||||||||||||||||
|
|
||||||||||||||||||||
| import ( | ||||||||||||||||||||
| "fmt" | ||||||||||||||||||||
| "os" | ||||||||||||||||||||
| "path" | ||||||||||||||||||||
| "strings" | ||||||||||||||||||||
|
|
||||||||||||||||||||
| "github.com/ProxySQL/dbdeployer/common" | ||||||||||||||||||||
| "github.com/ProxySQL/dbdeployer/defaults" | ||||||||||||||||||||
| "github.com/ProxySQL/dbdeployer/globals" | ||||||||||||||||||||
| "github.com/ProxySQL/dbdeployer/providers" | ||||||||||||||||||||
| "github.com/ProxySQL/dbdeployer/providers/postgresql" | ||||||||||||||||||||
| "github.com/ProxySQL/dbdeployer/sandbox" | ||||||||||||||||||||
| "github.com/spf13/cobra" | ||||||||||||||||||||
| ) | ||||||||||||||||||||
|
|
||||||||||||||||||||
| func deployMultipleNonMySQL(cmd *cobra.Command, args []string, providerName string) { | ||||||||||||||||||||
| flags := cmd.Flags() | ||||||||||||||||||||
| version := args[0] | ||||||||||||||||||||
| nodes, _ := flags.GetInt(globals.NodesLabel) | ||||||||||||||||||||
|
Comment on lines
+33
to
+36
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Missing argument validation before accessing Same pattern as other deploy commands: Move the validation before the provider check in func multipleSandbox(cmd *cobra.Command, args []string) {
flags := cmd.Flags()
+ common.CheckOrigin(args)
providerName, _ := flags.GetString(globals.ProviderLabel)
if providerName != "mysql" {
deployMultipleNonMySQL(cmd, args, providerName)
return
}
-
- common.CheckOrigin(args)🤖 Prompt for AI Agents |
||||||||||||||||||||
|
|
||||||||||||||||||||
| p, err := providers.DefaultRegistry.Get(providerName) | ||||||||||||||||||||
| if err != nil { | ||||||||||||||||||||
| common.Exitf(1, "provider error: %s", err) | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| flavor, _ := flags.GetString(globals.FlavorLabel) | ||||||||||||||||||||
| if flavor != "" { | ||||||||||||||||||||
| common.Exitf(1, "--flavor is only valid with --provider=mysql") | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| if !providers.ContainsString(p.SupportedTopologies(), "multiple") { | ||||||||||||||||||||
| common.Exitf(1, "provider %q does not support topology \"multiple\"\nSupported topologies: %s", | ||||||||||||||||||||
| providerName, strings.Join(p.SupportedTopologies(), ", ")) | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| if err := p.ValidateVersion(version); err != nil { | ||||||||||||||||||||
| common.Exitf(1, "version validation failed: %s", err) | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| if _, err := p.FindBinary(version); err != nil { | ||||||||||||||||||||
| common.Exitf(1, "binaries not found: %s", err) | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| basePort := p.DefaultPorts().BasePort | ||||||||||||||||||||
| if providerName == "postgresql" { | ||||||||||||||||||||
| basePort, _ = postgresql.VersionToPort(version) | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| sandboxHome := defaults.Defaults().SandboxHome | ||||||||||||||||||||
| topologyDir := path.Join(sandboxHome, fmt.Sprintf("%s_multi_%d", providerName, basePort)) | ||||||||||||||||||||
| if common.DirExists(topologyDir) { | ||||||||||||||||||||
| common.Exitf(1, "sandbox directory %s already exists", topologyDir) | ||||||||||||||||||||
| } | ||||||||||||||||||||
| os.MkdirAll(topologyDir, 0755) | ||||||||||||||||||||
|
Check failure on line 71 in cmd/multiple.go
|
||||||||||||||||||||
|
||||||||||||||||||||
| os.MkdirAll(topologyDir, 0755) | |
| if err := os.MkdirAll(topologyDir, 0755); err != nil { | |
| common.Exitf(1, "error creating sandbox directory %s: %s", topologyDir, err) | |
| } |
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.
Unchecked error from os.MkdirAll causes CI failure.
The pipeline failure confirms this issue. The error return value must be checked.
🐛 Proposed fix
- os.MkdirAll(topologyDir, 0755)
+ if err := os.MkdirAll(topologyDir, 0755); err != nil {
+ common.Exitf(1, "error creating topology directory %s: %s", topologyDir, err)
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| os.MkdirAll(topologyDir, 0755) | |
| if err := os.MkdirAll(topologyDir, 0755); err != nil { | |
| common.Exitf(1, "error creating topology directory %s: %s", topologyDir, err) | |
| } |
🧰 Tools
🪛 GitHub Actions: CI
[error] 71-71: golangci-lint: Error return value of os.MkdirAll is not checked (errcheck)
🪛 GitHub Check: Lint
[failure] 71-71:
Error return value of os.MkdirAll is not checked (errcheck)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/multiple.go` at line 71, The call to os.MkdirAll(topologyDir, 0755)
currently ignores its error; update the code around that call to capture the
returned error (err := os.MkdirAll(...)) and handle it appropriately — for
example, return the error up the call stack or log it and exit (using the
surrounding command's logger or fmt.Errorf/wrapping). Locate the os.MkdirAll
invocation in cmd/multiple.go and ensure the function (where the call appears)
propagates or handles the error instead of discarding it.
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.
Similar to deploy_postgresql.go, the error handling for common.FindFreePort here is not robust. If FindFreePort fails to find a free port, the original port (which might be in use) is kept. This can lead to subsequent errors during sandbox creation. Consider exiting or implementing a more robust port allocation strategy if a free port cannot be found.
| freePort, err := common.FindFreePort(port, []int{}, 1) | |
| if err == nil { | |
| port = freePort | |
| } | |
| freePort, err := common.FindFreePort(port, []int{}, 1) | |
| if err != nil { | |
| common.Exitf(1, "error finding a free port for node %d: %s", i, err) | |
| } | |
| port = freePort |
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.
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.
The current error handling for
common.FindFreePortis problematic. IfFindFreePortreturns an error (meaning it couldn't find a free port starting from the suggested one), the originalportvalue is retained. Thisportmight already be in use, leading to a subsequent failure whenCreateSandboxattempts to bind to it. It would be more robust to either exit with an error if no free port can be found or implement a retry mechanism to find an alternative port.