impv: weave config structure and migration #136
Conversation
WalkthroughThis pull request revises the configuration key naming conventions across the codebase. The "common." prefix has been removed from the gas station configuration keys in several files, and the key used for referencing the gas station mnemonic in JSON tests has been updated. Additionally, key file constants in the common module have been renamed, and the configuration management system has been enhanced with version migration functions and updated analytics keys. No public API declarations were altered. Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant Config as Config Module
App->>Config: LoadConfig()
Config->>Config: Check configuration version
alt Migration required
Config->>Config: MigrateConfigV1()
end
Config->>App: Return updated configuration
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
🔇 Additional comments (2)
✨ Finishing Touches
🪧 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: 3
🔭 Outside diff range comments (2)
models/minitia/launch.go (2)
2899-2923: Refactor log streaming logic into a reusable function.The log streaming logic is duplicated for stdout and stderr. Consider extracting it into a reusable function to reduce code duplication.
+func streamLogs(reader io.Reader, streamingLogs *[]string) { + scanner := bufio.NewScanner(reader) + for scanner.Scan() { + line := scanner.Text() + if !isJSONLog(line) { + *streamingLogs = append(*streamingLogs, line) + if len(*streamingLogs) > 10 { + *streamingLogs = (*streamingLogs)[1:] + } + } + } +} - go func() { - scanner := bufio.NewScanner(stdout) - for scanner.Scan() { - line := scanner.Text() - if !isJSONLog(line) { - *streamingLogs = append(*streamingLogs, line) - if len(*streamingLogs) > 10 { - *streamingLogs = (*streamingLogs)[1:] - } - } - } - }() - - go func() { - scanner := bufio.NewScanner(stderr) - for scanner.Scan() { - line := scanner.Text() - if !isJSONLog(line) { - *streamingLogs = append(*streamingLogs, line) - if len(*streamingLogs) > 10 { - *streamingLogs = (*streamingLogs)[1:] - } - } - } - }() + go streamLogs(stdout, streamingLogs) + go streamLogs(stderr, streamingLogs)
2787-2788: Add mutex for thread-safe log handling.The
streamingLogsslice is accessed from multiple goroutines without synchronization. This could lead to race conditions.type LaunchingNewMinitiaLoading struct { ui.Loading weavecontext.BaseModel + logsMutex sync.Mutex streamingLogs *[]string }And update the log handling:
+func (m *LaunchingNewMinitiaLoading) appendLog(line string) { + m.logsMutex.Lock() + defer m.logsMutex.Unlock() + *m.streamingLogs = append(*m.streamingLogs, line) + if len(*m.streamingLogs) > 10 { + *m.streamingLogs = (*m.streamingLogs)[1:] + } +}
🧹 Nitpick comments (5)
config/config.go (2)
124-125: Avoid silently discarding the SetConfig error.
Capturing and handling the return value fromSetConfig("analytics.opt_out", false)can help detect file I/O or permission issues.
133-135: Consider error handling for SetConfig.
Ignoring the error returned fromSetConfig("analytics.device_id", deviceID)could hide potential failures in saving the device ID.models/minitia/launch.go (3)
1290-1297: Improve error handling consistency.The error handling pattern here could be improved. The first error is handled with
m.HandlePanic, but the second error is returned directly. Consider using consistent error handling patterns throughout the code.- gasStationKey, err := config.RecoverGasStationKey(input.Text) - if err != nil { - return m, m.HandlePanic(err) - } - err = config.SetConfig("gas_station", gasStationKey) - if err != nil { - return m, m.HandlePanic(err) - } + if gasStationKey, err := config.RecoverGasStationKey(input.Text); err != nil { + return m, m.HandlePanic(fmt.Errorf("failed to recover gas station key: %w", err)) + } else if err = config.SetConfig("gas_station", gasStationKey); err != nil { + return m, m.HandlePanic(fmt.Errorf("failed to set gas station config: %w", err)) + }
2874-2878: Ensure secure file permissions for sensitive data.The code sets file permissions to 0600 for the config file, which is good. However, consider adding a check to verify that the parent directory also has secure permissions to prevent unauthorized access.
configFilePath = filepath.Join(userHome, common.WeaveDataDirectory, LaunchConfigFilename) + // Ensure parent directory has secure permissions + if err = os.Chmod(filepath.Dir(configFilePath), 0700); err != nil { + return ui.NonRetryableErrorLoading{Err: fmt.Errorf("failed to set directory permissions: %v", err)} + } if err = os.WriteFile(configFilePath, configBz, 0600); err != nil { return ui.NonRetryableErrorLoading{Err: fmt.Errorf("failed to write config file: %v", err)} }
2885-2893: Extract command execution logic to a separate function.The command execution setup logic is mixed with the business logic. Consider extracting it to a separate function for better maintainability and reusability.
+func setupCommandWithPipes(cmd *exec.Cmd) (stdout, stderr io.ReadCloser, err error) { + if stdout, err = cmd.StdoutPipe(); err != nil { + return nil, nil, fmt.Errorf("failed to capture stdout: %v", err) + } + if stderr, err = cmd.StderrPipe(); err != nil { + return nil, nil, fmt.Errorf("failed to capture stderr: %v", err) + } + return stdout, stderr, nil +} launchCmd := exec.Command(state.binaryPath, "launch", "--with-config", configFilePath, "--home", minitiaHome) - stdout, err := launchCmd.StdoutPipe() - if err != nil { - return ui.NonRetryableErrorLoading{Err: fmt.Errorf("failed to capture stdout: %v", err)} - } - stderr, err := launchCmd.StderrPipe() - if err != nil { - return ui.NonRetryableErrorLoading{Err: fmt.Errorf("failed to capture stderr: %v", err)} - } + stdout, stderr, err := setupCommandWithPipes(launchCmd) + if err != nil { + return ui.NonRetryableErrorLoading{Err: err} + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
cmd/gas_station_integration_test.go(1 hunks)common/constants.go(1 hunks)config/config.go(5 hunks)models/initialize.go(1 hunks)models/minitia/launch.go(1 hunks)models/minitia/launch_test.go(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- common/constants.go
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Run Integration Tests on Ubuntu
🔇 Additional comments (11)
config/config.go (7)
13-13: No concerns with the new import.
No issues spotted; the package might be necessary for the recently introduced I/O operations.
71-74: Well-structured call to the new migration function.
InvokingMigrateConfigV1immediately after reading the config clearly separates reading from migration.
95-95: Possible edge case in first-time setup check.
Relying solely on"gas_station"beingnilmay misclassify certain scenarios (e.g., if the user or a script clears the key).
103-103: Ensure JSON-serializable data.
Callingjson.Marshalondataretrieved viaGetConfig("gas_station")could fail if the value is stored in a non-serializable format.
143-143: Good practice returning SetConfig error.
Unlike the earlier occurrences, here the function returns the error, ensuring proper error handling.
146-149: Clear default configuration template.
Providing a version field and initializinganalyticsis beneficial for forward compatibility.
174-219: 🛠️ Refactor suggestionRobust migration function with minor type safety concern.
While this approach correctly preserves and restructures config data, a direct float64 type assertion may cause panics if a corrupted or non-numeric version is stored. Also, be mindful of potential concurrency issues if more than one process can trigger migrations.cmd/gas_station_integration_test.go (1)
41-41: Test aligned with the updated key.
Switching the key to"gas_station.mnemonic"maintains consistency with the newly migrated config structure.models/initialize.go (1)
325-325: Key rename to “gas_station” aligns with the migration.
Ensuring all references to the old"common.gas_station"key are replaced helps maintain consistency.models/minitia/launch_test.go (1)
1038-1038: LGTM! Configuration key change is consistent.The removal of the "common." prefix from the gas station configuration key aligns with the broader changes in the codebase.
models/minitia/launch.go (1)
1294-1294: Configuration key has been simplified.The configuration key has been changed from "common.gas_station" to "gas_station", aligning with the changes mentioned in the AI summary. This change appears to be part of a broader effort to simplify configuration key naming conventions.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
config/config.go (1)
129-129:⚠️ Potential issueAdd type assertion safety checks.
The code contains unsafe type assertions that could panic if the config values are of unexpected types.
Apply these changes to add safety checks:
- return GetConfig("analytics.opt_out").(bool) + if val, ok := GetConfig("analytics.opt_out").(bool); ok { + return val + } + return false // Default to false if type assertion fails - return GetConfig("analytics.device_id").(string) + if val, ok := GetConfig("analytics.device_id").(string); ok { + return val + } + deviceID := uuid.New().String() + _ = SetConfig("analytics.device_id", deviceID) + return deviceIDAlso applies to: 139-139
🧹 Nitpick comments (3)
config/config.go (3)
146-149: Consider using a struct for DefaultConfigTemplate.Instead of hardcoding the JSON template as a string, consider defining it as a struct and marshaling it to JSON. This would:
- Make the structure type-safe
- Allow compile-time validation
- Make it easier to maintain and modify
Here's a suggested implementation:
-const DefaultConfigTemplate = `{ - "version": 1, - "analytics": {} -}` +type Config struct { + Version int `json:"version"` + Analytics map[string]interface{} `json:"analytics"` +} + +var DefaultConfigTemplate = func() string { + config := Config{ + Version: 1, + Analytics: make(map[string]interface{}), + } + bytes, _ := json.MarshalIndent(config, "", " ") + return string(bytes) +}()
66-76: Consider atomic config migration.If migration fails, the config might be left in an inconsistent state. Consider:
- Backing up the config before migration
- Restoring the backup if migration fails
Here's a suggested implementation:
func LoadConfig() error { if err := viper.ReadInConfig(); err != nil { return fmt.Errorf("failed to read config file: %v", err) } + // Backup current config + backup := viper.AllSettings() + if err := MigrateConfigV1(); err != nil { + // Restore backup on failure + for k, v := range backup { + viper.Set(k, v) + } return err } return nil }
183-219: Enhance migration robustness and observability.Consider adding:
- Validation of migrated data
- Logging of migration progress
- Backup of old config before migration
Here's a suggested implementation:
func MigrateConfigV1() error { version := GetConfigVersion() if version == 1 { return nil // Already at latest version } // Migrate from version 0 to 1 if version == 0 { + fmt.Printf("Migrating config from version 0 to 1...\n") + // Preserve existing data gasStation := GetConfig("common.gas_station") analyticsOptOut := GetConfig("common.analytics_opt_out") analyticsDeviceID := GetConfig("common.analytics_device_id") + // Validate data before migration + if gasStation != nil { + if _, ok := gasStation.(map[string]interface{}); !ok { + return fmt.Errorf("invalid gas_station data format") + } + } + // Clear the config viper.Set("version", 1) viper.Set("gas_station", map[string]interface{}{}) viper.Set("analytics", map[string]interface{}{}) viper.Set("common", nil) // Restore the data if gasStation != nil { viper.Set("gas_station", gasStation) } if analyticsOptOut != nil { viper.Set("analytics.opt_out", analyticsOptOut) } if analyticsDeviceID != nil { viper.Set("analytics.device_id", analyticsDeviceID) } if err := WriteConfig(); err != nil { return fmt.Errorf("failed to migrate config to version 1: %v", err) } + + fmt.Printf("Successfully migrated config to version 1\n") } return nil }
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
config/config.go (2)
103-113: Simplify JSON conversion.The double JSON conversion (Marshal then Unmarshal) is inefficient. Consider using direct type assertion with a safety check.
- data := GetConfig("gas_station") - jsonData, err := json.Marshal(data) - if err != nil { - return nil, fmt.Errorf("failed to marshal json: %v", err) - } - - var gasKey GasStationKey - err = json.Unmarshal(jsonData, &gasKey) - if err != nil { - return nil, fmt.Errorf("failed to unmarshal json: %v", err) - } + data := GetConfig("gas_station") + if data == nil { + return nil, fmt.Errorf("gas station key not found") + } + + if mapData, ok := data.(map[string]interface{}); ok { + gasKey := GasStationKey{ + InitiaAddress: fmt.Sprint(mapData["initia_address"]), + CelestiaAddress: fmt.Sprint(mapData["celestia_address"]), + Mnemonic: fmt.Sprint(mapData["mnemonic"]), + } + return &gasKey, nil + } + return nil, fmt.Errorf("invalid gas station key format")
183-219: Consider atomic updates for config migration.While the migration logic is correct, a failure during migration could leave the config in an inconsistent state. Consider implementing a transaction-like behavior.
func MigrateConfigV1() error { version := GetConfigVersion() if version == 1 { return nil // Already at latest version } // Migrate from version 0 to 1 if version == 0 { + // Create a copy of the current config + tempConfig := make(map[string]interface{}) + for k, v := range viper.AllSettings() { + tempConfig[k] = v + } + // Preserve existing data gasStation := GetConfig("common.gas_station") analyticsOptOut := GetConfig("common.analytics_opt_out") analyticsDeviceID := GetConfig("common.analytics_device_id") // Clear the config viper.Set("version", 1) viper.Set("gas_station", map[string]interface{}{}) viper.Set("analytics", map[string]interface{}{}) viper.Set("common", nil) // Restore the data if gasStation != nil { viper.Set("gas_station", gasStation) } if analyticsOptOut != nil { viper.Set("analytics.opt_out", analyticsOptOut) } if analyticsDeviceID != nil { viper.Set("analytics.device_id", analyticsDeviceID) } if err := WriteConfig(); err != nil { + // Restore the original config on failure + for k, v := range tempConfig { + viper.Set(k, v) + } + _ = WriteConfig() // Best effort to restore return fmt.Errorf("failed to migrate config to version 1: %v", err) } } return nil }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
config/config.go(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Run Integration Tests on Ubuntu
🔇 Additional comments (4)
config/config.go (4)
70-73: LGTM! Migration is correctly placed.The migration is appropriately called after loading the config and includes proper error handling.
129-129: Guard against unsafe type assertion.Casting
GetConfig("analytics.opt_out")toboolwithout a check may lead to a panic if the config is malformed.
139-139: Unsafe string assertion.Casting
GetConfig("analytics.device_id")tostringcould panic if someone modifies the config to an unexpected type.
146-149: LGTM! Template structure is well-defined.The template correctly includes version and analytics fields, aligning with the new configuration structure.
Benzbeeb
dismissed stale reviews from traviolus and coderabbitai[bot]
via
233a90d
Description
Closes: #XXXX
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow-up issues.
I have...
!in the type prefix if API or client breaking changeReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
I have...
Summary by CodeRabbit