Repository navigation
feat: implement remote stack imports - #2037
Merged
Merged
Conversation
Erik Osterman (Cloud Posse) (osterman)
requested review from
a team
as code owners
January 29, 2026 15:07
Erik Osterman (Cloud Posse) (osterman)
added
the
minor
Contributor
|
Warning This PR exceeds the recommended limit of 1,000 lines.Large PRs are difficult to review and may be rejected due to their size. Please verify that this PR does not address multiple issues. |
Dependency Review✅ No vulnerabilities or license issues found.Scanned FilesNone |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2037 +/- ##
==========================================
+ Coverage 78.39% 78.54% +0.14%
==========================================
Files 1133 1141 +8
Lines 107784 109581 +1797
==========================================
+ Hits 84494 86065 +1571
- Misses 18585 18735 +150
- Partials 4705 4781 +76
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Contributor
📝 WalkthroughWalkthroughAdds remote stack import support: URI classification helpers, a RemoteImporter with in-memory + on-disk caching, atomic FileCache and FileLock (Unix flock / Windows noop), import-path resolution utilities, stack-processor integration for remote downloads and provenance, examples, docs, and extensive tests. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as Atmos CLI
participant Processor as Stack Processor
participant Importer as Remote Importer
participant Cache as File Cache
participant Remote as Remote Server
CLI->>Processor: Request stack processing
Processor->>Importer: Classify import URI (IsRemote / IsLocal)
alt Remote import
Importer->>Cache: Check in-memory session cache
alt In-memory hit
Cache-->>Importer: Return cached local path
else
Importer->>Cache: GetOrFetch(key)
alt Disk hit
Cache-->>Importer: Return cached local path
else
Importer->>Remote: Download URI
Remote-->>Importer: Return content
Importer->>Cache: Atomically store content (with FileLock)
Cache-->>Importer: Return cached local path
end
end
Importer-->>Processor: Provide local cached path
else Local import
Processor-->>Processor: Resolve local path (join basePath, glob/extension resolution)
end
Processor->>Processor: Parse and deep-merge imports in original order (propagate context, provenance)
Processor-->>CLI: Return merged stack configuration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/exec/stack_processor_utils.go (1)
832-1006: Preserve original remote URI in import metadata.Remote downloads are keyed by the local cache path, so
importsoutput and provenance end up showing temp hashed paths instead of the original URL. Carry the original URI through and use it as the import key.✅ Suggested change
@@ - var importMatches []string + var ( + importMatches []string + importKey string + isRemoteImport bool + ) @@ - if stackimports.IsRemote(imp) { + if stackimports.IsRemote(imp) { + isRemoteImport = true // Download the remote import. log.Debug("Downloading remote stack import", "uri", imp, "file", relativeFilePath) localPath, err := stackimports.DownloadRemoteImport(atmosConfig, imp) @@ // Remote imports return a single file path. importMatches = []string{localPath} + importKey = imp } else { @@ - for i, importFile := range importMatches { + for i, importFile := range importMatches { + key := importKey + remote := isRemoteImport wg.Add(1) - go func(index int, file string) { + go func(index int, file string, remote bool, key string) { defer wg.Done() @@ importRelativePathWithExt := strings.Replace(filepath.ToSlash(file), filepath.ToSlash(basePath)+"/", "", 1) ext2 := filepath.Ext(importRelativePathWithExt) if ext2 == "" { ext2 = u.DefaultStackConfigFileExtension } importRelativePathWithoutExt := strings.TrimSuffix(importRelativePathWithExt, ext2) + if remote && key != "" { + importRelativePathWithoutExt = key + } @@ - }(i, importFile) + }(i, importFile, remote, key) }
🤖 Fix all issues with AI agents
In `@pkg/stack/imports/remote_test.go`:
- Around line 3-15: Replace fragile string-based error assertions in
TestRemoteImporter_Download_NotFound and
TestRemoteImporter_Download_LocalPath_Error with sentinel checks using
errors.Is: after asserting an error was returned (require.Error), assert that
errors.Is(err, ErrDownloadRemoteImport) for the "not found" case and
errors.Is(err, ErrInvalidRemoteImport) for the "local path error" case (or both
as appropriate if the wrapper preserves both), ensuring you import the standard
errors package; use errors.Is rather than substring matching because the
importer wraps sentinel errors via errUtils.Build().
- Around line 127-140: The test hardcodes expected paths with "/stacks" which
breaks on Windows; update basePath to use filepath.Join (already changed) and
modify the expected values in the tests table to use filepath.Join(basePath,
...) instead of filepath.Join("/stacks", ...). Apply this change in both
TestProcessImportPath_Local and the sibling TestProcessImportPath_Remote (update
each test case entries so their expected field references basePath via
filepath.Join(basePath, "catalog/vpc"), filepath.Join(basePath,
"mixins/region"), filepath.Join(basePath, "./local"), filepath.Join(basePath,
"../shared"), etc.).
In `@pkg/stack/imports/remote.go`:
- Around line 69-74: After applying options in the constructor (the loop over
opts calling opt(r) that returns r), ensure the configured cache directory
exists so later Download calls don't fail; specifically, after options are
applied create the directory referenced by the struct field set by WithCacheDir
(e.g., r.cacheDir) using a recursive mkdir (os.MkdirAll) and return any error if
creation fails before returning r, nil.
- Around line 3-15: In pkg/stack/imports/remote.go preserve the source extension
when creating cached filenames: import net/url and path, parse the remote URL
(url.Parse) and get the extension via path.Ext(url.Path), defaulting to ".yaml"
if empty, then use that extension instead of hardcoded ".yaml" when building the
cache filename (the code that currently composes fmt.Sprintf("%x.yaml",
sha256.Sum...)). Ensure the new extension is used everywhere the cache filename
is created/checked so IsTemplateFile() will detect ".yaml.tmpl" / ".yml.tmpl"
correctly.
In `@pkg/stack/imports/uri.go`:
- Around line 16-20: The example comment lines in the IsLocalPath documentation
(and other example blocks in this file) do not end with periods, causing godot
linter failures; update the comment block for IsLocalPath (and any other example
comment blocks in the file) so each example line ends with a period (e.g.,
"/absolute/path"., "./relative/path". etc.), ensuring all sentence-like comment
lines terminate with a period and re-run the linter.
- Around line 97-108: The function isDomainLikeURI incorrectly treats dots in
later path segments as domains; update isDomainLikeURI to first isolate the
first path segment (split uri by "/" using strings.SplitN(uri, "/", 2)), then
check for a dot only within that first segment (ensure dotPos > 0 and dotPos <
len(firstSegment)-1). Return true only when that first segment contains a valid
dot and the original uri contains a slash (indicating a domain with path) so
paths like "configs/v1.0/base" are not flagged as remote; modify references in
isDomainLikeURI accordingly.
In `@tests/cli_remote_imports_test.go`:
- Around line 123-186: Replace the external https://nonexistent.invalid URL with
a local httptest server that deterministically returns a 404 so the test doesn't
rely on DNS/network; in TestRemoteStackImports_SkipIfMissing create an
httptest.NewServer handler (use t.Cleanup or defer server.Close()) that responds
404, inject server.URL into the stackContent import path instead of the
hardcoded external URL, then proceed with writing the stack file and running
cmd.RootCmd.SetArgs/ cmd.Execute() as before so skip_if_missing behavior is
exercised reliably.
In `@website/blog/2026-01-29-remote-stack-imports.mdx`:
- Around line 36-38: Replace the overstated header sentence "Remote imports
support all go-getter URL formats:" in the "## Supported URL Formats" section
with a precise phrase such as "Remote imports support the following go-getter
URL formats:" so the text matches the enumerated list (HTTPS, GitHub, Git with
ref, S3, GCS, Git SSH) and does not claim coverage of schemes like Mercurial or
SMB; update the specific sentence string in the file (look for the exact line
containing "Remote imports support all go-getter URL formats:") accordingly.
In `@website/src/data/roadmap.js`:
- Line 294: Update the roadmap milestone object with label 'Remote stack
imports' to include the missing pr field; locate the entry in
website/src/data/roadmap.js (the object containing label: 'Remote stack
imports', status: 'shipped', quarter: 'q1-2026') and add a pr:
'<PR_URL_OR_NUMBER>' property containing the PR link or reference string to
follow the project guideline for milestone entries.
🧹 Nitpick comments (1)
pkg/stack/imports/remote.go (1)
118-125: Prefer atomic fetch to avoid partial cache writes.Concurrent downloads can overlap; using FetchAtomic avoids partially written cache files.
✅ Suggested change
@@ - if err := r.downloader.Fetch(uri, destPath, downloader.ClientModeFile, defaultDownloadTimeout); err != nil { + if err := r.downloader.FetchAtomic(uri, destPath, downloader.ClientModeFile, defaultDownloadTimeout); err != nil { return "", errUtils.Build(errUtils.ErrDownloadRemoteImport). WithCause(err). WithContext("uri", uri). WithHint("Check network connectivity and verify the URL is accessible"). Err() }
Erik Osterman (Cloud Posse) (osterman)
force-pushed
the
osterman/remote-stack-imports
branch
from
January 29, 2026 17:33
70a7d57 to
91af909
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@pkg/cache/file_cache.go`:
- Around line 69-77: The lock file is created using the initial baseDir before
options are applied so WithBaseDir (or other opts) can change c.baseDir but
leave c.lock pointing at the old path; move creation of the lock until after
applying opts (i.e., create c.lock = NewFileLock(filepath.Join(c.baseDir,
"cache")) after the for _, opt := range opts loop) or detect a baseDir change
and reassign c.lock accordingly; update the FileCache initializer code that sets
baseDir, lock, fs and the opts application so the NewFileLock call uses the
final c.baseDir (references: FileCache struct, NewFileLock, WithBaseDir option,
opts loop, c.baseDir, c.lock).
🧹 Nitpick comments (9)
examples/remote-stack-imports/README.md (1)
15-22: Add language specification to fenced code block.The directory structure code block lacks a language identifier. Use
textorplaintextfor non-code content to satisfy markdown linting.Proposed fix
-``` +```text remote-stack-imports/ ├── atmos.yaml # Atmos configuration ├── components/terraform/myapp/ # Simple mock component └── stacks/ ├── catalog/base.yaml # Local base configuration └── deploy/demo.yaml # Stack with remote imports -``` +```pkg/cache/filelock_windows.go (1)
14-18: Missingperf.Trackfor consistency with Unix implementation.The Unix version in
pkg/cache/filelock_unix.go:31includesdefer perf.Track(nil, "cache.NewFileLock")(). Consider adding it here for consistency, even though this is a no-op implementation.Proposed fix
+import ( + "time" + + "github.com/cloudposse/atmos/pkg/perf" +) // NewFileLock creates a new FileLock for the given path. // On Windows, this returns a no-op implementation. func NewFileLock(_ string) FileLock { + defer perf.Track(nil, "cache.NewFileLock")() + return &noopFileLock{} }pkg/cache/filelock_unix.go (2)
5-14: Import organization needs adjustment.Per coding guidelines, imports should be: 1) stdlib, 2) 3rd-party, 3) atmos packages. Currently
github.com/gofrs/flock(3rd-party) comes after atmos packages.Proposed fix
import ( "errors" "fmt" "time" + "github.com/gofrs/flock" + errUtils "github.com/cloudposse/atmos/errors" log "github.com/cloudposse/atmos/pkg/logger" "github.com/cloudposse/atmos/pkg/perf" - "github.com/gofrs/flock" )As per coding guidelines: "Import organization (MANDATORY): Three groups separated by blank lines, sorted alphabetically: 1) Go stdlib, 2) 3rd-party (NOT cloudposse/atmos), 3) Atmos packages."
73-98:WithRLockalways executesfn()regardless of lock acquisition.When
TryRLockfails to acquire the lock (line 84),fn()is still executed without holding a lock. The comment says "caller should handle the case where fn wasn't executed" but the function actually does executefn(). This is fine for non-critical cache reads but the comment is misleading.Clarify comment
if !locked { - // If we can't get the lock immediately, return without error. - // This prevents deadlocks during concurrent access. - // The caller should handle the case where fn wasn't executed. - return fn() // Execute without lock - cache is non-critical. + // If we can't get the lock immediately, execute without the lock. + // This prevents deadlocks during concurrent access. + // Cache reads are non-critical, so unprotected access is acceptable. + return fn() }pkg/config/cache_deadlock_test.go (2)
5-15: Import order needs adjustment.Third-party imports (testify) should come before Atmos packages per coding guidelines. Current order mixes them.
♻️ Suggested fix
import ( "errors" "sync" "testing" "time" - errUtils "github.com/cloudposse/atmos/errors" - "github.com/cloudposse/atmos/pkg/cache" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + + errUtils "github.com/cloudposse/atmos/errors" + "github.com/cloudposse/atmos/pkg/cache" )As per coding guidelines: "Import organization (MANDATORY): Three groups separated by blank lines, sorted alphabetically: 1) Go stdlib, 2) 3rd-party (NOT cloudposse/atmos), 3) Atmos packages."
154-154: Variable shadows import alias.Line 154 declares
cache, err := LoadCache()which shadows thecachepackage import from line 12. Consider renaming tocacheConfigorloadedCache.♻️ Suggested fix
- cache, err := LoadCache() + loadedCache, err := LoadCache()pkg/cache/file_cache_test.go (1)
159-169: Consider clearer test keys for concurrent access test.
string(rune(n))for n=0-9 produces control characters (NUL, SOH, etc.). While functional, using readable keys would make debugging easier if tests fail.♻️ Suggested fix
for i := 0; i < 10; i++ { wg.Add(1) go func(n int) { defer wg.Done() - key := keyToFilename(string(rune(n))) - content := []byte{byte(n)} + key := fmt.Sprintf("concurrent-key-%d", n) + content := []byte(fmt.Sprintf("content-%d", n)) err := cache.Set(key, content) assert.NoError(t, err) }(i) }pkg/cache/file_cache.go (1)
266-271: Consider removing perf.Track from trivial getter.
BaseDir()is a simple field accessor. Per coding guidelines, trivial getters can skip performance tracking.internal/exec/stack_processor_utils.go (1)
894-925: Retry logic for glob matching.The TODO comment about reviewing
doublestarlibrary is noted. The retry is a reasonable workaround for intermittent issues in containerized environments. Consider tracking this as a follow-up.Would you like me to open an issue to track investigating the doublestar library behavior?
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@pkg/config/cache.go`:
- Around line 96-102: The error path in the cache read branch is inconsistent:
when readErr != nil the Windows branch returns CacheConfig{} but the non-Windows
branch returns the possibly partially-initialized cfg; update the non-Windows
error return to also return an empty CacheConfig (i.e., return CacheConfig{},
readErr) so both branches return the same zero-value shape; change the return
using the existing symbols readErr, runtime.GOOS, cacheFile and cfg in the
function handling cache reads.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/config/cache.go (1)
125-133: Persist BrowserSessionWarningShown in SaveCache/UpdateCache.
BrowserSessionWarningShownexists inCacheConfig, but it’s never serialized in either SaveCache or UpdateCache, so updates are lost.🧩 Suggested fix
data := map[string]interface{}{ "last_checked": cfg.LastChecked, "installation_id": cfg.InstallationId, "telemetry_disclosure_shown": cfg.TelemetryDisclosureShown, + "browser_session_warning_shown": cfg.BrowserSessionWarningShown, }data := map[string]interface{}{ "last_checked": cfg.LastChecked, "installation_id": cfg.InstallationId, "telemetry_disclosure_shown": cfg.TelemetryDisclosureShown, + "browser_session_warning_shown": cfg.BrowserSessionWarningShown, }Also applies to: 192-197
🤖 Fix all issues with AI agents
In `@internal/exec/stack_processor_utils_test.go`:
- Around line 1975-2031: Tests currently use hardcoded POSIX path strings like
"/test/path.yaml" which break on Windows; update all uses in the
ProcessImportSection tests to build paths with filepath.Join (e.g.
filepath.Join("test","path.yaml")) instead of hardcoded slashes, and when
asserting path suffixes or doing string-based checks use filepath.ToSlash on the
produced path for deterministic comparisons; update occurrences in
TestProcessImportSection_NoImportSection,
TestProcessImportSection_NilImportSection, TestProcessImportSection_EmptyList,
TestProcessImportSection_InvalidType, TestProcessImportSection_NilElement and
any other ProcessImportSection tests below to use filepath.Join and
filepath.ToSlash as appropriate.
In `@pkg/cache/file_cache.go`:
- Around line 225-229: The comment says an error should be logged when
c.Set(key, content) fails but no logging is performed; either emit a non-failing
log entry with the cached error (e.g., use the existing logger on the cache
struct such as c.logger.Errorf or standard log.Printf to record the error with
context including the key) before returning content, or change the comment to
state that the error is intentionally ignored; update the code around the
c.Set(...) error branch (the Set call and its surrounding return) accordingly to
include the chosen behavior.
- Around line 137-170: The FileCache methods bypass the injected FileSystem;
update FileCache.Get to call c.fs.ReadFile(path) instead of os.ReadFile, update
FileCache.GetPath to call c.fs.Stat(path) instead of os.Stat, and update
FileCache.Clear to use c.fs.Remove(path) instead of os.Remove; for the directory
listing in Clear (currently using os.ReadDir) either add a directory-walking
method to the FileSystem interface (e.g., Walk) and use that, or extend the
interface with ReadDir so you can mock it in tests—ensure all calls reference
c.fs (e.g., c.fs.ReadFile, c.fs.Stat, c.fs.Remove or c.fs.Walk/ReadDir) and
preserve existing error handling and return semantics.
- Around line 205-223: Add a new sentinel ErrCacheFetch in errors/errors.go
(e.g., ErrCacheFetch = errors.New("failed to fetch content")), then in
FileCache.GetOrFetch wrap the error returned by fetch() using the project
builder pattern (call WithCause(err) and WithContext(...) on ErrCacheFetch)
instead of returning the raw err; update the return to propagate this wrapped
error from the GetOrFetch method so callers receive ErrCacheFetch with the
original cause.
In `@pkg/config/cache_test.go`:
- Around line 475-496: In TestUpdateCache_MultipleFields add an assertion to
verify the BrowserSessionWarningShown field is persisted: after calling
UpdateCache(...) and LoadCache(), assert that
loadedCache.BrowserSessionWarningShown is true so the test will catch
regressions to that field; reference the test function
TestUpdateCache_MultipleFields and the fields set in UpdateCache and checked on
loadedCache (BrowserSessionWarningShown).
In `@pkg/stack/imports/remote_test.go`:
- Around line 129-142: Tests like TestProcessImportPath_Local hardcode basePath
= "/stacks" which embeds POSIX separators; change basePath to be OS-safe by
constructing it with filepath.Join (e.g., filepath.Join("", "stacks") or just
filepath.Join("stacks") depending on context) and update all other tests that
define basePath to use filepath.Join as well; where tests perform path suffix
comparisons, use filepath.ToSlash on the produced path before comparing to
normalize separators. Ensure you update the basePath variable in
TestProcessImportPath_Local and the other test functions that define basePath so
no hardcoded "/" remains.
Add support for importing stack configurations from remote URLs using go-getter. Stack imports now work consistently with remote atmos.yaml imports. ## What Changed - New pkg/stack/imports package with URL detection and remote downloading - Stack imports detect remote URLs (HTTP, Git, S3, GCS) and download via go-getter - Integration with stack_processor_utils.go for unified import handling - New sentinel errors for remote import failures - Comprehensive unit tests with mock HTTP server - Integration tests verifying end-to-end functionality - Complete example demonstrating local and remote imports - Blog post announcing the feature and roadmap update ## Architecture - pkg/stack/imports/uri.go: URL detection functions (IsLocalPath, IsGitURI, IsHTTPURI, IsS3URI, IsGCSURI) - pkg/stack/imports/remote.go: Remote downloading with caching - pkg/stack/imports/imports.go: Main ProcessImportPath entry point - stack_processor_utils.go: Updated to use remote import logic ## Testing All 60+ unit tests pass: - URL detection for local vs remote paths - Remote imports from mock HTTP server - Graceful handling of missing remotes with skip_if_missing - End-to-end integration tests Closes #2036 Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Extract the robust file-based caching infrastructure from pkg/config into a reusable pkg/cache package. This includes: - pkg/cache/filelock.go: Platform-agnostic FileLock interface - pkg/cache/filelock_unix.go: Unix flock implementation with retries - pkg/cache/filelock_windows.go: Windows graceful degradation - pkg/cache/file_cache.go: Generic FileCache with atomic writes and locking Update pkg/config/cache.go to use the new pkg/cache.FileLock interface. Update pkg/stack/imports/remote.go to properly use FileCache.GetOrFetch() for thread-safe caching with atomic writes, instead of bypassing the cache's locking mechanism. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Fix isDomainLikeURI incorrectly flagging version paths (e.g., configs/v1.0/base) - Preserve file extension in cached filenames for proper template processing - Use errors.Is() for sentinel error assertions in tests - Use httptest mock server for skip_if_missing test instead of real network - Add PR 2037 link to roadmap milestone - Clarify go-getter URL format coverage in blog post Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
These sites block automated requests but URLs are manually verified as valid: - https://kubernetes.io/docs/tasks/extend-kubectl/kubectl-plugins/ - https://www.runatlantis.io/docs/pre-workflow-hooks.html Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Andriy Knysh (aknysh)
approved these changes
May 25, 2026
Contributor
|
These changes were released in v1.220.0-rc.3. |
This branch was successfully deployed
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
2 participants
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.
what
pkg/stack/importspackage handles URL detection and remote downloadingstack_processor_utils.goto detect remote URLs and download them automaticallywhy
references
website/blog/2026-01-29-remote-stack-imports.mdxexamples/remote-stack-imports/Summary by CodeRabbit
New Features
Documentation
Chores
Tests