Conversation
tis24dev
commented
Feb 3, 2026
tis24dev
commented
- Improve env config parsing and upgrade logic
- Report config upgrade warnings
Refactor config parsing and upgrade flow to better handle comments, casing and multiline values. - setEnvValue: skip commented lines, preserve indentation and inline comments when replacing values. - parseEnvFile/parseEnvValues: ignore comment-only lines inside multi-line block values; return comment text and a caseMap (UPPER->original) for case-insensitive matching; add comment storage to envValue. - splitKeyValueRaw: return parsed inline comment, handle legacy "export " prefixes, and more robustly extract quoted/unquoted values and trailing comments. - computeConfigUpgrade: use case-insensitive lookups via caseMap, track processed user keys, preserve user key casing for extras, count preserved values correctly, and detect no-op upgrades by comparing newContent with originalContent. - renderEnvValue: include preserved inline comments when rendering single-line values. - Added import of utils.IsComment where needed. Also updated internal/orchestrator/.backup.lock (pid/time) as part of the changeset.
Add non-fatal warnings support to the config upgrade flow. Introduce a Warnings []string field on UpgradeResult and propagate warning messages from parseEnvValues through computeConfigUpgrade. Detect and warn about duplicate template keys that differ only by case, case-collision between user keys and template keys, and ignored non-KEY=VALUE lines; preserve behavior for multi-line values. Surface these warnings in the CLI (cmd/proxsave/main.go and cmd/proxsave/upgrade.go) so they are printed during plan/upgrade operations. Update parseEnvValues signature to return case conflict info and warnings, and add tests verifying case-collision and ignored-line warnings.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Pull request overview
This PR syncs changes from the dev branch to main, focusing on improving environment configuration parsing and upgrade logic with enhanced warning reporting.
Changes:
- Enhanced configuration upgrade logic with case-insensitive key matching and warning system
- Added support for parsing and preserving inline comments in configuration files
- Improved handling of legacy "export KEY=VALUE" format
- Added comprehensive warning reporting for configuration issues (case collisions, ignored lines, etc.)
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/orchestrator/.backup.lock | Updated runtime lock file with new PID and timestamp (should not be committed) |
| internal/config/upgrade_test.go | Added tests for case collision and ignored line warnings |
| internal/config/upgrade.go | Major refactoring: added warning system, case-insensitive key matching, comment preservation, and export handling |
| internal/config/config.go | Added filtering of comments within block values during parsing |
| cmd/proxsave/upgrade.go | Added display of configuration warnings in upgrade footer |
| cmd/proxsave/main.go | Added warning display for both dry-run and actual config upgrades |
| cmd/proxsave/config_helpers.go | Refactored setEnvValue to better preserve formatting and comments |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
|
|
||
| // Handle block values | ||
| if blockValueKeys[key] && trimmed == fmt.Sprintf("%s=\"", key) { |
There was a problem hiding this comment.
The blockValueKeys check uses the template's key casing directly. Since blockValueKeys is defined with uppercase keys like "CUSTOM_BACKUP_PATHS" and "BACKUP_BLACKLIST", if a template uses different casing (e.g., "Custom_Backup_Paths"), this check will fail. Although templates are typically controlled and use consistent casing, this inconsistency with line 357 suggests the check should use uppercase key comparison for consistency.
| if blockValueKeys[key] && trimmed == fmt.Sprintf("%s=\"", key) { | |
| if blockValueKeys[upperKey] && trimmed == fmt.Sprintf("%s=\"", key) { |
| pid=99930 | ||
| host=pve | ||
| time=2026-02-03T00:07:31+01:00 | ||
| time=2026-02-03T19:42:03+01:00 |
There was a problem hiding this comment.
This appears to be a runtime lock file containing process-specific information (PID, hostname, timestamp). Lock files should not be committed to version control as they are generated during runtime and will differ between machines and executions. This file should be added to .gitignore instead.
| if idx := strings.Index(line, "#"); idx >= 0 { | ||
| // Verify # is not part of the key or value? | ||
| // Assuming standard comment | ||
| commentPart := line[idx:] | ||
| // Ensure it's not inside quotes? The original logic didn't check quotes carefully but let's be safe(r). | ||
| // For setEnvValue we are replacing the value, so we just want to keep the comment at the end. | ||
| comment = commentPart | ||
|
|
||
| // Find spacing before comment | ||
| beforeComment := line[:idx] | ||
| trimmedBefore := strings.TrimRight(beforeComment, " \t") | ||
| commentSpacing = beforeComment[len(trimmedBefore):] | ||
| } |
There was a problem hiding this comment.
The comment extraction logic doesn't properly handle quoted values containing "#". For example, if a line is KEY="value#with#hash", the code will incorrectly treat #with#hash" as a comment. The logic should parse the value part first (respecting quotes) before looking for comments, similar to how splitKeyValueRaw handles it in upgrade.go.
|
|
||
| caseMap[upperKey] = key | ||
|
|
||
| if blockValueKeys[key] && trimmed == fmt.Sprintf("%s=\"", key) { |
There was a problem hiding this comment.
The blockValueKeys check uses the user's key casing, but blockValueKeys is defined with uppercase keys like "CUSTOM_BACKUP_PATHS" and "BACKUP_BLACKLIST". If a user has "custom_backup_paths=" with different casing, this check will fail to detect it as a block value. Consider checking blockValueKeys using the uppercase version of the key to handle case-insensitive matching consistently.
| if blockValueKeys[key] && trimmed == fmt.Sprintf("%s=\"", key) { | |
| if blockValueKeys[upperKey] && trimmed == fmt.Sprintf("%s=\"", key) { |
1 participant
