Initialize Configuration Management Using Viper #447
Conversation
WalkthroughThis pull request introduces command-line flag parsing in the main application to retrieve configuration file paths and log levels. The configuration is now loaded from a YAML file with structured sections for server, authentication, JWT, Yorkie, MongoDB, and storage options. Dependency management in the module is updated with new direct and indirect packages. Additionally, the internal configuration package receives comprehensive enhancements with functions for environment binding, default value assignments, and validations, alongside associated tests. Changes
Sequence Diagram(s)sequenceDiagram
participant M as Main
participant FP as parseFlag
participant LS as setLogLevel
participant CL as LoadConfig
participant CF as Config Functions
M->>FP: Retrieve configPath & loglevel
FP-->>M: Return configPath & loglevel
M->>LS: Set log level with provided flag
LS-->>M: Return success or error
M->>CL: Load configuration from configPath
CL->>CF: Bind env vars, read file, ensure defaults, validate
CF-->>CL: Return config or error
CL-->>M: Provide configuration object or error
Possibly related PRs
Suggested Reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🔇 Additional comments (7)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (12)
backend-go/internal/config/config.go (2)
20-43: Resolve documentation discrepancy regarding 'status message'.The docstring mentions returning "a status message describing which sources were used," but the function signature only returns
(*Config, error). Either remove that reference from the docstring or implement the status message functionality.-// It returns the populated Config, a status message describing which sources were used, and an error if any. +// It returns the populated Config and an error if any.
56-77: Validate handling of file path fallback.When no file path is provided, the system defaults to
config.yamlin the current directory. Confirm there's no confusion if a different directory is used, or if there's a user expectation to place config in an alternate location.backend-go/internal/config/server.go (1)
21-25: Port range validation is correct.Restricting the port range to
[1, 65535]prevents invalid server binding. Consider logging an additional hint if the port is out of range to guide users.backend-go/internal/config/yorkie.go (1)
29-34: API address validation is a good start.The
validate()method checksAPIAddrfor emptiness. Consider also validating valid URL structure to prevent misconfiguration.backend-go/internal/config/mongo.go (2)
9-9: Consider separating database name from default connection URIThe default connection URI includes the database name (
codepair), which is redundant sinceDatabaseNameis configured separately. This could lead to confusion if the database names don't match.-DefaultMongoConnectionURI = "mongodb://localhost:27017/codepair" +DefaultMongoConnectionURI = "mongodb://localhost:27017"
37-46: Enhance MongoDB configuration validationThe validation could be more comprehensive by checking for valid timeout values and URI format.
func (m *Mongo) validate() error { if m.ConnectionURI == "" { return fmt.Errorf("mongo connection URI cannot be empty") } if m.DatabaseName == "" { return fmt.Errorf("mongo database name cannot be empty") } + if m.ConnectionTimeout <= 0 { + return fmt.Errorf("connection timeout must be positive") + } + if m.PingTimeout <= 0 { + return fmt.Errorf("ping timeout must be positive") + } return nil }backend-go/internal/config/env.go (2)
3-37: Document environment variable requirementsAdd documentation to clarify which environment variables are required vs optional, and their expected formats.
Add this documentation above the map:
// EnvVarMap defines the mapping between configuration keys and their corresponding // environment variables. Required variables in production: // - OAUTH_GITHUB_CLIENT_ID, OAUTH_GITHUB_CLIENT_SECRET: GitHub OAuth credentials // - JWT_ACCESS_TOKEN_SECRET, JWT_REFRESH_TOKEN_SECRET: JWT signing secrets // - MONGO_CONNECTION_URI, MONGO_DATABASE_NAME: MongoDB connection details // - STORAGE_PROVIDER: Storage provider selection ("minio" or "s3") // For MinIO: STORAGE_MINIO_* variables // For S3: STORAGE_S3_* variables
4-4: Consider more specific naming for server port environment variableThe
SERVER_PORTenvironment variable could be more specific to avoid conflicts with other services.- "server.port": "SERVER_PORT", + "server.port": "CODEPAIR_SERVER_PORT",backend-go/internal/config/oauth.go (1)
34-39: Enhance OAuth validationThe validation only checks for empty strings. Consider adding URL format validation for the OAuth endpoints.
func (o *OAuth) validate() error { + if err := validateURL(o.Github.CallbackURL); err != nil { + return fmt.Errorf("invalid callback URL: %w", err) + } + if err := validateURL(o.Github.AuthorizationURL); err != nil { + return fmt.Errorf("invalid authorization URL: %w", err) + } if err := o.Github.validate(); err != nil { return err } return nil }backend-go/internal/config/storage.go (1)
82-87: Enhance storage validationThe Minio validation should also check for access and secret keys, and validate that the endpoint URL uses HTTPS in production environments.
func (m *Minio) validate() error { - if m.Bucket == "" || m.Endpoint == "" { - return fmt.Errorf("minio requires Bucket and Endpoint to be set") + if m.Bucket == "" || m.Endpoint == "" || m.AccessKey == "" || m.SecretKey == "" { + return fmt.Errorf("minio requires Bucket, Endpoint, AccessKey, and SecretKey to be set") + } + if !strings.HasPrefix(m.Endpoint, "https://") && os.Getenv("ENV") == "production" { + return fmt.Errorf("minio endpoint must use HTTPS in production") } return nil }backend-go/internal/config/config_test.go (1)
124-129: Add more test cases for config validationThe test suite should include negative test cases for invalid configurations, such as:
- Invalid URLs in OAuth config
- Invalid time durations
- Missing required fields
Example test case to add:
t.Run("invalid oauth config", func(t *testing.T) { const invalidOAuthYAML = ` OAuth: Github: ClientID: "test" ClientSecret: "test" CallbackURL: "invalid-url" ` filePath := writeTempConfigFile(t, "invalid_config.yaml", invalidOAuthYAML) _, err := config.LoadConfig(filePath) assert.Error(t, err) assert.Contains(t, err.Error(), "invalid callback URL") })backend-go/config.yaml (1)
17-19: Document time duration formatAdd comments explaining the accepted time duration format for expiration times.
+ # Duration format: number + unit (s: seconds, m: minutes, h: hours) AccessTokenExpirationTime: "24h" RefreshTokenExpirationTime: "168h"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
backend-go/go.sumis excluded by!**/*.sum
📒 Files selected for processing (12)
backend-go/cmd/codepair/main.go(1 hunks)backend-go/config.yaml(1 hunks)backend-go/go.mod(1 hunks)backend-go/internal/config/config.go(1 hunks)backend-go/internal/config/config_test.go(1 hunks)backend-go/internal/config/env.go(1 hunks)backend-go/internal/config/jwt.go(1 hunks)backend-go/internal/config/mongo.go(1 hunks)backend-go/internal/config/oauth.go(1 hunks)backend-go/internal/config/server.go(1 hunks)backend-go/internal/config/storage.go(1 hunks)backend-go/internal/config/yorkie.go(1 hunks)
🧰 Additional context used
🪛 golangci-lint (1.62.2)
backend-go/internal/config/oauth.go
[high] 9-9: G101: Potential hardcoded credentials
(gosec)
🔇 Additional comments (20)
backend-go/internal/config/config.go (5)
3-8: Consider maintaining consistency in import usage.These imports look appropriate for Viper-based configuration. However, if you adopt
errorsfrom the standard library, also ensure that you consistently usefmt.Errorfrather thanerrors.New+fmt.Sprintfto maintain code consistency.
10-18: Ensure mapstructure tags align with YAML configuration keys.The struct fields named
Server,OAuth,JWT,Yorkie,Mongo, andStorageare each assignedmapstructuretags that match the top-level sections in the YAML. Verify that the YAML keys are properly capitalized (Server:,OAuth:, etc.) to avoid unexpected parsing issues.
45-54: Confirm that all relevant environment variables are bound.
bindEnvironmentVariables()relies onEnvVarMapfor key-to-environment mappings. Ensure that all critical configuration parameters (especially sensitive credentials) have corresponding environment variable bindings to allow secure overrides in production.
79-86: Good use of default value enforcement.
ensureDefaultValue()properly sets defaults for each sub-config, ensuring the app doesn't crash from unset fields. This is a best practice for robust configuration management.
88-109: Validate sub-config error messages.The chain of validations is done cleanly, with each error wrapped using
fmt.Errorf(). Ensure each sub-config's error message is user-friendly and sufficiently descriptive for troubleshooting.backend-go/internal/config/server.go (3)
3-3: Import usage is minimal and appropriate.The
fmtimport is essential for error reporting in validation.
9-12: Mapstructure tag alignment.
Port int "mapstructure:\"Port\""is consistent with the YAML key naming in theServersection. Make sure theServer.Portkey in the YAML is capitalized exactly as "Port".
14-19: Great approach to default port.Enforcing a default port of
3001ensures the server will start even if the user omits a port setting.backend-go/internal/config/yorkie.go (4)
1-4: File structure looks solid.This file organizes Yorkie configuration cleanly under a dedicated struct, aiding clarity and maintainability.
5-9: Ensure default constants match production environment needs.
DefaultYorkieAPIAddris set tolocalhost:8080. Verify if this aligns with your deployments.
11-15: Field mapping is clear and straightforward.The
Yorkiestruct uses consistentmapstructuretags, which should align well with the YAML definition.
17-27: Proactively handle empty strings with defaults.Calling
ensureDefaultValue()sets safe defaults. This is beneficial if users omit them in their configuration.backend-go/go.mod (8)
6-8: Dependency Additions for Debugging, FS Notifications, and HCL Support
The new dependencies on lines 6–8:
github.com/davecgh/go-spew v1.1.2-0.20180830191138-d8f796af33cc // indirectgithub.com/fsnotify/fsnotify v1.7.0 // indirectgithub.com/hashicorp/hcl v1.0.0 // indirectare appropriate for better debugging (go-spew), file system event monitoring (fsnotify), and HCL parsing support. They align with the PR’s aim to enhance configuration management. Please ensure these dependencies are indeed required in your configuration setup.
11-11: Inclusion of Properties Library
The addition ofgithub.com/magiconair/properties v1.8.7 // indirectprovides utilities to work with property files. Verify that its use complements your configuration strategy (or is required transitively by another tool).
14-16: Mapping & Parsing Enhancements
The new dependencies added on lines 14–16:
github.com/mitchellh/mapstructure v1.5.0 // indirectgithub.com/pelletier/go-toml/v2 v2.2.2 // indirectgithub.com/pmezard/go-difflib v1.0.1-0.20181226105442-5d4384ee4fb2 // indirectsupport clearer mapping of YAML values to Go structs, TOML file parsing, and diffing functionalities. They directly support the PR objective to improve configuration management.
17-19: File-System Abstraction, Logging Shim, and Concurrency Utilities
The dependencies added on lines 17–19:
github.com/sagikazarmark/locafero v0.4.0 // indirectgithub.com/sagikazarmark/slog-shim v0.1.0 // indirectgithub.com/sourcegraph/conc v0.3.0 // indirectare likely intended to provide additional file-system abstraction, a logging interface shim, and concurrency utilities. Ensure their inclusion is justified in your configuration and application context to avoid unnecessary bloat.
20-25: Core Configuration & Environment Handling Libraries
The block from lines 20–25 adds several essential dependencies:
github.com/spf13/afero v1.11.0 // indirectgithub.com/spf13/cast v1.6.0 // indirectgithub.com/spf13/pflag v1.0.5 // indirectgithub.com/spf13/viper v1.19.0 // indirectgithub.com/stretchr/testify v1.10.0 // indirectgithub.com/subosito/gotenv v1.6.0 // indirectThese libraries are central to the PR’s new configuration management approach. Viper, in combination with pflag and cast, allows for flexible command-line and file/environment-based configuration, while testify and gotenv support testing and environment variable loading respectively. Confirm that these libraries integrate seamlessly with your intended configuration hierarchy.
28-29: Advanced Error Handling and Atomic Operations
The additions on lines 28–29:
go.uber.org/atomic v1.9.0 // indirectgo.uber.org/multierr v1.9.0 // indirectprovide robust support for atomic operations and the aggregation of multiple errors. These can be very helpful for ensuring thread-safe configuration updates and improved error handling.
31-31: Usage of Experimental Package
The inclusion ofgolang.org/x/exp v0.0.0-20230905200255-921286631fa9 // indirecton line 31 introduces an experimental package. Since such packages can sometimes be unstable, please verify that its use is justified and well-isolated, minimizing any risk in production deployments.
35-36: INI and YAML Parsing Support
The addition of:
gopkg.in/ini.v1 v1.67.0 // indirectgopkg.in/yaml.v3 v3.0.1 // indirectprovides enhanced support for configuration file formats (INI and YAML). This nicely complements the flexible configuration setup described in the PR objectives.
| func parseFlag() (string, string) { | ||
| var configPath string | ||
| var loglevel string | ||
| flag.StringVar(&configPath, "config", "", "Path to the configuration file") | ||
| flag.StringVar(&loglevel, "loglevel", "error", "Log level") | ||
| flag.Parse() | ||
|
|
||
| return configPath, loglevel | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add config file path validation
The parseFlag function should validate that the config file exists when a path is provided.
func parseFlag() (string, string) {
var configPath string
var loglevel string
flag.StringVar(&configPath, "config", "", "Path to the configuration file")
flag.StringVar(&loglevel, "loglevel", "error", "Log level")
flag.Parse()
+ if configPath != "" {
+ if _, err := os.Stat(configPath); err != nil {
+ log.Fatalf("Config file not found: %v", err)
+ }
+ }
return configPath, loglevel
}📝 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.
| func parseFlag() (string, string) { | |
| var configPath string | |
| var loglevel string | |
| flag.StringVar(&configPath, "config", "", "Path to the configuration file") | |
| flag.StringVar(&loglevel, "loglevel", "error", "Log level") | |
| flag.Parse() | |
| return configPath, loglevel | |
| } | |
| func parseFlag() (string, string) { | |
| var configPath string | |
| var loglevel string | |
| flag.StringVar(&configPath, "config", "", "Path to the configuration file") | |
| flag.StringVar(&loglevel, "loglevel", "error", "Log level") | |
| flag.Parse() | |
| if configPath != "" { | |
| if _, err := os.Stat(configPath); err != nil { | |
| log.Fatalf("Config file not found: %v", err) | |
| } | |
| } | |
| return configPath, loglevel | |
| } |
backend-go/config.yaml
Outdated
| ClientID: "your_client_id" | ||
| ClientSecret: "your_client_secret" | ||
| CallbackURL: "http://localhost:3000/auth/login/github" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use clear placeholder values for sensitive fields
Replace example values with descriptive placeholders to prevent accidental use in production. Also, add a comment warning about sensitive values.
- ClientID: "your_client_id"
- ClientSecret: "your_client_secret"
+ # WARNING: Replace these values with your GitHub OAuth credentials
+ ClientID: "<GITHUB_CLIENT_ID>"
+ ClientSecret: "<GITHUB_CLIENT_SECRET>"
- AccessTokenSecret: "your_access_token_secret"
- RefreshTokenSecret: "your_refresh_token_secret"
+ # WARNING: Use strong, unique secrets in production
+ AccessTokenSecret: "<STRONG_RANDOM_SECRET>"
+ RefreshTokenSecret: "<STRONG_RANDOM_SECRET>"Also applies to: 16-19
- viper cannot parse day
- viper use `mapstructure` and `yaml` is not needed
- return nil when config path is not given and config file not found
- `PORT` -> `SERVER_PORT`
window9u
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
backend-go/config.yaml (1)
32-46: Add security warnings and environment variable examples for storage credentialsThe storage configuration should include documentation about using environment variables for sensitive credentials.
Storage: - Provider: "minio" # or "s3" + # Provider can be either "minio" or "s3" + # Override credentials using environment variables: + # - CODEPAIR_STORAGE_MINIO_ACCESS_KEY + # - CODEPAIR_STORAGE_MINIO_SECRET_KEY + # - CODEPAIR_STORAGE_S3_ACCESS_KEY + # - CODEPAIR_STORAGE_S3_SECRET_KEY + Provider: "minio" Minio: Bucket: "default-storage" Endpoint: "http://localhost:9000" - AccessKey: "minioadmin" - SecretKey: "minioadmin" + AccessKey: "<MINIO_ACCESS_KEY>" # Default: minioadmin + SecretKey: "<MINIO_SECRET_KEY>" # Default: minioadmin S3: Bucket: "default-storage" Region: "us-east-1" - AccessKey: "aws_access_key" - SecretKey: "aws_secret_key" + AccessKey: "<AWS_ACCESS_KEY>" + SecretKey: "<AWS_SECRET_KEY>"
🧹 Nitpick comments (2)
backend-go/internal/config/storage.go (1)
36-44: Consider providing default S3 configuration as wellCurrently,
ensureDefaultValue()applies only to the Minio struct. For consistency, you may consider providing a similar defaulting mechanism for S3 (e.g., fallback region or bucket name) if S3 is used. This would improve parity between the two providers.backend-go/internal/config/config.go (1)
56-77: Enhance error messaging for clarityWhile the error handling is functional, consider making the fallback behavior more explicit in the error messages.
if errors.As(err, &nf) { if filePath != "" { - return fmt.Errorf("file path given but not found: %w", err) + return fmt.Errorf("config file not found at %s, falling back to environment variables: %w", filePath, err) } return nil } -return fmt.Errorf("failed to read config file: %w", err) +return fmt.Errorf("failed to read config file (check file format and permissions): %w", err)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
backend-go/go.sumis excluded by!**/*.sum
📒 Files selected for processing (12)
backend-go/cmd/codepair/main.go(1 hunks)backend-go/config.yaml(1 hunks)backend-go/go.mod(1 hunks)backend-go/internal/config/config.go(1 hunks)backend-go/internal/config/config_test.go(1 hunks)backend-go/internal/config/env.go(1 hunks)backend-go/internal/config/jwt.go(1 hunks)backend-go/internal/config/mongo.go(1 hunks)backend-go/internal/config/oauth.go(1 hunks)backend-go/internal/config/server.go(1 hunks)backend-go/internal/config/storage.go(1 hunks)backend-go/internal/config/yorkie.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- backend-go/cmd/codepair/main.go
- backend-go/internal/config/mongo.go
- backend-go/internal/config/jwt.go
- backend-go/internal/config/yorkie.go
- backend-go/internal/config/env.go
- backend-go/internal/config/config_test.go
🧰 Additional context used
🪛 golangci-lint (1.62.2)
backend-go/internal/config/oauth.go
[high] 9-9: G101: Potential hardcoded credentials
(gosec)
🪛 GitHub Actions: CI GO Backend
backend-go/internal/config/oauth.go
[error] 9-9: Potential hardcoded credentials (gosec)
🔇 Additional comments (20)
backend-go/internal/config/storage.go (6)
12-13: Remove default credentials from codeThese credentials pose a security risk, as flagged in a previous review. Setting them to empty strings will force configuration via environment variables or a config file:
- DefaultMinioAccessKey = "minioadmin" - DefaultMinioSecretKey = "minioadmin" + DefaultMinioAccessKey = "" + DefaultMinioSecretKey = ""
16-34: Struct definitions look solidNo issues found in the definitions of
Storage,S3, andMinio. They are well-structured and coherent with the rest of the codebase.
46-65: Validation of provider is correctYour logic correctly enforces that only “minio” or “s3” are valid providers, returning helpful error messages if either
MinioorS3configs are missing. Nicely done.
67-80: Minio default value assignment is properThe code ensures each needed field is assigned a default if not provided at runtime. This looks good.
82-87: Minio validation is sufficientEnsures required fields (
Bucket,Endpoint) are present, returning descriptive errors. Looks good.
89-94: S3 validation is sufficientChecks for required fields (
Bucket,Region,AccessKey,SecretKey) and returns clear errors. No concerns here.backend-go/internal/config/server.go (3)
3-12: Server struct changes look goodNo issues found with the
fmtimport or the addition ofmapstructure:"Port". This ensures correct mapping from configuration sources.
14-19: Default port assignment is appropriateThe
ensureDefaultValue()method nicely sets the port toDefaultServerPort(3001) if no custom port is specified. No concerns here.
21-25: Port validation is robustVerifying the port is within a valid range (1-65535) ensures safe usage and avoids common pitfalls. Good job.
backend-go/internal/config/oauth.go (7)
8-9: False positive from static analysisThe pipeline flagged these as potential “hardcoded credentials,” but they’re actually standard public GitHub OAuth endpoints. No real secret is exposed here.
🧰 Tools
🪛 golangci-lint (1.62.2)
[high] 9-9: G101: Potential hardcoded credentials
(gosec)
🪛 GitHub Actions: CI GO Backend
[error] 9-9: Potential hardcoded credentials (gosec)
11-11: Use HTTPS for default callback URLAs previously suggested, adopting HTTPS, even for localhost, implements more secure practices:
- DefaultGitHubCallbackURL = "http://localhost:3000/auth/login/github" + DefaultGitHubCallbackURL = "https://localhost:3000/auth/login/github"
14-25: The OAuth struct setup is clear
OAuthandGitHubOAuthare well-defined and align with other config structures.
27-32: Default value handling is consistentEnsuring that
o.Githubis not nil and callingensureDefaultValue()is a good pattern.
34-39: Validation flow for OAuth is correctRuns
Github.validate()and returns any error encountered. Straightforward and effective.
41-54: Proper fallback for GitHub endpointsSets the correct defaults if the user doesn’t provide them. Implementation looks spot-on.
56-65: Client credentials validation is adequateChecks for non-empty
ClientIDandClientSecret, returning informative errors. Nicely done.backend-go/internal/config/config.go (2)
11-18: Well-structured configuration layout!The Config struct is well-organized with clear section separation and proper mapstructure tags.
20-43: Excellent configuration loading implementation!The function implements a clear and robust configuration loading sequence that aligns with the PR objectives:
- Environment variables take precedence
- Config file as fallback
- Proper validation and default values
backend-go/config.yaml (1)
6-8: Use clear placeholder values for sensitive fieldsThe configuration file contains sensitive fields with unclear placeholder values.
Also applies to: 16-19
backend-go/go.mod (1)
5-40: Dependencies are well-managedThe addition of Viper and its dependencies is properly handled with specific version pins.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
backend-go/internal/config/storage.go (3)
7-12: Add documentation for configuration constants.Consider adding documentation comments for these constants to explain their purpose and when they are used, especially since they're part of the configuration management system.
const ( + // DefaultStorageProvider specifies the default storage backend to use if not configured DefaultStorageProvider = "minio" + // DefaultMinioBucket specifies the default bucket name for Minio storage DefaultMinioBucket = "default-storage" + // DefaultMinioEndpoint specifies the default Minio server endpoint for development DefaultMinioEndpoint = "http://localhost:9000" )
44-63: Simplify validation logic and add nil check.The validation can be simplified, and a nil check should be added for safety.
+func (s *Storage) validate() error { + if s == nil { + return fmt.Errorf("storage config is nil") + } + if s.Provider != "minio" && s.Provider != "s3" { return fmt.Errorf("invalid storage provider: %s (only 'minio' or 's3' are supported)", s.Provider) } - switch s.Provider { - case "minio": + if s.Provider == "minio" { if s.Minio == nil { return fmt.Errorf("storage provider is minio but Minio config is nil") } return s.Minio.validate() - case "s3": + } else { // s.Provider == "s3" if s.S3 == nil { return fmt.Errorf("storage provider is s3 but S3 config is nil") } return s.S3.validate() } - - return nil }
20-25: Add EnsureDefaultValue for S3 configuration.For consistency with Minio configuration, consider adding an EnsureDefaultValue method for S3 to set default region if needed.
type S3 struct { Bucket string `mapstructure:"bucket"` Region string `mapstructure:"region"` AccessKey string `mapstructure:"accessKey" sensitive:"true"` SecretKey string `mapstructure:"secretKey" sensitive:"true"` } + +func (s3 *S3) EnsureDefaultValue() { + if s3.Region == "" { + s3.Region = "us-east-1" // Common default region + } +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
backend-go/cmd/codepair/main.go(1 hunks)backend-go/config.yaml(1 hunks)backend-go/internal/config/config_test.go(1 hunks)backend-go/internal/config/jwt.go(1 hunks)backend-go/internal/config/oauth.go(1 hunks)backend-go/internal/config/storage.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- backend-go/cmd/codepair/main.go
- backend-go/internal/config/jwt.go
- backend-go/internal/config/oauth.go
- backend-go/config.yaml
- backend-go/internal/config/config_test.go
🔇 Additional comments (1)
backend-go/internal/config/storage.go (1)
1-5: LGTM!The package name and imports are appropriate for the configuration management purpose.
| type S3 struct { | ||
| Bucket string `mapstructure:"bucket"` | ||
| Region string `mapstructure:"region"` | ||
| AccessKey string `mapstructure:"accessKey"` | ||
| SecretKey string `mapstructure:"secretKey"` | ||
| } | ||
|
|
||
| type Minio struct { | ||
| Bucket string `mapstructure:"bucket"` | ||
| Endpoint string `mapstructure:"endpoint"` | ||
| AccessKey string `mapstructure:"accessKey"` | ||
| SecretKey string `mapstructure:"secretKey"` | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Mark credential fields as sensitive.
Add sensitive:"true" tag to credential fields to prevent them from being logged in plain text.
type S3 struct {
Bucket string `mapstructure:"bucket"`
Region string `mapstructure:"region"`
- AccessKey string `mapstructure:"accessKey"`
- SecretKey string `mapstructure:"secretKey"`
+ AccessKey string `mapstructure:"accessKey" sensitive:"true"`
+ SecretKey string `mapstructure:"secretKey" sensitive:"true"`
}
type Minio struct {
Bucket string `mapstructure:"bucket"`
Endpoint string `mapstructure:"endpoint"`
- AccessKey string `mapstructure:"accessKey"`
- SecretKey string `mapstructure:"secretKey"`
+ AccessKey string `mapstructure:"accessKey" sensitive:"true"`
+ SecretKey string `mapstructure:"secretKey" sensitive:"true"`
}📝 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.
| type S3 struct { | |
| Bucket string `mapstructure:"bucket"` | |
| Region string `mapstructure:"region"` | |
| AccessKey string `mapstructure:"accessKey"` | |
| SecretKey string `mapstructure:"secretKey"` | |
| } | |
| type Minio struct { | |
| Bucket string `mapstructure:"bucket"` | |
| Endpoint string `mapstructure:"endpoint"` | |
| AccessKey string `mapstructure:"accessKey"` | |
| SecretKey string `mapstructure:"secretKey"` | |
| } | |
| type S3 struct { | |
| Bucket string `mapstructure:"bucket"` | |
| Region string `mapstructure:"region"` | |
| AccessKey string `mapstructure:"accessKey" sensitive:"true"` | |
| SecretKey string `mapstructure:"secretKey" sensitive:"true"` | |
| } | |
| type Minio struct { | |
| Bucket string `mapstructure:"bucket"` | |
| Endpoint string `mapstructure:"endpoint"` | |
| AccessKey string `mapstructure:"accessKey" sensitive:"true"` | |
| SecretKey string `mapstructure:"secretKey" sensitive:"true"` | |
| } |
- edit invalid test url
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
backend-go/internal/config/storage.go (1)
44-60: Validate unselected providers.Currently, switching on
s.Providervalidates only one struct (either Minio or S3). If the user sets both inadvertently (or partially), it won’t be flagged. Consider warning or ignoring the unused struct to prevent confusion.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
backend-go/go.sumis excluded by!**/*.sum
📒 Files selected for processing (9)
backend-go/go.mod(1 hunks)backend-go/internal/config/config.go(1 hunks)backend-go/internal/config/config_test.go(1 hunks)backend-go/internal/config/jwt.go(1 hunks)backend-go/internal/config/mongo.go(1 hunks)backend-go/internal/config/oauth.go(1 hunks)backend-go/internal/config/server.go(1 hunks)backend-go/internal/config/storage.go(1 hunks)backend-go/internal/config/yorkie.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- backend-go/internal/config/jwt.go
- backend-go/internal/config/mongo.go
- backend-go/internal/config/oauth.go
- backend-go/internal/config/yorkie.go
- backend-go/internal/config/config_test.go
🔇 Additional comments (17)
backend-go/internal/config/storage.go (4)
7-11: Default credentials removed – good job!Previously, default Minio credentials were hard-coded. It's great to see they're no longer part of these constants, aligning with secure best practices.
19-24: Addsensitive:"true"tags to S3 credential fields.Marking the credential fields as sensitive helps avoid accidental logging or exposure of secrets. This was mentioned in a past review.
26-31: Addsensitive:"true"tags to Minio credential fields.Similarly, the
AccessKeyandSecretKeyin Minio should also be marked sensitive to prevent logging in plain text. This was also mentioned in a past review.
62-69: No default credentials for Minio – approved.By not setting default access/secret keys here, you reduce the risk of accidentally shipping insecure credentials, which is consistent with past security guidance.
backend-go/internal/config/server.go (4)
3-4: Import usage is fine.The
fmtpackage is necessary for error formatting. No issues here.
10-11: Port validation looks good.Requiring a valid port range (1–65535) ensures correct server binding. Well done.
13-18: Default port assignment is clear.Assigning
DefaultServerPortwhenPort == 0provides a sensible, easy-to-understand fallback.
20-26: Validation approach is consistent.Using
validate.Struct(s)ensures all struct fields are checked, returning a clear error message if anything is invalid.backend-go/internal/config/config.go (7)
3-9: Imports and validator initialization.The chosen packages (
errors,fmt,github.com/go-playground/validator/v10,github.com/spf13/viper) are appropriate for flexible configuration and validation. Good setup.
15-21: Expanded config struct.Including
Server,OAuth,JWT,Yorkie,Mongo, andStorageunderConfigorganizes settings effectively and promotes clarity.
23-46: Robust config loading with environment overrides.Binding environment variables and gracefully handling file-based config ensures flexible runtime configuration. The error messages provide clear diagnostics.
48-57: Environment variable binding loop.Systematically mapping config keys to environment variables reduces direct code duplication and fosters consistency. Solid approach.
59-80: Graceful fallback when config file is absent.Skipping errors if the file is not found (and no path is specified) is user-friendly, letting environment variables or defaults take precedence.
82-89: Default value application is thorough.All sub-configs (Server, OAuth, JWT, Yorkie, Mongo, and Storage) get sensible defaults before final validation.
91-112: Comprehensive validation across sub-configs.Validating each sub-config clarifies which section failed and simplifies debugging. Good error structuring.
backend-go/go.mod (2)
5-10: Reevaluate Direct Dependency Listing forgithub.com/labstack/gommon
The direct dependency block now includes critical libraries such asgithub.com/spf13/viper(for configuration management) andgithub.com/stretchr/testify(for testing), which aligns well with the PR objectives. However, note thatgithub.com/labstack/gommon v0.4.2is included as a direct dependency even though the PR summary mentioned that it should be removed from the main list and relegated to an indirect dependency. Please verify if this inclusion was intentional or if it needs adjusting for consistency with the overall configuration management improvements.
12-45: Review of Updated Indirect Dependency Block
The updated indirect dependency block brings in several new libraries—such asgithub.com/mitchellh/mapstructure, which aids in YAML-to-struct mapping—as well as various other helpers that support the new configuration and validation features. Ensure that all these indirect dependencies are actively required by the code base; if any are unused, runninggo mod tidyshould automatically clean them up. This will help keep the dependency graph lean and maintainable.
- `oauth` -> `auth` - add `FrontendBaseURL` to auth config
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
backend-go/internal/config/config.go (1)
48-57: Consider adding environment variable prefixTo avoid naming conflicts with other applications, consider adding a prefix to environment variables.
func bindEnvironmentVariables() error { + viper.SetEnvPrefix("CODEPAIR") for key, env := range EnvVarMap {backend-go/config.yaml (2)
22-26: Add security warning for Yorkie project secret keySimilar to other sensitive fields, add a warning comment for the project secret key.
Yorkie: APIAddr: "http://localhost:8080" ProjectName: "default" + # WARNING: Use a strong secret key in production - ProjectSecretKey: "" + ProjectSecretKey: "<YORKIE_PROJECT_SECRET_KEY>"
33-47: Add security warnings for storage credentialsAdd warning comments for MinIO and S3 credentials similar to other sensitive fields.
Storage: Provider: "minio" # or "s3" Minio: + # WARNING: Replace with secure credentials in production Bucket: "default-storage" Endpoint: "http://localhost:9000" - AccessKey: "minioadmin" - SecretKey: "minioadmin" + AccessKey: "<MINIO_ACCESS_KEY>" + SecretKey: "<MINIO_SECRET_KEY>" S3: + # WARNING: Use secure AWS credentials in production Bucket: "default-storage" Region: "us-east-1" - AccessKey: "aws_access_key" - SecretKey: "aws_secret_key" + AccessKey: "<AWS_ACCESS_KEY>" + SecretKey: "<AWS_SECRET_KEY>"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
backend-go/config.yaml(1 hunks)backend-go/internal/config/auth.go(1 hunks)backend-go/internal/config/config.go(1 hunks)backend-go/internal/config/config_test.go(1 hunks)backend-go/internal/config/env.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- backend-go/internal/config/env.go
- backend-go/internal/config/config_test.go
🔇 Additional comments (6)
backend-go/internal/config/auth.go (3)
11-14: LGTM! Well-structured Auth configurationThe Auth struct is well-designed with proper validation tags and required fields.
16-23: LGTM! Comprehensive GitHub configurationThe Github struct includes all necessary fields with appropriate validation tags.
25-31: LGTM! Robust default value handling and validationThe implementation includes proper nil checks and error wrapping.
Also applies to: 33-39, 41-51
backend-go/internal/config/config.go (3)
23-46: LGTM! Well-implemented configuration loadingThe LoadConfig function follows best practices:
- Proper error handling and wrapping
- Clear configuration hierarchy
- Comprehensive validation
59-80: LGTM! Robust config file handlingGood error handling for config file loading with proper distinction between "file not found" and other errors.
82-89: LGTM! Comprehensive validation chainThe validation implementation properly checks all configuration sections with clear error messages.
Also applies to: 91-112
| const ( | ||
| DefaultGitHubAuthorizationURL = "https://github.com/login/oauth/authorize" | ||
| DefaultGitHubUserProfileURL = "https://api.github.com/user" | ||
| DefaultGitHubCallbackURL = "https://localhost:3000/auth/login/github" |
There was a problem hiding this comment.
Avoid hardcoding localhost in default callback URL
The default callback URL should not assume localhost as it may cause issues in production environments.
Consider making this configurable or environment-aware:
-const DefaultGitHubCallbackURL = "https://localhost:3000/auth/login/github"
+const DefaultGitHubCallbackURL = "" // Force explicit configuration📝 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.
| DefaultGitHubCallbackURL = "https://localhost:3000/auth/login/github" | |
| const DefaultGitHubCallbackURL = "" // Force explicit configuration |
- set GitHub ClientID empty - set GitHub GithubSecret empty
|
Here’s what I’m considering:
|
What This PR Does / Why We Need It:
Configuration Priority (from highest to lowest, where higher-priority settings override lower ones):
config.yamlFileConfiguration Method:
config.yamlfile, and default values, enabling flexible management.config.yamlis accepted as a flag.config.yamlconfigfilepathandloglevel../config.yaml.Using expectation
config.yamlconfig.yaml.Migration consideration
codepairis using Docker Compose to obtain settings from environment variables, this approach should be compatible.Which Issue(s) This PR Fixes:
Special Notes for the Reviewer:
databaseName).mapstructure? It is used whenvipermaps values to a structure. Currently, it works withoutmapstructurebecause the Go struct names match the YAML object names. However, I have included it for explicit documentation.Does This PR Introduce a User-Facing Change?
Additional Documentation:
EnvVarMapinbackend-go/internal/config/env.gois essential for parsing environment variables.OAUTH_GITHUB_CLIENT_IDis provided,vipercannot automatically determine whetherGITHUBrepresents a structure or if it is part of a larger key (e.g.,GITHUB_CLIENT). In such cases, we explicitly map it tooauth.github.ClientIdto indicate the correct structure depth, ensuring thatvipercorrectly interprets thatGITHUBis not a nested structure.Checklist:
Summary by CodeRabbit
New Features
Tests
Chores