Conversation
|
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 77.29% 77.36% +0.06%
==========================================
Files 960 964 +4
Lines 91088 91424 +336
==========================================
+ Hits 70410 70726 +316
- Misses 16593 16607 +14
- Partials 4085 4091 +6
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 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)
📝 Coding Plan
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}
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() }
osterman
force-pushed
the
osterman/remote-stack-imports
branch
from
70a7d57 to
91af909
Compare
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?
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.
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.
| if err := c.Set(key, content); err != nil { | ||
| // Log but don't fail - cache write errors are non-critical. | ||
| // The content was fetched successfully. | ||
| return content, nil | ||
| } |
There was a problem hiding this comment.
Comment says “log” but nothing logs.
Either log here or update the comment to reflect the current behavior.
📝 Comment-only fix
- // Log but don't fail - cache write errors are non-critical.
- // The content was fetched successfully.
+ // Cache write errors are non-critical; return fetched content.🤖 Prompt for AI Agents
In `@pkg/cache/file_cache.go` around lines 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.
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>
Add missing browser_session_warning_shown field to SaveCache and UpdateCache data maps so the field is properly persisted to disk. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@pkg/cache/file_cache.go`:
- Around line 110-135: The extractExtension function currently only checks
single-part extensions in validExts and thus strips compound template extensions
like ".yaml.tmpl"; update extractExtension to detect and return compound
template extensions by first checking for a ".tmpl" suffix (e.g., if
strings.HasSuffix(strings.ToLower(base), ".tmpl") then inspect the preceding
extension like ".yaml.tmpl" or ".yml.tmpl") and treat those compound extensions
as valid, or alternatively add ".tmpl" and the common compound combos to the
validExts set; ensure you reference the existing symbols extractExtension, base,
ext, and validExts when making the change so template files such as
"stack.yaml.tmpl" preserve their full extension.
In `@website/blog/2026-01-29-remote-stack-imports.mdx`:
- Around line 40-47: Add an explicit HTTP example row to the markdown table that
currently lists "HTTPS" to avoid implying HTTPS-only support; update the table
in website/blog/2026-01-29-remote-stack-imports.mdx by inserting a new row like
"HTTP | `http://example.com/config.yaml`" alongside the existing "HTTPS" row so
readers see both protocols are supported (ensure table pipes align and
formatting matches the other rows).
🧹 Nitpick comments (5)
examples/remote-stack-imports/README.md (1)
15-22: Add language specifier to the fenced code block.The directory structure block should have a language identifier to pass markdown linting. Consider using
textorplaintext.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</details> </blockquote></details> <details> <summary>pkg/cache/filelock_unix_test.go (1)</summary><blockquote> `81-105`: **Minor: Test name could be clearer.** `TestWithLock_NestedCalls` actually tests sequential lock acquisition/release, not truly nested locks. The inner function in the first `WithLock` doesn't acquire another lock. Consider renaming to `TestWithLock_SequentialCalls` for clarity. </blockquote></details> <details> <summary>pkg/config/cache_deadlock_test.go (2)</summary><blockquote> `5-15`: **Import organization needs adjustment.** The imports should follow the guideline: stdlib → 3rd-party → atmos packages. Currently, atmos packages (`errUtils`, `cache`) are mixed with 3rd-party (`testify`). <details> <summary>Proposed fix</summary> ```diff 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 imported package.The variable
cacheshadows the importedcachepackage. Consider renaming toloadedCacheorcachedConfigto avoid confusion.Proposed fix
- cache, err := LoadCache() + loadedCache, err := LoadCache() elapsed := time.Since(start) assert.NoError(t, err) assert.Less(t, elapsed, 500*time.Millisecond, "LoadCache should return quickly, took: %v", elapsed) // The cache should be empty since it couldn't acquire the lock. // This is the expected behavior according to the LoadCache implementation. - if cache.InstallationId != "" { + if loadedCache.InstallationId != "" { t.Log("LoadCache returned data despite lock being held - this is fine if it read before lock") }pkg/config/cache_test.go (1)
3-15: Import order: 3rd-party before Atmos packages.The testify import should come before Atmos packages per coding guidelines. Current order places
testifyaftererrUtilsandcache.✅ Suggested fix
import ( "os" "path/filepath" "runtime" "strings" "testing" "time" "github.com/adrg/xdg" + "github.com/stretchr/testify/assert" + errUtils "github.com/cloudposse/atmos/errors" "github.com/cloudposse/atmos/pkg/cache" - "github.com/stretchr/testify/assert" )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.
|
💥 This pull request now has conflicts. Could you fix it @osterman? 🙏 |
…-imports Resolve conflict in internal/exec/stack_processor_utils_test.go by keeping both sets of new tests from each branch. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@internal/exec/stack_processor_utils.go`:
- Around line 1021-1058: The error strings returned when no import matches are
found in processImports (stack_processor_utils.go) must be wrapped with the
project sentinel errors instead of errors.New; update the branches that
currently return errors.New(...) (the cases using relativeFilePath, imp,
importMatches and err after GetGlobMatches retry and the SkipIfMissing check) to
wrap with the appropriate static sentinel from errors/errors.go (e.g.
errUtils.ErrInvalidImport or a more specific sentinel) using fmt.Errorf("%w:
...", sentinel) and include the underlying err (or use errors.Join) so callers
can use errors.Is/IsGolangTemplate/Is checks; keep the original message text for
context but with %w/Join wrapping.
- Around line 970-985: Remote imports are currently replacing the user-provided
URI with the cached localPath (importMatches = []string{localPath}), which
causes downstream lookups (ProcessImportSection →
FindComponentDependenciesLegacy) and the final imports output to use cache paths
instead of the original URI; change the logic so the downloaded file(s) still
use localPath for content access but preserve the original imp (URI) as the
import key (e.g., set an importKey := imp and pass/attach that key to wherever
importMatches are consumed or to the imports map creation) so lookups and output
remain keyed by the user-specified URI while file resolution uses localPath from
stackimports.DownloadRemoteImport.
In `@pkg/cache/file_cache.go`:
- Around line 82-89: The cache directory creation currently calls os.MkdirAll
directly; replace this with the FileSystem abstraction by calling
c.fs.MkdirAll(c.baseDir, DefaultCacheDirPerm) so mocks set via WithFileSystem
are used in tests; modify the block that references
c.baseDir/DefaultCacheDirPerm to use the FileSystem implementation (method
MkdirAll on the FileSystem stored in c.fs) and propagate errors the same way.
🧹 Nitpick comments (2)
internal/exec/stack_processor_utils.go (1)
620-623: Optional: consider a helper to avoid expanding nolint scope.
A small extraction for remote/local import resolution would keep the core flow lean.internal/exec/stack_processor_utils_test.go (1)
1976-2032: Optional: table-drive the ProcessImportSection edge cases.
These five variants are tightly related; a single table-driven test would reduce repetition and ease future additions.As per coding guidelines: Use table-driven tests for comprehensive coverage - Prefer unit tests with mocks over integration tests - Target >80% coverage.
|
💥 This pull request now has conflicts. Could you fix it @osterman? 🙏 |
- Preserve original URI as import key for remote imports instead of cache paths - Wrap import errors with sentinel errors (ErrStackImportSelf, ErrStackImportNotFound) - Use c.fs.MkdirAll instead of os.MkdirAll in file_cache.go for filesystem abstraction - Resolve merge conflicts in pkg/config/cache.go and pkg/config/cache_test.go Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
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)
51-214: 🛠️ Refactor suggestion | 🟠 MajorAdd perf.Track to public cache functions. The public functions here still lack perf tracking, which is required for non-trivial public APIs. Consider adding it to LoadCache, SaveCache, UpdateCache, and ShouldCheckForUpdates.
As per coding guidelines: Add `defer perf.Track(atmosConfig, "pkg.FuncName")()` + blank line to all public functions, except trivial getters/setters, command constructors, simple factories, and functions that only delegate to another tracked function.Example pattern
+import ( + ... + "github.com/cloudposse/atmos/pkg/perf" + ... +) + func LoadCache() (CacheConfig, error) { + defer perf.Track(nil, "config.LoadCache")() + cacheFile, err := GetCacheFilePath() ... }
🤖 Fix all issues with AI agents
In `@pkg/cache/file_cache.go`:
- Around line 248-287: The Clear method may remove the active lock file while
walking c.baseDir with c.fs.Walk; change FileCache.Clear to compute the lock
file path before calling c.fs.Walk (obtain it from c.lock via its path/filename
accessor or stored field) and inside the Walk callback skip that specific path
(return nil) so the lock file is never removed; keep the existing error handling
for other files and directories and still run the whole operation under
c.lock.WithLock.
🧹 Nitpick comments (1)
lychee.toml (1)
41-44: Remove duplicatekubernetes\.ioexclusionIt’s listed twice (Line 41-42 and Line 65-66). Keeping a single entry reduces noise and future confusion.
🧹 Proposed cleanup
# Atmos Pro redirects to sign-in page for unauthenticated users "atmos-pro\\.com", - # Kubernetes.io (frequent connection failures/timeouts) - "kubernetes\\.io", ]Also applies to: 65-66
Fix a bug where FileCache.Clear() would delete the lock file (cache.lock) while walking the cache directory, breaking mutual exclusion for concurrent processes. Add lockFilePath field to FileCache and skip it during Clear(). Improve test coverage across cache, imports, and config packages: - pkg/cache: 72%/70% -> 91.7% (lock error paths, retry exhaustion, fallback) - pkg/stack/imports: 85%/89% -> 95.7% (memory cache invalidation, URI helpers) - pkg/config: 72% -> 86.8% (corrupted file handling, docstring for CacheConfig) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@pkg/stack/imports/remote_test.go`:
- Around line 253-291: TestRemoteImporter_Download_MemoryCacheInvalidation uses
a non-synchronized downloadCount accessed from the httptest handler goroutine
and the test goroutine; change downloadCount to an int32 or int64 and replace
direct increments and reads with atomic operations (e.g.,
atomic.AddInt32/atomic.LoadInt32 or the int64 variants) in the handler and where
initialCount, comparisons, and assertions read it, and add the necessary import
for sync/atomic; update references to downloadCount in the handler and at lines
where initialCount, subsequent reads, and assertions occur so they use
atomic.Load instead of unsynchronized reads.
🧹 Nitpick comments (6)
pkg/stack/imports/uri_test.go (1)
9-75: Consider adding an empty-string edge case toTestIsLocalPath.The table covers a great range — local paths, remote URLs, version-like segments, domain heuristics. One thing missing: what happens with
"". It's a common boundary input and documenting the expected behavior here prevents future surprises.Same applies to
TestIsRemote(lines 77–101).Suggested additions
In
TestIsLocalPath:+ {"empty string", "", true}, }In
TestIsRemote:+ {"empty string", "", false}, }(Adjust expected values to match your implementation's actual behavior.)
pkg/cache/filelock_unix_test.go (1)
84-108: Test nameTestWithLock_NestedCallsis misleading — this tests sequential re-acquisition.The comment on Line 100 confirms what it actually tests: acquiring the lock again after the previous lock is released. Consider renaming to
TestWithLock_SequentialAcquisitionfor clarity.pkg/config/cache.go (1)
67-104: The two-variable error pattern inLoadCacheworks but is subtle.
readErris captured in the closure while the callback always returnsnil, solockErronly reflects lock acquisition failures. This separates lock errors (non-critical, line 89) from read errors (platform-dependent, line 96). It's correct but worth a brief inline comment explaining why the callback swallows errors intoreadErrinstead of returning them directly.pkg/cache/file_cache.go (1)
220-249:GetOrFetchhas a small thundering-herd window.Two concurrent callers for the same uncached key will both call
fetch()sinceGetandSetaren't atomic together. For remote stack imports this is unlikely to matter in practice — the duplicate download is harmless and the secondSetjust overwrites with identical content. Worth knowing, not worth blocking.pkg/cache/file_cache_test.go (1)
178-194: Concurrent test key generation is a bit confusing.Line 187 passes
keyToFilename(string(rune(n)))as the key tocache.Set, which then internally callskeyToFilenameagain — resulting in a double-hash. It works but is unnecessarily opaque.Cleaner alternative
- key := keyToFilename(string(rune(n))) + key := fmt.Sprintf("key-%d", n)pkg/stack/imports/remote_test.go (1)
293-344: Global importer error propagation and ResolveImportPaths error tests complete the coverage.One note: the global state resets (lines 312-314, 326-328) make these tests inherently non-parallelizable. That's fine as long as no one adds
t.Parallel()later. A brief comment like// Not safe for t.Parallel() — mutates package globals.at the top of each would help.
Use atomic.Int32 for downloadCount in TestRemoteImporter_Download_MemoryCacheInvalidation to fix race between httptest handler goroutine and test goroutine. Add test coverage for error paths: - FileCache.Get() with non-NotExist read errors - FileCache.Set() with write errors on read-only directory - FileCache.Clear() with file removal errors - IsGitURI() with malformed URL containing invalid escape sequence Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
💥 This pull request now has conflicts. Could you fix it @osterman? 🙏 |
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
github-actions
bot
added
size/l
|
💥 This pull request now has conflicts. Could you fix it @osterman? 🙏 |
1 participant
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