Add filecache to git plugin FileContent with configurable TTL#2798
Add filecache to git plugin FileContent with configurable TTL#2798
Conversation
The git plugin's FileContent method was cloning the repo on every call. This adds caching using filecache (matching the github plugin pattern), with TTL controlled by DEVBOX_X_GITHUB_PLUGIN_CACHE_TTL env var. Includes tests for cache hit, TTL expiry, and invalid TTL handling.
There was a problem hiding this comment.
Pull request overview
Adds file-based caching for gitPlugin.FileContent so git+https plugin repos aren’t cloned on every read, with cache TTL controlled by DEVBOX_X_GITHUB_PLUGIN_CACHE_TTL.
Changes:
- Add a
filecache-backed cache for git pluginFileContent, with TTL read fromDEVBOX_X_GITHUB_PLUGIN_CACHE_TTL. - Add tests covering cache hit behavior, TTL expiry behavior, and invalid TTL parsing.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| internal/plugin/git.go | Introduces filecache for FileContent and reads TTL from env var. |
| internal/plugin/git_test.go | Adds integration-style tests using a local bare git repo to validate cache/TTL behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ttl := 24 * time.Hour | ||
| var err error | ||
| ttlStr := os.Getenv("DEVBOX_X_GITHUB_PLUGIN_CACHE_TTL") | ||
| if ttlStr != "" { | ||
| ttl, err = time.ParseDuration(ttlStr) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| } | ||
| cacheKey := p.LockfileKey() + "/" + subpath + "/" + ttlStr | ||
| return gitCache.GetOrSet(cacheKey, func() ([]byte, time.Duration, error) { | ||
| content, err := p.cloneAndRead(subpath) | ||
| if err != nil { | ||
| return nil, 0, err | ||
| } | ||
| return content, ttl, nil | ||
| }) |
There was a problem hiding this comment.
cacheKey includes the raw env var value (ttlStr). If filecache maps keys onto filesystem paths (directly or partially), this allows path-separator injection (e.g. ../) or just unexpected directory structures/trailing slashes (when ttlStr == "", the key ends with /). Recommendation: don’t include the raw env var in the key; if you need “TTL changes create a new entry”, include a normalized/safe representation (e.g. use the parsed ttl and append ttl.String()), and ensure the key is constructed from sanitized path-safe segments (or hash the full key).
| if ttlStr != "" { | ||
| ttl, err = time.ParseDuration(ttlStr) | ||
| if err != nil { | ||
| return nil, err |
There was a problem hiding this comment.
The duration parse error is returned without context, which makes it harder to diagnose in user reports. Wrap it with a message that names the offending env var and value (e.g. invalid DEVBOX_X_GITHUB_PLUGIN_CACHE_TTL="...": %w) so callers get an actionable error.
| return nil, err | |
| return nil, fmt.Errorf("invalid DEVBOX_X_GITHUB_PLUGIN_CACHE_TTL=%q: %w", ttlStr, err) |
| t.Fatalf("bare clone failed: %v\n%s", err, out) | ||
| } | ||
|
|
||
| return "file://" + bareDir |
There was a problem hiding this comment.
The test builds and later parses a file:// URL using string concatenation/slicing. This is not robust across platforms (notably Windows file URLs like file:///C:/...) and can also break if URL formatting changes (extra slashes, URL-escaping, etc.). Use net/url to construct the file:// URL and to parse it back to a filesystem path (or return both the URL and the local path from setupLocalGitRepo to avoid re-parsing).
| repoPath := repoURL[len("file://"):] | ||
| if err := os.RemoveAll(repoPath); err != nil { | ||
| t.Fatalf("failed to remove repo: %v", err) | ||
| } |
There was a problem hiding this comment.
The test builds and later parses a file:// URL using string concatenation/slicing. This is not robust across platforms (notably Windows file URLs like file:///C:/...) and can also break if URL formatting changes (extra slashes, URL-escaping, etc.). Use net/url to construct the file:// URL and to parse it back to a filesystem path (or return both the URL and the local path from setupLocalGitRepo to avoid re-parsing).
| runGit := func(args ...string) { | ||
| t.Helper() | ||
| cmd := exec.Command("git", args...) |
There was a problem hiding this comment.
These tests hard-depend on an external git binary being present. To avoid opaque failures in minimal CI environments, check exec.LookPath("git") once and t.Skip(...) if it’s missing (especially since these are cache semantics tests, not specifically “git CLI availability” tests).
Summary
When we implemented the new git+https plugins feature in the last release, we did not configure it to use the DEVBOX_X_GITHUB_PLUGIN_CACHE_TTL env var to set TTL for the plugin. The git plugin's FileContent method was cloning the plugin repo on every call. This adds caching using filecache (matching the github plugin pattern), with TTL controlled by DEVBOX_X_GITHUB_PLUGIN_CACHE_TTL env var.
How was it tested?
Added 3 tests:
Cache hit (TestGitPluginFileContentCache) — Clones from a local bare repo, then deletes the repo on disk and calls FileContent again. The second call succeeds, proving it’s served from cache rather than attempting a fresh clone.
TTL expiry via env var (TestGitPluginFileContentCacheRespectsEnvVar) — Sets DEVBOX_X_GITHUB_PLUGIN_CACHE_TTL=1ns so entries expire immediately, clones once, deletes the repo, then calls FileContent again. The second call fails because the expired cache entry forces a re-clone against the now-deleted repo. This proves the env var controls cache lifetime.
Invalid TTL (TestGitPluginFileContentCacheInvalidTTL) — Sets the env var to "not-a-duration" and verifies FileContent returns a parse error before attempting any clone.
Will also test functionality via a dev release.
Is this backwards compatible?
Yes