fix(config): migrate legacy config.yaml after config dir move#596
Closed
skevetter wants to merge 2 commits into
Closed
fix(config): migrate legacy config.yaml after config dir move#596skevetter wants to merge 2 commits into
skevetter wants to merge 2 commits into
Conversation
PR #580 moved unix ConfigDir from ~/.devsy to ~/.config/devsy without a migration. config.yaml holds each provider's Initialized flag, so after upgrade devsy read an empty config from the new dir and every workspace failed with "provider is not initialized". ConfigFilePath now relocates a legacy DataDir/config.yaml into the new ConfigDir when the new location is empty. No-op on Windows and for fresh installs.
✅ Deploy Preview for devsydev canceled.
|
📝 WalkthroughWalkthroughAdds automatic migration of a legacy config file to a new config path within ChangesLegacy config migration
Estimated code review effort: 2 (Simple) | ~15 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
✅ Deploy Preview for images-devsy-sh canceled.
|
Move migrateLegacyConfigFile after exported methods (funcorder), annotate test reads with #nosec G304, and drop section-divider comments.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
pkg/config/pathmanager_darwin_test.go (1)
99-135: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider also asserting the legacy file's fate in the no-clobber test.
The test verifies the current config isn't overwritten, but doesn't assert whether
legacyPathis left in place afterward (sincemigrateLegacyConfigFileshort-circuits without touching it). Adding that assertion would fully pin down the no-clobber contract.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/config/pathmanager_darwin_test.go` around lines 99 - 135, Update TestDarwinConfigFilePath_KeepsExistingConfig to also assert the legacy config at legacyPath remains present after pm.ConfigFilePath() is called. Use the existing ConfigFilePath and migrateLegacyConfigFile behavior as the reference point, and verify both that the current file content stays unchanged and the legacy file is not removed or modified.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/config/pathmanager.go`:
- Around line 365-378: The migrateLegacyConfigFile logic in basePathManager is
eagerly calling DataDir() just to look for an old config, which creates the
legacy directory on fresh installs. Update migrateLegacyConfigFile to avoid that
side effect by deriving the legacy config path without invoking DataDir() unless
a migration is actually needed, or by otherwise deferring the DataDir() call
until you have confirmed a legacy file exists.
- Around line 366-368: In the config migration logic around os.Stat(configPath),
make the os.Rename call tolerate a concurrent migration race: if the rename
fails with ENOENT, treat it as already migrated and continue instead of
returning an error. Update the migration path to explicitly check for that error
case, alongside the existing os.IsNotExist handling, so startup does not fail
when another process moves the legacy file first.
---
Nitpick comments:
In `@pkg/config/pathmanager_darwin_test.go`:
- Around line 99-135: Update TestDarwinConfigFilePath_KeepsExistingConfig to
also assert the legacy config at legacyPath remains present after
pm.ConfigFilePath() is called. Use the existing ConfigFilePath and
migrateLegacyConfigFile behavior as the reference point, and verify both that
the current file content stays unchanged and the legacy file is not removed or
modified.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 27980dd8-7e66-4c7a-bead-1005f87c40d2
📒 Files selected for processing (3)
pkg/config/pathmanager.gopkg/config/pathmanager_darwin_test.gopkg/config/pathmanager_linux_test.go
Comment on lines
+365
to
+378
| func (b *basePathManager) migrateLegacyConfigFile(configPath string) error { | ||
| if _, err := os.Stat(configPath); err == nil || !os.IsNotExist(err) { | ||
| return err | ||
| } | ||
|
|
||
| dataDir, err := b.pm.DataDir() | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| legacyPath := filepath.Join(dataDir, ConfigFile) | ||
| if legacyPath == configPath { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm whether ensureDir creates the directory as a side effect.
rg -n -A6 'func ensureDir' pkg/configRepository: devsy-org/devsy
Length of output: 704
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant path manager implementations and migration flow.
sed -n '1,120p' pkg/config/pathmanager.go
printf '\n--- darwin ---\n'
sed -n '1,120p' pkg/config/pathmanager_darwin.go
printf '\n--- linux ---\n'
sed -n '1,120p' pkg/config/pathmanager_linux.go
printf '\n--- call sites ---\n'
rg -n 'migrateLegacyConfigFile|ConfigFilePath\(|DataDir\(' pkg/configRepository: devsy-org/devsy
Length of output: 8540
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read only the migration function and adjacent code.
sed -n '340,410p' pkg/config/pathmanager.goRepository: devsy-org/devsy
Length of output: 1616
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the migration function and adjacent logic.
sed -n '340,410p' pkg/config/pathmanager.go
printf '\n--- call sites ---\n'
rg -n 'migrateLegacyConfigFile|ConfigFilePath\(|DataDir\(' pkg/configRepository: devsy-org/devsy
Length of output: 3556
Avoid creating the legacy data dir during config migration
migrateLegacyConfigFile calls DataDir() whenever the config file is missing, and the platform implementations create that directory with ensureDir/os.MkdirAll. On a fresh install, ConfigFilePath() still recreates ~/.devsy just to check for an old config file. Compute the legacy path without the side effect, or defer the DataDir() call until migration is actually needed.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/config/pathmanager.go` around lines 365 - 378, The
migrateLegacyConfigFile logic in basePathManager is eagerly calling DataDir()
just to look for an old config, which creates the legacy directory on fresh
installs. Update migrateLegacyConfigFile to avoid that side effect by deriving
the legacy config path without invoking DataDir() unless a migration is actually
needed, or by otherwise deferring the DataDir() call until you have confirmed a
legacy file exists.
Comment on lines
+366
to
+368
| if _, err := os.Stat(configPath); err == nil || !os.IsNotExist(err) { | ||
| return err | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the file and inspect the relevant section with line numbers.
git ls-files pkg/config/pathmanager.go
wc -l pkg/config/pathmanager.go
sed -n '330,410p' pkg/config/pathmanager.go
# Find related uses of legacy/config paths and migration logic.
rg -n "legacyPath|configPath|os\.Rename|os\.Stat|ConfigFilePath|DataDir|ConfigDir" pkg/config/pathmanager.goRepository: devsy-org/devsy
Length of output: 2748
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the path manager interface and platform implementations.
sed -n '1,220p' pkg/config/pathmanager.go
# Find concrete implementations of ConfigDir/DataDir.
rg -n "func .*ConfigDir|func .*DataDir|ConfigDir\(\)|DataDir\(\)" pkg/config -g '*.go'
# Inspect any platform-specific path manager files.
fd -a 'pathmanager*.go' pkg/configRepository: devsy-org/devsy
Length of output: 8225
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect platform-specific ConfigDir/DataDir implementations.
sed -n '1,120p' pkg/config/pathmanager_linux.go
sed -n '1,120p' pkg/config/pathmanager_darwin.go
sed -n '1,120p' pkg/config/pathmanager_windows.go
# Inspect any tests that pin the directory layout.
sed -n '1,180p' pkg/config/pathmanager_linux_test.go
sed -n '1,180p' pkg/config/pathmanager_darwin_test.goRepository: devsy-org/devsy
Length of output: 11532
Handle the concurrent migration race
os.Rename can still return ENOENT if another process migrates the legacy file first; treat that as already migrated instead of failing startup.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/config/pathmanager.go` around lines 366 - 368, In the config migration
logic around os.Stat(configPath), make the os.Rename call tolerate a concurrent
migration race: if the rename fails with ENOENT, treat it as already migrated
and continue instead of returning an error. Update the migration path to
explicitly check for that error case, alongside the existing os.IsNotExist
handling, so startup does not fail when another process moves the legacy file
first.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
v1.4.0-beta.1 broke every existing workspace with
provider "docker" is not initialized.Root cause: PR #580 moved the unix config directory from
~/.devsyto~/.config/devsywithout a migration. Each provider'sInitializedflag lives inconfig.yaml(Contexts[ctx].Providers[...].Initialized), which is read fromConfigDir. After upgrade, devsy read an empty config from the new (empty) directory —LoadConfigsilently returns an empty config when the file is absent — soprovider.State == niland the check atpkg/workspace/workspace.gofailed for all providers.Fix
ConfigFilePath()now transparently relocates a legacyDataDir/config.yaml(~/.devsy/config.yaml) into the newConfigDirwhen the new location is empty. The operation is idempotent and self-limiting:ConfigDiralways differed fromDataDir) → no-op; never clobbers an existing config.Added migration and no-clobber tests for darwin and linux path managers.
Summary by CodeRabbit