Add remote config support (lets -c https://...)#357
Open
kindermax wants to merge 13 commits into
Open
Conversation
Reviewer's GuideAdds support for loading lets configs from remote HTTP(S) URLs with caching, centralizes HTTP/YAML download logic in a new fetch package, and wires a new --no-cache flag through the CLI and config loader to control re-download behavior. Sequence diagram for loading remote config with caching and --no-cachesequenceDiagram
actor User
participant CLI as cli.Main
participant Loader as config.LoadRemote
participant CacheFn as ensureRemoteConfig
participant Fetch as fetch.Download
User->>CLI: lets -c https://url [--no-cache]
CLI->>Loader: LoadRemote(url, noCache, version)
Loader->>CacheFn: ensureRemoteConfig(url, noCache)
alt noCache is false and cache exists
CacheFn-->>Loader: return cachedPath
else cache missing or noCache is true
CacheFn->>Fetch: Download(context.Background, url)
alt Download succeeds
Fetch-->>CacheFn: data
CacheFn->>CacheFn: os.WriteFile(cachePath, data, 0o644)
CacheFn-->>Loader: return cachePath
else Download fails and cache exists
CacheFn-->>Loader: return cachePath
else Download fails and no cache
CacheFn-->>Loader: error
Loader-->>CLI: error
CLI-->>User: config error
end
end
Loader->>Loader: yaml.NewDecoder.Decode
Loader->>Loader: validate
Loader->>Loader: Config.SetupEnv
Loader-->>CLI: *config.Config
CLI-->>User: runs requested command from remote config
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- Consider threading a context from the CLI into
LoadRemote/ensureRemoteConfigand down tofetch.Downloadinstead of always usingcontext.Background(), so long-running or hung remote fetches can be cancelled cleanly on Ctrl-C. - In
fetch.Download, responses with no or malformedContent-Typeheaders are currently rejected; if you expect configs to be served by simpler static hosts, it might be worth treating an empty/unknownContent-Typeas acceptable YAML instead of hard-failing.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider threading a context from the CLI into `LoadRemote`/`ensureRemoteConfig` and down to `fetch.Download` instead of always using `context.Background()`, so long-running or hung remote fetches can be cancelled cleanly on Ctrl-C.
- In `fetch.Download`, responses with no or malformed `Content-Type` headers are currently rejected; if you expect configs to be served by simpler static hosts, it might be worth treating an empty/unknown `Content-Type` as acceptable YAML instead of hard-failing.
## Individual Comments
### Comment 1
<location path="internal/fetch/fetch.go" line_range="41-42" />
<code_context>
-
- if resp.StatusCode == http.StatusNotFound {
- return nil, fmt.Errorf("no such file at: %s", rm.URL)
- } else if resp.StatusCode < 200 || resp.StatusCode > 299 {
- return nil, fmt.Errorf("network error: %s", resp.Status)
- }
-
</code_context>
<issue_to_address>
**suggestion:** Include the URL in non-2xx HTTP error messages for better diagnosability
Right now `network error: %s` only includes the status text, which makes it hard to tell which request failed when multiple remote configs/mixins are fetched. Please include the URL (or at least host/path) in this error to make logs and user-facing messages easier to interpret.
```suggestion
if resp.StatusCode == http.StatusNotFound {
return nil, fmt.Errorf("no such file at: %s", url)
} else if resp.StatusCode < 200 || resp.StatusCode > 299 {
return nil, fmt.Errorf("network error for %s: %s", url, resp.Status)
}
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+41
to
+42
| if resp.StatusCode == http.StatusNotFound { | ||
| return nil, fmt.Errorf("no such file at: %s", url) |
There was a problem hiding this comment.
suggestion: Include the URL in non-2xx HTTP error messages for better diagnosability
Right now network error: %s only includes the status text, which makes it hard to tell which request failed when multiple remote configs/mixins are fetched. Please include the URL (or at least host/path) in this error to make logs and user-facing messages easier to interpret.
Suggested change
| if resp.StatusCode == http.StatusNotFound { | |
| return nil, fmt.Errorf("no such file at: %s", url) | |
| if resp.StatusCode == http.StatusNotFound { | |
| return nil, fmt.Errorf("no such file at: %s", url) | |
| } else if resp.StatusCode < 200 || resp.StatusCode > 299 { | |
| return nil, fmt.Errorf("network error for %s: %s", url, resp.Status) | |
| } |
Remove inline HTTP download logic (allowedContentTypes var, normalizeContentType func, and the old download() body) in favour of delegating to fetch.Download. Delete the now-redundant unit tests for those helpers; equivalent coverage lives in internal/fetch/fetch_test.go.
Downloads a remote config by URL, caches it under ~/.config/lets/remote-configs/<sha256(url)>.yaml, and sets the working directory to the caller's CWD (not the cache dir). Falls back to cache when a --no-cache refresh fails; errors when there is no cache and the download fails.
- Restore 5-minute httpClient timeout in fetch (was regressed to 30s) - Thread cancellable ctx through LoadRemote -> fetch.Download - Fix index OOB panic when --config has no following value - Add RemoteSource field to Config; set LETS_CONFIG=url, LETS_CONFIG_DIR=CWD for remote configs - Warn when LETS_CONFIG_DIR env var is set alongside a remote URL - Atomic cache write via temp-file + rename to prevent partial files - Improve corrupt-cache error to suggest --no-cache - Extract shared loadConfigFromFile helper to avoid Load/LoadRemote duplication
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
lets -c https://urlsupport: downloads and caches remotelets.yamlto~/.config/lets/remote-configs/<sha256>.yaml--no-cacheflag to force re-download; falls back to cached version if download fails with a warninginternal/fetchpackage, reused by remote mixins and the newLoadRemotepathwork_dir)LETS_CONFIGis set to the original URL;LETS_CONFIG_DIRis set to CWD (not the cache directory)Limitations
mixins— only standalone configs (nomixins:key) are supported for nowImplementation notes
sha256(url)— stable and collision-resistant; use--no-cacheto pick up URL updatesctxfrom SIGINT/SIGTERM is propagated throughLoadRemoteso Ctrl+C cancels in-progress downloadsLETS_CONFIG_DIRenv var is ignored (with a warning) when using a remote URLTest Plan
go test ./...passes (all packages green)lets -c https://<raw-yaml-url> <command>downloads config on first run--no-cacheuses cached file (no HTTP request)lets -c https://<url> --no-cache <command>re-downloadslets --helpshows--no-cacheflag$LETS_CONFIGin a remote command equals the original URL$LETS_CONFIG_DIRin a remote command equals the invocation CWD