-
Notifications
You must be signed in to change notification settings - Fork 265
[WIP] respect XDG Base Directory Specification for config, data, and cache #1963
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,6 +7,8 @@ import ( | |
| "path/filepath" | ||
| "slices" | ||
| "strings" | ||
|
|
||
| "github.com/docker/cagent/pkg/paths" | ||
| ) | ||
|
|
||
| type History struct { | ||
|
|
@@ -17,14 +19,14 @@ type History struct { | |
| } | ||
|
|
||
| type options struct { | ||
| homeDir string | ||
| dataDir string | ||
| } | ||
|
|
||
| type Opt func(*options) | ||
|
|
||
| func WithBaseDir(dir string) Opt { | ||
| return func(o *options) { | ||
| o.homeDir = dir | ||
| o.dataDir = dir | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -34,20 +36,17 @@ func New(opts ...Opt) (*History, error) { | |
| opt(o) | ||
| } | ||
|
|
||
| homeDir := o.homeDir | ||
| if homeDir == "" { | ||
| var err error | ||
| if homeDir, err = os.UserHomeDir(); err != nil { | ||
| return nil, err | ||
| } | ||
| dataDir := o.dataDir | ||
| if dataDir == "" { | ||
| dataDir = paths.GetDataDir() | ||
| } | ||
|
|
||
| h := &History{ | ||
| path: filepath.Join(homeDir, ".cagent", "history"), | ||
| path: filepath.Join(dataDir, "history"), | ||
| current: -1, | ||
| } | ||
|
|
||
| if err := h.migrateOldHistory(homeDir); err != nil { | ||
| if err := h.migrateOldHistory(dataDir); err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
|
|
@@ -58,8 +57,8 @@ func New(opts ...Opt) (*History, error) { | |
| return h, nil | ||
| } | ||
|
|
||
| func (h *History) migrateOldHistory(homeDir string) error { | ||
| oldPath := filepath.Join(homeDir, ".cagent", "history.json") | ||
| func (h *History) migrateOldHistory(dataDir string) error { | ||
| oldPath := filepath.Join(dataDir, "history.json") | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟠 LIKELY BUG - Migration Path Issue The Problem: If the XDG directory exists but is empty (e.g., created manually or by another component), Recommendation: The migration should explicitly check the legacy directory for legacyHistoryPath := filepath.Join(legacyDir(), "history.json")
if _, err := os.Stat(legacyHistoryPath); err == nil {
// migrate from legacy location
}This ensures migration always checks the legacy location first. |
||
|
|
||
| data, err := os.ReadFile(oldPath) | ||
| if os.IsNotExist(err) { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔴 CONFIRMED BUG - Error Handling Degradation
The new code calls
paths.GetDataDir()without error handling. Ifos.UserHomeDir()fails,GetDataDir()silently falls back tofilepath.Join(os.TempDir(), ".cagent"), creating the store under/tmp/.cagent/store.Impact: On systems where
/tmpis cleared on reboot, stored artifacts will be lost, causing unexpected re-downloads. The old code explicitly returned the error, allowing callers to handle the failure appropriately.Recommendation: Either:
GetDataDir()return an error and handle it here, ORos.TempDir()and fail with a clear error messageFailing fast is better than silently using an ephemeral location for persistent data.