Conversation
Introduce a machine-readable config-upgrade mode (--upgrade-config-json) and wire automatic configuration upgrades into the binary upgrade flow. The upgrade command now attempts to run the newly installed binary to merge template changes into backup.env (adding missing keys while preserving existing values). Implemented robust parsing and rendering for multi-line (block) env values, refactored value parsing into helpers (parseEnvValues, splitKeyValueRaw, renderEnvValue, findClosingQuoteLine) and updated computeConfigUpgrade to preserve block entries and multiple values per key. Added tests covering block preservation and missing-block detection. printUpgradeFooter now reports detailed config-upgrade results or errors, and installer/help text was updated to reflect the behavior change. Error handling for the JSON mode and subprocess invocation was added to ensure clear diagnostics.
There was a problem hiding this comment.
Pull request overview
This PR introduces automated configuration upgrades during binary upgrades by adding a --upgrade-config-json flag and integrating it into the upgrade workflow. The upgrade command now automatically merges template changes into backup.env by adding missing keys while preserving existing values.
Changes:
- Added
--upgrade-config-jsonflag for machine-readable config upgrade output - Implemented robust parsing and rendering for multi-line (block) environment values
- Integrated automatic config upgrade into binary upgrade flow
- Updated help text and installer messages to reflect new upgrade behavior
Reviewed changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/config/upgrade_test.go | Added tests for block value preservation and missing block key detection |
| internal/config/upgrade.go | Refactored value parsing with helpers to support block values and multiple values per key |
| internal/cli/args.go | Added UpgradeConfigJSON flag and updated upgrade description |
| cmd/proxsave/upgrade.go | Integrated config upgrade subprocess call and enhanced upgrade footer reporting |
| cmd/proxsave/main.go | Added JSON mode handler and updated incompatibility checks and help text |
| cmd/proxsave/install.go | Updated help text to reflect new upgrade behavior |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
internal/config/upgrade_test.go
Outdated
| if !strings.Contains(content, "CUSTOM_BACKUP_PATHS=\"\n/etc/custom.conf\n\"\n") && | ||
| !strings.Contains(content, "CUSTOM_BACKUP_PATHS=\"\r\n/etc/custom.conf\r\n\"\r\n") { |
There was a problem hiding this comment.
The test checks for both Unix and Windows line endings, but the logic could be clearer. Consider extracting the line ending normalization into a test helper function to avoid repeating this pattern.
internal/config/upgrade.go
Outdated
| userValues[key] = append(userValues[key], value) | ||
| userValues, userKeyOrder, err := parseEnvValues(originalLines) | ||
| if err != nil { | ||
| return result, "", originalContent, err |
There was a problem hiding this comment.
The error returned from parseEnvValues is passed through without additional context. Consider wrapping it with context about which config file failed to parse, e.g., fmt.Errorf(\"failed to parse config %s: %w\", configPath, err).
| return result, "", originalContent, err | |
| return result, "", originalContent, fmt.Errorf("failed to parse config %s: %w", configPath, err) |
internal/config/upgrade.go
Outdated
| blockLines := make([]string, 0) | ||
| blockEnd, err := findClosingQuoteLine(lines, i+1) | ||
| if err != nil { | ||
| return nil, nil, fmt.Errorf("unterminated multi-line value for %s", key) |
There was a problem hiding this comment.
The error message could be more helpful by indicating the approximate line number where the unterminated block was detected, which would help users locate the issue in large config files.
| return nil, nil, fmt.Errorf("unterminated multi-line value for %s", key) | |
| return nil, nil, fmt.Errorf("unterminated multi-line value for %s starting at line %d", key, i+1) |
cmd/proxsave/upgrade.go
Outdated
| if len(preview) > 4000 { | ||
| preview = preview[:4000] + "…" |
There was a problem hiding this comment.
The magic number 4000 for truncating preview output should be extracted as a named constant (e.g., maxPreviewLength) to make the code more maintainable and self-documenting.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Make the upgrade UX and diagnostics clearer: update the interactive prompt to inform users a backup will be created and backup.env may be updated; extract maxUpgradeConfigJSONPreviewLength constant and use it when truncating invalid JSON preview. Improve error messages to include the config path when parsing fails and the starting line number for unterminated multi-line values. Update tests to normalize CRLF vs LF so assertions are deterministic across platforms.
1 participant
Introduce a machine-readable config-upgrade mode (--upgrade-config-json) and wire automatic configuration upgrades into the binary upgrade flow. The upgrade command now attempts to run the newly installed binary to merge template changes into backup.env (adding missing keys while preserving existing values).
Implemented robust parsing and rendering for multi-line (block) env values, refactored value parsing into helpers (parseEnvValues, splitKeyValueRaw, renderEnvValue, findClosingQuoteLine) and updated computeConfigUpgrade to preserve block entries and multiple values per key. Added tests covering block preservation and missing-block detection.
printUpgradeFooter now reports detailed config-upgrade results or errors, and installer/help text was updated to reflect the behavior change. Error handling for the JSON mode and subprocess invocation was added to ensure clear diagnostics.