Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds a configuration option to disable SSL for PostgreSQL database connections. The change introduces a new DisableSSL boolean field that controls whether sslmode=disable is appended to the connection string.
- Added
DisableSSLboolean field toDatabaseConfigstruct - Loads
DisableSSLfromDATABASE_CONFIG_DISABLE_SSLenvironment variable - Conditionally appends
?sslmode=disableto connection string whenDisableSSLis true
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| databaseConfig.go | Adds DisableSSL boolean field to the configuration struct |
| loadConfig.go | Parses DATABASE_CONFIG_DISABLE_SSL environment variable and handles errors |
| getDatabaseConnectionString.go | Conditionally appends ?sslmode=disable query parameter when SSL is disabled |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
loadConfig.go
Outdated
| loadedConfig.DisableSSL, err = strconv.ParseBool(os.Getenv("DATABASE_CONFIG_DISABLE_SSL")) | ||
| if err != nil { | ||
| return fmt.Errorf("Invalid DisableSSL value: %v", err) |
There was a problem hiding this comment.
The strconv.ParseBool function returns an error when the environment variable is empty or not set. This makes DisableSSL a required configuration field, which is likely not the intended behavior for an optional feature like disabling SSL. Consider providing a default value (false) when the environment variable is not set or empty, similar to how DatabasePort has a default value on line 32. This would allow the feature to be truly optional.
| loadedConfig.DisableSSL, err = strconv.ParseBool(os.Getenv("DATABASE_CONFIG_DISABLE_SSL")) | |
| if err != nil { | |
| return fmt.Errorf("Invalid DisableSSL value: %v", err) | |
| disableSSLStr := os.Getenv("DATABASE_CONFIG_DISABLE_SSL") | |
| if disableSSLStr != "" { | |
| loadedConfig.DisableSSL, err = strconv.ParseBool(disableSSLStr) | |
| if err != nil { | |
| return fmt.Errorf("Invalid DisableSSL value: %v", err) | |
| } |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return fmt.Sprintf("postgres://%s:%s@%s:%d/%s", activeConfig.DatabaseUser, activeConfig.DatabasePass, activeConfig.DatabaseHost, activeConfig.DatabasePort, activeConfig.DatabaseName) | ||
| dbString := fmt.Sprintf("postgres://%s:%s@%s:%d/%s", activeConfig.DatabaseUser, activeConfig.DatabasePass, activeConfig.DatabaseHost, activeConfig.DatabasePort, activeConfig.DatabaseName) | ||
| if activeConfig.DisableSSL { | ||
| dbString += "?sslmode=disable" |
There was a problem hiding this comment.
When DisableSSL is false, no sslmode parameter is added to the connection string. This means the default behavior depends on the PostgreSQL driver's default, which may vary. Consider explicitly setting sslmode=require (or another appropriate mode like sslmode=verify-full) when SSL is enabled to ensure consistent and secure behavior across different environments.
| dbString += "?sslmode=disable" | |
| dbString += "?sslmode=disable" | |
| } else { | |
| dbString += "?sslmode=require" |
1 participant
No description provided.