feat: Implement unified import adapter registry#1897
Conversation
Add a unified adapter registry pattern for all import handling (local, remote, mock, etc). This replaces the previous if/else chain with extensible adapter architecture. - Add ImportAdapter interface for pluggable import sources - Add ImportAdapterRegistry with lazy initialization via sync.Once - Implement GoGetterAdapter for remote URLs (http, s3, git, oci, etc) - Implement LocalAdapter for filesystem paths (default fallback) - Implement MockAdapter for mock:// scheme (testing) - Simplify processImports() to use unified registry - Remove obsolete functions (isRemoteImport, processRemoteImport, processLocalImport, downloadRemoteConfig) - Export ResolvedPaths struct fields for adapter use - Add comprehensive adapter tests 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
|
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 |
📝 WalkthroughWalkthroughIntroduces an ImportAdapter system: a new ImportAdapter interface, a thread-safe global registry with lazy builtin initialization, three built-in adapters (GoGetter, Local, Mock), refactors import resolution to route through adapters, adds tests and docs, and registers adapters via a blank import in Changes
Sequence DiagramsequenceDiagram
participant Caller as Caller
participant Importer as Import Resolver
participant Registry as Adapter Registry
participant Adapter as Selected Adapter
participant Storage as FS/Remote/Mock
Note over Registry: lazy builtin init possible
Importer->>Registry: EnsureAdaptersInitialized()
Registry-->>Registry: invoke builtin initializer (register adapters)
loop per import
Caller->>Importer: ProcessImports(basePath, importPaths)
Importer->>Registry: FindImportAdapter(importPath)
Registry-->>Importer: Adapter (matched or default)
Importer->>Adapter: Resolve(ctx, importPath, basePath, tempDir, depth, maxDepth)
Adapter->>Storage: fetch/load/generate YAML
Storage-->>Adapter: YAML content or error
Adapter-->>Importer: []ResolvedPaths (may include nested imports)
alt nested imports present
Importer->>Importer: ProcessImportsFromAdapter(nested paths...)
end
end
Importer-->>Caller: aggregated []ResolvedPaths
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
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: 3
🧹 Nitpick comments (9)
pkg/config/import_test.go (1)
22-27: Prefert.Setenvoveros.Unsetenvfor test isolation.Per testing best practices in this codebase, use
t.Setenvfor environment variable setup/teardown to ensure test-scoped isolation. This applies to lines 22-27, 95-100, and similar patterns throughout the file.Recommended refactor
- err := os.Unsetenv("ATMOS_CLI_CONFIG_PATH") - assert.NoError(t, err, "Unset 'ATMOS_CLI_CONFIG_PATH' environment variable should execute without error") - err = os.Unsetenv("ATMOS_BASE_PATH") - assert.NoError(t, err, "Unset 'ATMOS_BASE_PATH' environment variable should execute without error") - err = os.Unsetenv("ATMOS_LOGS_LEVEL") - assert.NoError(t, err, "Unset 'ATMOS_LOGS_LEVEL' environment variable should execute without error") + t.Setenv("ATMOS_CLI_CONFIG_PATH", "") + t.Setenv("ATMOS_BASE_PATH", "") + t.Setenv("ATMOS_LOGS_LEVEL", "")Based on learnings, tests should prefer
t.Setenvfor automatic setup/teardown.pkg/config/adapters/gogetter_adapter.go (1)
112-116: Hardcoded 5-second timeout may be too restrictive.Large remote configurations or slow networks could fail with this timeout. Consider making the timeout configurable via AtmosConfiguration settings or at least increase it to a more reasonable default (e.g., 30-60 seconds).
Suggested enhancement
- ctx, cancel = context.WithTimeout(context.Background(), 5*time.Second) + // Use a longer default timeout for remote config downloads. + // Large configs or slow networks need more time. + ctx, cancel = context.WithTimeout(context.Background(), 30*time.Second)pkg/config/imports.go (1)
141-141: Background context without timeout could cause hangs.Creating an unbounded
context.Background()means adapter resolution could hang indefinitely on slow or unresponsive sources. Consider adding a reasonable timeout or making it configurable.Suggested enhancement
- ctx := context.Background() + // Set a reasonable timeout for import resolution to prevent hangs. + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Minute) + defer cancel()Alternatively, accept a context parameter in
processImportsto allow callers to control timeouts.pkg/config/import_adapter_registry.go (1)
149-166: Consider returning an error from noopAdapter.Resolve().The noopAdapter is a fallback that "should never happen in practice." Returning an error would make misconfiguration easier to debug rather than silently returning nil.
🔎 Proposed change
func (n *noopAdapter) Resolve( _ context.Context, _ string, _ string, _ string, _ int, _ int, ) ([]ResolvedPaths, error) { - return nil, nil + return nil, fmt.Errorf("no default adapter registered (this should not happen)") }You'll need to import
fmtif you make this change.pkg/config/adapters/adapters_test.go (3)
151-160: Consider defining a static error for MockAdapter.Line 159 checks error content via string matching. Per coding guidelines, static errors should be defined in errors/errors.go for better error handling.
This allows using
assert.ErrorIs()instead of string matching, making tests more robust against message changes.
39-54: Clarify test intent or integrate with adapter.This test directly invokes Viper instead of testing the adapter's behavior. Consider either calling the adapter's
Resolve()method or clarifying in comments why direct Viper testing is needed here.Per test guidelines, prefer testing behavior over implementation details.
90-107: Test name doesn't match behavior.The test is named
TestLocalAdapter_ReadConfigErrorbut assertsNoError. Consider renaming toTestLocalAdapter_SkipsInvalidYAMLto better reflect the graceful degradation behavior being tested.pkg/config/adapters/mock_adapter.go (2)
94-99: Consider error handling for nested imports.Line 97 silently discards errors from nested import processing. The comment mentions logging but no logging occurs. Consider either:
- Propagating the error:
return paths, fmt.Errorf("failed to process nested imports: %w", err)- Actually logging the error using appropriate logging functions
Per coding guidelines, errors should be properly wrapped and communicated.
31-53: Document thread-safety expectations for global mock adapter.The global mock adapter's
MockDatamap is not mutex-protected. If tests usingSetMockData()run in parallel, race conditions may occur. Consider either:
- Adding a comment that tests using the global mock adapter should not run in parallel
- Adding mutex protection to
MockDataaccess
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (13)
cmd/root.go(1 hunks)docs/prd/import-adapter-registry.md(1 hunks)pkg/config/adapters/adapters_test.go(1 hunks)pkg/config/adapters/gogetter_adapter.go(1 hunks)pkg/config/adapters/init.go(1 hunks)pkg/config/adapters/local_adapter.go(1 hunks)pkg/config/adapters/mock_adapter.go(1 hunks)pkg/config/import_adapter.go(1 hunks)pkg/config/import_adapter_registry.go(1 hunks)pkg/config/import_test.go(8 hunks)pkg/config/import_test_helpers_test.go(1 hunks)pkg/config/imports.go(3 hunks)pkg/config/imports_error_paths_test.go(0 hunks)
💤 Files with no reviewable changes (1)
- pkg/config/imports_error_paths_test.go
🧰 Additional context used
📓 Path-based instructions (4)
cmd/**/*.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
cmd/**/*.go: Use Cobra's recommended command structure with a root command and subcommands, implementing each command in a separate file undercmd/directory
Provide comprehensive help text for all commands and flags, include examples in command help, and follow Go's documentation conventions in Cobra command definitions
Provide meaningful feedback to users and include progress indicators for long-running operations in CLI commands
cmd/**/*.go: Commands MUST use flags.NewStandardParser() for command-specific flags; NEVER call viper.BindEnv() or viper.BindPFlag() directly (Forbidigo enforces this); see cmd/version/version.go for reference
Embed CLI examples from cmd/markdown/*_usage.md using //go:embed; render with utils.PrintfMarkdown()
Files:
cmd/root.go
**/*.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
**/*.go: Use Viper for managing configuration, environment variables, and flags in CLI commands
Use interfaces for external dependencies to facilitate mocking and consider using testify/mock for creating mock implementations
All code must pass golangci-lint checks
Follow Go's error handling idioms: use meaningful error messages, wrap errors with context usingfmt.Errorf("context: %w", err), and consider using custom error types for domain-specific errors
Follow standard Go coding style: usegofmtandgoimportsto format code, prefer short descriptive variable names, use kebab-case for command-line flags, and snake_case for environment variables
Document all exported functions, types, and methods following Go's documentation conventions
Document complex logic with inline comments in Go code
Support configuration via files, environment variables, and flags following the precedence order: flags > environment variables > config file > defaults
Provide clear error messages to users, include troubleshooting hints when appropriate, and log detailed errors for debugging
**/*.go: NEVER use fmt.Fprintf(os.Stdout/Stderr) or fmt.Println(); use data.* or ui.* functions instead
All comments must end with periods (enforced by godot linter)
Organize imports in three groups separated by blank lines, sorted alphabetically: 1) Go stdlib, 2) 3rd-party (NOT cloudposse/atmos), 3) Atmos packages; maintain aliases: cfg, log, u, errUtils
Adddefer perf.Track(atmosConfig, "pkg.FuncName")()+ blank line to all public functions for performance tracking; use nil if no atmosConfig param
All errors MUST be wrapped using static errors defined in errors/errors.go; use errors.Join for combining multiple errors; use fmt.Errorf with %w for adding string context; use error builder for complex errors; use errors.Is() for error checking; NEVER use dynamic errors directly
Use go.uber.org/mock/mockgen with //go:generate directives for mock generation; never create manual mocks
Keep files small...
Files:
cmd/root.gopkg/config/adapters/local_adapter.gopkg/config/adapters/gogetter_adapter.gopkg/config/import_adapter_registry.gopkg/config/import_adapter.gopkg/config/adapters/init.gopkg/config/imports.gopkg/config/adapters/adapters_test.gopkg/config/adapters/mock_adapter.gopkg/config/import_test_helpers_test.gopkg/config/import_test.go
**/*_test.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
**/*_test.go: Every new feature must include comprehensive unit tests targeting >80% code coverage for all packages
Use table-driven tests for testing multiple scenarios in Go
Include integration tests for command flows and test CLI end-to-end when possible with test fixtures
**/*_test.go: Prefer unit tests with mocks over integration tests; use interfaces + dependency injection for testability; generate mocks with go.uber.org/mock/mockgen; use table-driven tests; target >80% coverage
Test behavior, not implementation; never test stub functions; avoid tautological tests; make code testable via DI; no coverage theater; remove always-skipped tests; use errors.Is() for error checking
Files:
pkg/config/adapters/adapters_test.gopkg/config/import_test_helpers_test.gopkg/config/import_test.go
docs/prd/**/*.md
📄 CodeRabbit inference engine (CLAUDE.md)
All Product Requirement Documents (PRDs) MUST be placed in docs/prd/ with kebab-case filenames
Files:
docs/prd/import-adapter-registry.md
🧠 Learnings (42)
📓 Common learnings
Learnt from: aknysh
Repo: cloudposse/atmos PR: 768
File: internal/exec/vendor_model_component.go:3-20
Timestamp: 2024-11-18T13:59:10.824Z
Learning: When replacing significant dependencies like `go-getter` that require extensive changes, prefer to address them in separate PRs.
Learnt from: aknysh
Repo: cloudposse/atmos PR: 1363
File: internal/exec/template_utils.go:18-18
Timestamp: 2025-07-05T20:59:02.914Z
Learning: In the Atmos project, gomplate v4 is imported with a blank import (`_ "github.com/hairyhenderson/gomplate/v4"`) alongside v3 imports to resolve AWS SDK version conflicts. V3 uses older AWS SDK versions that conflict with newer AWS modules used by Atmos. A full migration to v4 requires extensive refactoring due to API changes and should be handled in a separate PR.
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T18:20:55.630Z
Learning: Applies to **/*.go : Organize imports in three groups separated by blank lines, sorted alphabetically: 1) Go stdlib, 2) 3rd-party (NOT cloudposse/atmos), 3) Atmos packages; maintain aliases: cfg, log, u, errUtils
Learnt from: haitham911
Repo: cloudposse/atmos PR: 1091
File: pkg/config/imports.go:0-0
Timestamp: 2025-04-03T11:00:27.980Z
Learning: The Atmos configuration system intentionally supports both HTTP and HTTPS protocols for remote imports, as confirmed by the project maintainers. This is implemented in the `isRemoteImport` and `processRemoteImport` functions in pkg/config/imports.go.
Learnt from: osterman
Repo: cloudposse/atmos PR: 1466
File: website/docs/cli/commands/toolchain/usage.mdx:117-121
Timestamp: 2025-09-08T01:25:44.958Z
Learning: XDG Base Directory Specification compliance implementation for atmos toolchain is complete: created toolchain/xdg_cache.go with GetXDGCacheDir() and GetXDGTempCacheDir() functions, updated toolchain/installer.go and cmd/toolchain_clean.go to use these XDG helpers, and changed all cache paths from hardcoded ~/.cache/tools-cache to XDG-compliant ${XDG_CACHE_HOME}/atmos-toolchain (or ~/.cache/atmos-toolchain fallback).
Learnt from: osterman
Repo: cloudposse/atmos PR: 1697
File: internal/exec/oci_utils.go:0-0
Timestamp: 2025-11-08T19:56:18.660Z
Learning: In the Atmos codebase, when a function receives an `*schema.AtmosConfiguration` parameter, it should read configuration values from `atmosConfig.Settings` fields rather than using direct `os.Getenv()` or `viper.GetString()` calls. The Atmos pattern is: viper.BindEnv in cmd/root.go binds environment variables → Viper unmarshals into atmosConfig.Settings via mapstructure → business logic reads from the Settings struct. This provides centralized config management, respects precedence, and enables testability. Example: `atmosConfig.Settings.AtmosGithubToken` instead of `os.Getenv("ATMOS_GITHUB_TOKEN")` in functions like `getGHCRAuth` in internal/exec/oci_utils.go.
📚 Learning: 2025-12-16T18:20:55.630Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T18:20:55.630Z
Learning: Applies to **/*.go : Organize imports in three groups separated by blank lines, sorted alphabetically: 1) Go stdlib, 2) 3rd-party (NOT cloudposse/atmos), 3) Atmos packages; maintain aliases: cfg, log, u, errUtils
Applied to files:
cmd/root.gopkg/config/imports.gopkg/config/import_test.godocs/prd/import-adapter-registry.md
📚 Learning: 2025-12-13T04:37:25.223Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1686
File: cmd/root.go:0-0
Timestamp: 2025-12-13T04:37:25.223Z
Learning: In Atmos cmd/root.go Execute(), after cfg.InitCliConfig, we must call both toolchainCmd.SetAtmosConfig(&atmosConfig) and toolchain.SetAtmosConfig(&atmosConfig) so the CLI wrapper and the toolchain package receive configuration; missing either can cause nil-pointer panics in toolchain path resolution.
Applied to files:
cmd/root.go
📚 Learning: 2025-07-05T20:59:02.914Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 1363
File: internal/exec/template_utils.go:18-18
Timestamp: 2025-07-05T20:59:02.914Z
Learning: In the Atmos project, gomplate v4 is imported with a blank import (`_ "github.com/hairyhenderson/gomplate/v4"`) alongside v3 imports to resolve AWS SDK version conflicts. V3 uses older AWS SDK versions that conflict with newer AWS modules used by Atmos. A full migration to v4 requires extensive refactoring due to API changes and should be handled in a separate PR.
Applied to files:
cmd/root.gopkg/config/imports.gopkg/config/import_test.godocs/prd/import-adapter-registry.md
📚 Learning: 2024-12-11T18:40:12.808Z
Learnt from: Listener430
Repo: cloudposse/atmos PR: 844
File: cmd/helmfile.go:37-37
Timestamp: 2024-12-11T18:40:12.808Z
Learning: In the atmos project, `cliConfig` is initialized within the `cmd` package in `root.go` and can be used in other command files.
Applied to files:
cmd/root.go
📚 Learning: 2025-10-11T19:04:55.909Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1599
File: internal/exec/workflow.go:4-4
Timestamp: 2025-10-11T19:04:55.909Z
Learning: In Go, blank importing the embed package (`import _ "embed"`) is the correct and idiomatic way to enable `go:embed` directives. The blank import is required for the compiler to recognize and process `//go:embed` directives. The embed package should not be imported with a named import unless its exported functions are explicitly used, which is rare.
Applied to files:
cmd/root.go
📚 Learning: 2025-12-13T03:21:35.786Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1813
File: cmd/terraform/shell.go:28-73
Timestamp: 2025-12-13T03:21:35.786Z
Learning: In Atmos, when calling cfg.InitCliConfig, you must first populate the schema.ConfigAndStacksInfo struct with global flag values using flags.ParseGlobalFlags(cmd, v) rather than passing an empty struct. The LoadConfig function (pkg/config/load.go) reads config selection fields (AtmosConfigFilesFromArg, AtmosConfigDirsFromArg, BasePath, ProfilesFromArg) directly from the ConfigAndStacksInfo struct, NOT from Viper. Passing an empty struct causes config selection flags (--base-path, --config, --config-path, --profile) to be silently ignored. Correct pattern: parse flags → populate struct → call InitCliConfig. See cmd/terraform/plan_diff.go for reference implementation.
Applied to files:
cmd/root.go
📚 Learning: 2025-09-13T18:06:07.674Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 1466
File: toolchain/list.go:39-42
Timestamp: 2025-09-13T18:06:07.674Z
Learning: In the cloudposse/atmos repository, for UI messages in the toolchain package, use utils.PrintfMessageToTUI instead of log.Error or fmt.Fprintln(os.Stderr, ...). Import pkg/utils with alias "u" to follow the established pattern.
Applied to files:
cmd/root.go
📚 Learning: 2025-12-16T18:20:55.630Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T18:20:55.630Z
Learning: Applies to **/*.go : Use ATMOS_ prefix for environment variables with viper.BindEnv("ATMOS_VAR", "ATMOS_VAR", "FALLBACK")
Applied to files:
cmd/root.gopkg/config/imports.go
📚 Learning: 2025-12-16T18:20:55.630Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T18:20:55.630Z
Learning: Applies to **/*.go : Add `defer perf.Track(atmosConfig, "pkg.FuncName")()` + blank line to all public functions for performance tracking; use nil if no atmosConfig param
Applied to files:
cmd/root.go
📚 Learning: 2024-10-23T21:36:40.262Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 740
File: cmd/cmd_utils.go:340-359
Timestamp: 2024-10-23T21:36:40.262Z
Learning: In the Go codebase for Atmos, when reviewing functions like `checkAtmosConfig` in `cmd/cmd_utils.go`, avoid suggesting refactoring to return errors instead of calling `os.Exit` if such changes would significantly increase the scope due to the need to update multiple call sites.
Applied to files:
cmd/root.go
📚 Learning: 2025-11-08T19:56:18.660Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1697
File: internal/exec/oci_utils.go:0-0
Timestamp: 2025-11-08T19:56:18.660Z
Learning: In the Atmos codebase, when a function receives an `*schema.AtmosConfiguration` parameter, it should read configuration values from `atmosConfig.Settings` fields rather than using direct `os.Getenv()` or `viper.GetString()` calls. The Atmos pattern is: viper.BindEnv in cmd/root.go binds environment variables → Viper unmarshals into atmosConfig.Settings via mapstructure → business logic reads from the Settings struct. This provides centralized config management, respects precedence, and enables testability. Example: `atmosConfig.Settings.AtmosGithubToken` instead of `os.Getenv("ATMOS_GITHUB_TOKEN")` in functions like `getGHCRAuth` in internal/exec/oci_utils.go.
Applied to files:
cmd/root.go
📚 Learning: 2025-08-16T23:33:07.477Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 1405
File: internal/exec/describe_dependents_test.go:651-652
Timestamp: 2025-08-16T23:33:07.477Z
Learning: In the cloudposse/atmos Go codebase, ExecuteDescribeDependents expects a pointer to AtmosConfiguration (*schema.AtmosConfiguration), so when calling it with a value returned by cfg.InitCliConfig (which returns schema.AtmosConfiguration), the address-of operator (&) is necessary: ExecuteDescribeDependents(&atmosConfig, ...).
Applied to files:
cmd/root.go
📚 Learning: 2025-12-13T04:37:40.435Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1686
File: cmd/toolchain/get.go:23-40
Timestamp: 2025-12-13T04:37:40.435Z
Learning: In Go CLI command files using Cobra, constrain the subcommand to accept at most one positional argument (MaximumNArgs(1)) so it supports both listing all items (zero args) and fetching a specific item (one arg). Define and parse flags with a standard parser (e.g., flags.NewStandardParser()) and avoid binding flags to Viper (no viper.BindEnv/BindPFlag). This promotes explicit argument handling and predictable flag behavior across command files.
Applied to files:
cmd/root.go
📚 Learning: 2025-11-24T17:35:37.209Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-11-24T17:35:37.209Z
Learning: Applies to **/*.go : Use Viper for managing configuration, environment variables, and flags in CLI commands
Applied to files:
pkg/config/imports.go
📚 Learning: 2025-04-23T15:02:50.246Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1202
File: pkg/utils/yaml_func_exec.go:104-104
Timestamp: 2025-04-23T15:02:50.246Z
Learning: In the Atmos codebase, direct calls to `os.Getenv` should be avoided. Instead, use `viper.BindEnv` for environment variable access. This provides a consistent approach to configuration management across the codebase.
Applied to files:
pkg/config/imports.go
📚 Learning: 2025-12-13T06:19:17.739Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1686
File: pkg/http/client.go:0-0
Timestamp: 2025-12-13T06:19:17.739Z
Learning: In pkg/http/client.go, GetGitHubTokenFromEnv intentionally reads viper.GetString("github-token") to honor precedence of the persistent --github-token flag over ATMOS_GITHUB_TOKEN and GITHUB_TOKEN. Do not replace this with os.Getenv in future reviews; using Viper here is by design.
Applied to files:
pkg/config/imports.go
📚 Learning: 2025-12-16T18:20:55.630Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T18:20:55.630Z
Learning: Applies to cmd/**/*.go : Commands MUST use flags.NewStandardParser() for command-specific flags; NEVER call viper.BindEnv() or viper.BindPFlag() directly (Forbidigo enforces this); see cmd/version/version.go for reference
Applied to files:
pkg/config/imports.go
📚 Learning: 2025-09-29T15:47:10.908Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 1540
File: internal/exec/terraform_cli_args_utils.go:64-73
Timestamp: 2025-09-29T15:47:10.908Z
Learning: In the Atmos codebase, viper.BindEnv is required for CLI commands in the cmd/ package, but internal utilities can use os.Getenv directly when parsing environment variables for business logic purposes. The requirement to use viper is specific to the CLI interface layer, not all environment variable access throughout the codebase.
Applied to files:
pkg/config/imports.go
📚 Learning: 2025-12-16T18:20:55.630Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T18:20:55.630Z
Learning: Applies to **/*_test.go : Prefer unit tests with mocks over integration tests; use interfaces + dependency injection for testability; generate mocks with go.uber.org/mock/mockgen; use table-driven tests; target >80% coverage
Applied to files:
pkg/config/adapters/adapters_test.gopkg/config/adapters/mock_adapter.gopkg/config/import_test.go
📚 Learning: 2025-11-24T17:35:37.209Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-11-24T17:35:37.209Z
Learning: Applies to **/*_test.go : Include integration tests for command flows and test CLI end-to-end when possible with test fixtures
Applied to files:
pkg/config/adapters/adapters_test.gopkg/config/import_test.go
📚 Learning: 2025-11-24T17:35:37.209Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-11-24T17:35:37.209Z
Learning: Applies to **/*_test.go : Every new feature must include comprehensive unit tests targeting >80% code coverage for all packages
Applied to files:
pkg/config/adapters/adapters_test.go
📚 Learning: 2025-11-24T17:35:37.209Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-11-24T17:35:37.209Z
Learning: Applies to **/*_test.go : Use table-driven tests for testing multiple scenarios in Go
Applied to files:
pkg/config/adapters/adapters_test.gopkg/config/import_test.go
📚 Learning: 2025-12-16T18:20:55.630Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T18:20:55.630Z
Learning: Applies to **/*_test.go : Test behavior, not implementation; never test stub functions; avoid tautological tests; make code testable via DI; no coverage theater; remove always-skipped tests; use errors.Is() for error checking
Applied to files:
pkg/config/adapters/adapters_test.gopkg/config/import_test.go
📚 Learning: 2025-11-24T17:35:37.209Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-11-24T17:35:37.209Z
Learning: Applies to **/*.go : Use interfaces for external dependencies to facilitate mocking and consider using testify/mock for creating mock implementations
Applied to files:
pkg/config/adapters/adapters_test.gopkg/config/adapters/mock_adapter.gopkg/config/import_test.go
📚 Learning: 2025-11-10T23:23:39.771Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1686
File: toolchain/registry/aqua/aqua_test.go:417-442
Timestamp: 2025-11-10T23:23:39.771Z
Learning: In Atmos toolchain AquaRegistry, tests should not hit real GitHub. Use the options pattern via WithGitHubBaseURL to inject an httptest server URL and make GetLatestVersion/GetAvailableVersions deterministic.
Applied to files:
pkg/config/adapters/adapters_test.gopkg/config/import_test.go
📚 Learning: 2025-12-16T18:20:55.630Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T18:20:55.630Z
Learning: Define interfaces for all major functionality and use dependency injection for testability; generate mocks with go.uber.org/mock/mockgen
Applied to files:
pkg/config/adapters/adapters_test.gopkg/config/adapters/mock_adapter.gopkg/config/import_test.go
📚 Learning: 2025-12-16T18:20:55.630Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T18:20:55.630Z
Learning: Applies to **/*.go : Use go.uber.org/mock/mockgen with //go:generate directives for mock generation; never create manual mocks
Applied to files:
pkg/config/adapters/mock_adapter.go
📚 Learning: 2025-05-23T19:51:47.091Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 1255
File: cmd/describe_affected_test.go:15-15
Timestamp: 2025-05-23T19:51:47.091Z
Learning: The atmos codebase has a custom extension to *testing.T that provides a Chdir method, allowing test functions to call t.Chdir() to change working directories during tests. This is used consistently across test files in the codebase.
Applied to files:
pkg/config/import_test.go
📚 Learning: 2025-11-11T03:47:45.878Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1686
File: toolchain/add_test.go:67-77
Timestamp: 2025-11-11T03:47:45.878Z
Learning: In the cloudposse/atmos codebase, tests should prefer t.Setenv for environment variable setup/teardown instead of os.Setenv/Unsetenv to ensure test-scoped isolation.
Applied to files:
pkg/config/import_test.go
📚 Learning: 2025-11-11T03:47:59.576Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1686
File: toolchain/which_test.go:166-223
Timestamp: 2025-11-11T03:47:59.576Z
Learning: In the cloudposse/atmos repo, tests that manipulate environment variables should use testing.T.Setenv for automatic setup/teardown instead of os.Setenv/Unsetenv.
Applied to files:
pkg/config/import_test.go
📚 Learning: 2025-08-15T14:43:41.030Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 1352
File: pkg/store/artifactory_store_test.go:108-113
Timestamp: 2025-08-15T14:43:41.030Z
Learning: In test files for the atmos project, it's acceptable to ignore errors from os.Setenv/Unsetenv operations during test environment setup and teardown, as these are controlled test scenarios.
Applied to files:
pkg/config/import_test.go
📚 Learning: 2025-05-23T19:51:47.091Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 1255
File: cmd/describe_affected_test.go:15-15
Timestamp: 2025-05-23T19:51:47.091Z
Learning: In the atmos codebase, t.Chdir() is a valid method call on *testing.T objects and works correctly for changing directories in tests. This is implemented through custom testing framework extensions and is used consistently throughout the test suite.
Applied to files:
pkg/config/import_test.go
📚 Learning: 2025-05-23T19:51:47.091Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 1255
File: cmd/describe_affected_test.go:15-15
Timestamp: 2025-05-23T19:51:47.091Z
Learning: In the atmos codebase, t.Chdir() is a valid method call on *testing.T objects and works correctly for changing directories in tests.
Applied to files:
pkg/config/import_test.go
📚 Learning: 2024-11-19T23:00:45.899Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 795
File: internal/exec/stack_processor_utils.go:378-386
Timestamp: 2024-11-19T23:00:45.899Z
Learning: In the `ProcessYAMLConfigFile` function within `internal/exec/stack_processor_utils.go`, directory traversal in stack imports is acceptable and should not be restricted.
Applied to files:
pkg/config/import_test.go
📚 Learning: 2025-05-23T19:51:47.091Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 1255
File: cmd/describe_affected_test.go:15-15
Timestamp: 2025-05-23T19:51:47.091Z
Learning: In the atmos codebase, t.Chdir() is a valid method that can be called on *testing.T objects. This functionality is implemented through custom testing framework extensions and is used consistently throughout the test suite for changing working directories during tests.
Applied to files:
pkg/config/import_test.go
📚 Learning: 2024-10-28T01:51:30.811Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 727
File: internal/exec/terraform_clean.go:329-332
Timestamp: 2024-10-28T01:51:30.811Z
Learning: In the Atmos Go code, when deleting directories or handling file paths (e.g., in `terraform_clean.go`), always resolve the absolute path using `filepath.Abs` and use the logger `u.LogWarning` for logging messages instead of using `fmt.Printf`.
Applied to files:
pkg/config/import_test.go
📚 Learning: 2025-12-13T06:10:25.156Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1686
File: internal/exec/workflow_utils.go:0-0
Timestamp: 2025-12-13T06:10:25.156Z
Learning: Atmos workflows: In internal/exec/workflow_utils.go ExecuteWorkflow, non-identity steps intentionally use baseWorkflowEnv, which is constructed from the parent environment with PATH modifications for the toolchain. Avoid appending os.Environ() again; prefer documenting this behavior and testing that standard environment variables are preserved.
Applied to files:
pkg/config/import_test.go
📚 Learning: 2025-02-24T22:46:39.744Z
Learnt from: haitham911
Repo: cloudposse/atmos PR: 1085
File: pkg/config/load.go:219-221
Timestamp: 2025-02-24T22:46:39.744Z
Learning: In the Atmos configuration system, imports from atmos.d are optional. When import errors occur, they should be logged at debug level and the process should continue, rather than failing completely.
Applied to files:
pkg/config/import_test.go
📚 Learning: 2025-01-25T15:21:40.413Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 808
File: examples/demo-atmos-cli-imports/atmos.yaml:8-8
Timestamp: 2025-01-25T15:21:40.413Z
Learning: In Atmos, when a directory is specified for configuration loading (e.g., in the `import` section of atmos.yaml), all files within that directory should be treated as Atmos configurations. Do not suggest restricting file extensions in directory-based glob patterns.
Applied to files:
pkg/config/import_test.go
📚 Learning: 2025-04-03T11:00:27.980Z
Learnt from: haitham911
Repo: cloudposse/atmos PR: 1091
File: pkg/config/imports.go:0-0
Timestamp: 2025-04-03T11:00:27.980Z
Learning: The Atmos configuration system intentionally supports both HTTP and HTTPS protocols for remote imports, as confirmed by the project maintainers. This is implemented in the `isRemoteImport` and `processRemoteImport` functions in pkg/config/imports.go.
Applied to files:
docs/prd/import-adapter-registry.md
📚 Learning: 2025-12-13T06:07:34.794Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1686
File: docs/prd/tool-dependencies-integration.md:58-64
Timestamp: 2025-12-13T06:07:34.794Z
Learning: For docs in the cloudposse/atmos repository under docs/prd/, markdownlint issues MD040, MD010, and MD034 should be deferred to a separate documentation cleanup commit and must not block the current PR. If needed, address these issues in a follow-up PR dedicated to documentation improvements.
Applied to files:
docs/prd/import-adapter-registry.md
🧬 Code graph analysis (6)
pkg/config/adapters/gogetter_adapter.go (3)
pkg/config/imports.go (3)
ResolvedPaths(64-68)REMOTE(57-57)ProcessImportsFromAdapter(121-123)pkg/logger/log.go (1)
Debug(24-26)pkg/downloader/file_downloader_interface.go (1)
ClientModeFile(53-53)
pkg/config/import_adapter.go (1)
pkg/config/imports.go (1)
ResolvedPaths(64-68)
pkg/config/imports.go (3)
pkg/filesystem/os_filesystem.go (1)
NewOSFileSystem(15-17)pkg/logger/log.go (2)
Trace(14-16)Debug(24-26)pkg/config/import_adapter_registry.go (1)
FindImportAdapter(76-104)
pkg/config/adapters/adapters_test.go (6)
pkg/config/adapters/gogetter_adapter.go (1)
GoGetterAdapter(19-19)pkg/config/adapters/local_adapter.go (1)
LocalAdapter(18-18)errors/errors.go (2)
ErrImportPathRequired(519-519)ErrResolveLocal(517-517)pkg/config/imports.go (2)
LOCAL(56-56)ADAPTER(58-58)pkg/config/adapters/mock_adapter.go (4)
MockAdapter(25-29)ClearMockData(51-53)SetMockData(45-47)GetGlobalMockAdapter(39-41)pkg/config/import_adapter_registry.go (1)
FindImportAdapter(76-104)
pkg/config/adapters/mock_adapter.go (2)
pkg/config/imports.go (3)
ResolvedPaths(64-68)ProcessImportsFromAdapter(121-123)ADAPTER(58-58)pkg/perf/perf.go (1)
Track(121-138)
pkg/config/import_test_helpers_test.go (4)
pkg/config/imports.go (4)
ResolvedPaths(64-68)SearchAtmosConfig(166-189)LOCAL(56-56)ProcessImportsFromAdapter(121-123)errors/errors.go (2)
ErrImportPathRequired(519-519)ErrResolveLocal(517-517)pkg/logger/log.go (2)
Trace(14-16)Debug(24-26)pkg/config/import_adapter_registry.go (1)
ResetImportAdapterRegistry(119-127)
🪛 LanguageTool
docs/prd/import-adapter-registry.md
[typographical] ~12-~12: To join two clauses or introduce examples, consider using an em dash.
Context: ...ts many schemes: - http://, https:// - HTTP(S) downloads - git:: - Git reposi...
(DASH_RULE)
[typographical] ~58-~58: Consider using a typographic opening quote here.
Context: ...URL scheme the adapter handles (without "://") | | FR-1.3 | The Resolve() metho...
(EN_QUOTES)
[typographical] ~58-~58: Consider using a typographic opening quote here.
Context: ...scheme the adapter handles (without "://") | | FR-1.3 | The Resolve() method SH...
(EN_QUOTES)
[typographical] ~83-~83: Consider using a typographic opening quote here.
Context: ...xtract the scheme from paths containing "://" | | FR-3.5 | The extractScheme() ...
(EN_QUOTES)
[typographical] ~83-~83: Consider using a typographic opening quote here.
Context: ...ct the scheme from paths containing "://" | | FR-3.5 | The extractScheme() func...
(EN_QUOTES)
[grammar] ~125-~125: Please add a punctuation mark at the end of paragraph.
Context: ...nt transformation before becoming valid YAML ### Architecture Overview ```text ┌──...
(PUNCTUATION_PARAGRAPH_END)
[style] ~744-~744: This word has been used in one of the immediately preceding sentences. Using a synonym could make your text more interesting to read, unless the repetition is intentional.
Context: ...port adapters are only for schemes that need special processing (content transformat...
(EN_REPEATEDWORDS_NEED)
[grammar] ~759-~759: Please add a punctuation mark at the end of paragraph.
Context: ...::, oci://, etc.) 3. Fall back to local filesystem ## References - [Command Registry Pat...
(PUNCTUATION_PARAGRAPH_END)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Acceptance Tests (linux)
- GitHub Check: Acceptance Tests (macos)
- GitHub Check: Acceptance Tests (windows)
- GitHub Check: Summary
🔇 Additional comments (22)
cmd/root.go (1)
31-32: LGTM! Adapter registration follows established pattern.The blank import correctly triggers side-effect registration of built-in adapters. This follows the same pattern used elsewhere in the file for command registration (lines 51-63).
pkg/config/adapters/init.go (2)
9-12: Lazy initialization pattern correctly avoids circular dependencies.Setting the initializer function pointer in
init()allows adapters to be registered on first use, preventing import cycle issues between config and adapters packages.
14-24: Adapter registration logic is clean and straightforward.The three built-in adapters (GoGetter, Mock, Local) are registered in the correct order with LocalAdapter as the default fallback.
pkg/config/import_adapter.go (1)
17-50: Well-designed adapter interface with clear contracts.The
ImportAdapterinterface provides a clean abstraction for pluggable import sources. Documentation clearly explains parameters, return values, and adapter responsibilities for nested imports.pkg/config/import_test.go (1)
20-20: Test adapter setup correctly initializes the registry.The
setupTestAdapters()calls ensure tests have a clean adapter registry state, which is essential for deterministic test behavior with the new adapter system.Also applies to: 93-93, 228-228
pkg/config/adapters/gogetter_adapter.go (1)
59-59: Performance tracking correctly applied.Both
ResolveanddownloadRemoteConfighave properdefer perf.Track()calls as required for performance monitoring.Also applies to: 105-105
pkg/config/adapters/local_adapter.go (3)
35-35: Performance tracking correctly implemented.The
Resolvemethod includes proper performance tracking as required by coding guidelines.
38-39: Static error usage follows guidelines.Returns
ErrImportPathRequiredandErrResolveLocalfrom the errors package, correctly following the error handling conventions.Also applies to: 59-59
70-73: Error handling strategy is appropriate for glob patterns.Logging errors and continuing is reasonable when processing multiple files from glob patterns. Individual file failures shouldn't block the entire import resolution. The errors are logged at debug level for troubleshooting.
Also applies to: 91-94
pkg/config/import_test_helpers_test.go (2)
14-94: Test adapter duplication is intentional to avoid circular imports.The comment clearly explains why
testLocalAdapterduplicates the logic fromadapters.LocalAdapter. This is a common and acceptable pattern for test helpers that need to avoid circular package dependencies.
96-101: Registry reset provides clean test state.
setupTestAdapters()ensures each test starts with a clean adapter registry, which is essential for test isolation and deterministic behavior.pkg/config/imports.go (4)
56-58: New ADAPTER import type extends the system correctly.Adding the
ADAPTERconstant to theimportTypesenum enables the registry-based resolution path while maintaining backward compatibility withLOCALandREMOTEtypes.
63-68: Exported fields enable adapter implementations.Making
FilePath,ImportPaths, andImportTypepublic is necessary for adapters to constructResolvedPathsentries. This is a clean API boundary for the adapter system.
119-123: Public adapter API follows good encapsulation.
ProcessImportsFromAdapterprovides a clean entry point for adapters to process nested imports while keepingprocessImportsinternal. This maintains proper separation between public and private interfaces.
151-159: Adapter-based resolution unifies import handling.The new approach using
FindImportAdapterandadapter.Resolvecleanly replaces the previous conditional logic with a registry pattern. Error handling continues to follow the "log and continue" pattern, which is appropriate for optional imports.pkg/config/import_adapter_registry.go (2)
117-127: LGTM!The test-only reset function properly clears all state including the sync.Once guard. The write lock ensures thread safety during reset.
76-104: LGTM!The prefix-matching logic with case-insensitive comparison is correct. Documentation appropriately warns about registration order for overlapping schemes.
pkg/config/adapters/adapters_test.go (2)
198-225: LGTM!Well-structured table-driven test covering multiple routing scenarios. Good use of the test pattern per coding guidelines.
3-14: Add blank lines to separate import groups.Per coding guidelines, import groups should be separated by blank lines.
🔎 Proposed fix
import ( "context" "os" "path/filepath" "testing" + "github.com/spf13/viper" "github.com/stretchr/testify/assert" + errUtils "github.com/cloudposse/atmos/errors" "github.com/cloudposse/atmos/pkg/config" )Based on learnings, import organization is enforced across all Go files.
⛔ Skipped due to learnings
Learnt from: CR Repo: cloudposse/atmos PR: 0 File: CLAUDE.md:0-0 Timestamp: 2025-12-16T18:20:55.630Z Learning: Applies to **/*.go : Organize imports in three groups separated by blank lines, sorted alphabetically: 1) Go stdlib, 2) 3rd-party (NOT cloudposse/atmos), 3) Atmos packages; maintain aliases: cfg, log, u, errUtilsLearnt from: osterman Repo: cloudposse/atmos PR: 808 File: pkg/config/config.go:478-483 Timestamp: 2024-12-02T21:26:32.337Z Learning: In the 'atmos' project, when reviewing Go code like `pkg/config/config.go`, avoid suggesting file size checks after downloading remote configs if such checks aren't implemented elsewhere in the codebase.Learnt from: aknysh Repo: cloudposse/atmos PR: 1363 File: internal/exec/template_utils.go:18-18 Timestamp: 2025-07-05T20:59:02.914Z Learning: In the Atmos project, gomplate v4 is imported with a blank import (`_ "github.com/hairyhenderson/gomplate/v4"`) alongside v3 imports to resolve AWS SDK version conflicts. V3 uses older AWS SDK versions that conflict with newer AWS modules used by Atmos. A full migration to v4 requires extensive refactoring due to API changes and should be handled in a separate PR.Learnt from: CR Repo: cloudposse/atmos PR: 0 File: CLAUDE.md:0-0 Timestamp: 2025-12-16T18:20:55.630Z Learning: Applies to **/*.go : Keep files small and focused (<600 lines); one cmd/impl per file; co-locate tests; never use //revive:disable:file-length-limitLearnt from: CR Repo: cloudposse/atmos PR: 0 File: .cursor/rules/atmos-rules.mdc:0-0 Timestamp: 2025-11-24T17:35:37.209Z Learning: Applies to **/*.go : Follow standard Go coding style: use `gofmt` and `goimports` to format code, prefer short descriptive variable names, use kebab-case for command-line flags, and snake_case for environment variablesLearnt from: samtholiya Repo: cloudposse/atmos PR: 1466 File: toolchain/http_client_test.go:3-10 Timestamp: 2025-09-10T21:17:55.273Z Learning: In the cloudposse/atmos repository, imports should never be changed as per samtholiya's coding guidelines.Learnt from: CR Repo: cloudposse/atmos PR: 0 File: CLAUDE.md:0-0 Timestamp: 2025-12-16T18:20:55.630Z Learning: Applies to **/*.go : Add `defer perf.Track(atmosConfig, "pkg.FuncName")()` + blank line to all public functions for performance tracking; use nil if no atmosConfig paramLearnt from: CR Repo: cloudposse/atmos PR: 0 File: CLAUDE.md:0-0 Timestamp: 2025-12-16T18:20:55.630Z Learning: Applies to **/*_test.go : Test behavior, not implementation; never test stub functions; avoid tautological tests; make code testable via DI; no coverage theater; remove always-skipped tests; use errors.Is() for error checkingLearnt from: CR Repo: cloudposse/atmos PR: 0 File: .cursor/rules/atmos-rules.mdc:0-0 Timestamp: 2025-11-24T17:35:37.209Z Learning: Applies to **/*.go : Document complex logic with inline comments in Go codeLearnt from: osterman Repo: cloudposse/atmos PR: 740 File: cmd/cmd_utils.go:340-359 Timestamp: 2024-10-23T21:36:40.262Z Learning: In the Go codebase for Atmos, when reviewing functions like `checkAtmosConfig` in `cmd/cmd_utils.go`, avoid suggesting refactoring to return errors instead of calling `os.Exit` if such changes would significantly increase the scope due to the need to update multiple call sites.Learnt from: osterman Repo: cloudposse/atmos PR: 1697 File: internal/exec/oci_utils.go:0-0 Timestamp: 2025-11-08T19:56:18.660Z Learning: In the Atmos codebase, when a function receives an `*schema.AtmosConfiguration` parameter, it should read configuration values from `atmosConfig.Settings` fields rather than using direct `os.Getenv()` or `viper.GetString()` calls. The Atmos pattern is: viper.BindEnv in cmd/root.go binds environment variables → Viper unmarshals into atmosConfig.Settings via mapstructure → business logic reads from the Settings struct. This provides centralized config management, respects precedence, and enables testability. Example: `atmosConfig.Settings.AtmosGithubToken` instead of `os.Getenv("ATMOS_GITHUB_TOKEN")` in functions like `getGHCRAuth` in internal/exec/oci_utils.go.Learnt from: aknysh Repo: cloudposse/atmos PR: 1352 File: pkg/store/artifactory_store_test.go:108-113 Timestamp: 2025-08-15T14:43:41.030Z Learning: In test files for the atmos project, it's acceptable to ignore errors from os.Setenv/Unsetenv operations during test environment setup and teardown, as these are controlled test scenarios.Learnt from: osterman Repo: cloudposse/atmos PR: 1686 File: toolchain/which_test.go:166-223 Timestamp: 2025-11-11T03:47:59.576Z Learning: In the cloudposse/atmos repo, tests that manipulate environment variables should use testing.T.Setenv for automatic setup/teardown instead of os.Setenv/Unsetenv.Learnt from: osterman Repo: cloudposse/atmos PR: 1202 File: pkg/utils/yaml_func_exec.go:104-104 Timestamp: 2025-04-23T15:02:50.246Z Learning: In the Atmos codebase, direct calls to `os.Getenv` should be avoided. Instead, use `viper.BindEnv` for environment variable access. This provides a consistent approach to configuration management across the codebase.Learnt from: samtholiya Repo: cloudposse/atmos PR: 1255 File: cmd/describe_affected_test.go:15-15 Timestamp: 2025-05-23T19:51:47.091Z Learning: The atmos codebase has a custom extension to *testing.T that provides a Chdir method, allowing test functions to call t.Chdir() to change working directories during tests. This is used consistently across test files in the codebase.Learnt from: osterman Repo: cloudposse/atmos PR: 1686 File: toolchain/add_test.go:67-77 Timestamp: 2025-11-11T03:47:45.878Z Learning: In the cloudposse/atmos codebase, tests should prefer t.Setenv for environment variable setup/teardown instead of os.Setenv/Unsetenv to ensure test-scoped isolation.Learnt from: osterman Repo: cloudposse/atmos PR: 1433 File: cmd/theme_list.go:33-36 Timestamp: 2025-08-29T20:57:35.423Z Learning: In the Atmos codebase, avoid using viper.SetEnvPrefix("ATMOS") with viper.AutomaticEnv() because canonical environment variable names are not exclusive to Atmos and could cause conflicts. Instead, use selective environment variable binding through the setEnv function in pkg/config/load.go with bindEnv(v, "config.key", "ENV_VAR_NAME") for specific environment variables.Learnt from: osterman Repo: cloudposse/atmos PR: 727 File: internal/exec/terraform_clean.go:329-332 Timestamp: 2024-10-28T01:51:30.811Z Learning: In the Atmos Go code, when deleting directories or handling file paths (e.g., in `terraform_clean.go`), always resolve the absolute path using `filepath.Abs` and use the logger `u.LogWarning` for logging messages instead of using `fmt.Printf`.Learnt from: Listener430 Repo: cloudposse/atmos PR: 844 File: cmd/helmfile.go:37-37 Timestamp: 2024-12-11T18:40:12.808Z Learning: In the atmos project, `cliConfig` is initialized within the `cmd` package in `root.go` and can be used in other command files.pkg/config/adapters/mock_adapter.go (2)
60-117: LGTM!The Resolve implementation correctly handles multiple test scenarios with appropriate switch-case logic. Good separation of concerns for different mock paths.
3-13: Add blank line to separate import groups.Per coding guidelines, import groups should be separated by blank lines.
🔎 Proposed fix
import ( "context" "fmt" "os" "path/filepath" "strings" "time" + "github.com/cloudposse/atmos/pkg/config" "github.com/cloudposse/atmos/pkg/perf" )Based on learnings, import organization is enforced across all Go files.
⛔ Skipped due to learnings
Learnt from: CR Repo: cloudposse/atmos PR: 0 File: CLAUDE.md:0-0 Timestamp: 2025-12-16T18:20:55.630Z Learning: Applies to **/*.go : Organize imports in three groups separated by blank lines, sorted alphabetically: 1) Go stdlib, 2) 3rd-party (NOT cloudposse/atmos), 3) Atmos packages; maintain aliases: cfg, log, u, errUtilsLearnt from: osterman Repo: cloudposse/atmos PR: 808 File: pkg/config/config.go:478-483 Timestamp: 2024-12-02T21:26:32.337Z Learning: In the 'atmos' project, when reviewing Go code like `pkg/config/config.go`, avoid suggesting file size checks after downloading remote configs if such checks aren't implemented elsewhere in the codebase.Learnt from: CR Repo: cloudposse/atmos PR: 0 File: CLAUDE.md:0-0 Timestamp: 2025-12-16T18:20:55.630Z Learning: Applies to **/*.go : Add `defer perf.Track(atmosConfig, "pkg.FuncName")()` + blank line to all public functions for performance tracking; use nil if no atmosConfig paramLearnt from: aknysh Repo: cloudposse/atmos PR: 1363 File: internal/exec/template_utils.go:18-18 Timestamp: 2025-07-05T20:59:02.914Z Learning: In the Atmos project, gomplate v4 is imported with a blank import (`_ "github.com/hairyhenderson/gomplate/v4"`) alongside v3 imports to resolve AWS SDK version conflicts. V3 uses older AWS SDK versions that conflict with newer AWS modules used by Atmos. A full migration to v4 requires extensive refactoring due to API changes and should be handled in a separate PR.Learnt from: samtholiya Repo: cloudposse/atmos PR: 1466 File: toolchain/http_client_test.go:3-10 Timestamp: 2025-09-10T21:17:55.273Z Learning: In the cloudposse/atmos repository, imports should never be changed as per samtholiya's coding guidelines.Learnt from: CR Repo: cloudposse/atmos PR: 0 File: CLAUDE.md:0-0 Timestamp: 2025-12-16T18:20:55.630Z Learning: Applies to **/*.go : Keep files small and focused (<600 lines); one cmd/impl per file; co-locate tests; never use //revive:disable:file-length-limitLearnt from: osterman Repo: cloudposse/atmos PR: 740 File: cmd/cmd_utils.go:340-359 Timestamp: 2024-10-23T21:36:40.262Z Learning: In the Go codebase for Atmos, when reviewing functions like `checkAtmosConfig` in `cmd/cmd_utils.go`, avoid suggesting refactoring to return errors instead of calling `os.Exit` if such changes would significantly increase the scope due to the need to update multiple call sites.Learnt from: CR Repo: cloudposse/atmos PR: 0 File: .cursor/rules/atmos-rules.mdc:0-0 Timestamp: 2025-11-24T17:35:37.209Z Learning: Applies to **/*.go : Follow standard Go coding style: use `gofmt` and `goimports` to format code, prefer short descriptive variable names, use kebab-case for command-line flags, and snake_case for environment variablesLearnt from: Benbentwo Repo: cloudposse/atmos PR: 1475 File: tests/fixtures/scenarios/atmos-auth/stacks/deploy/nonprod.yaml:3-4 Timestamp: 2025-09-24T20:45:40.401Z Learning: In Atmos stack files, the correct syntax for importing other stack files is `import:` (singular), not `imports:` (plural). All stack files in the Atmos codebase consistently use `import:` followed by a list of paths to import.Learnt from: osterman Repo: cloudposse/atmos PR: 1697 File: internal/exec/oci_utils.go:0-0 Timestamp: 2025-11-08T19:56:18.660Z Learning: In the Atmos codebase, when a function receives an `*schema.AtmosConfiguration` parameter, it should read configuration values from `atmosConfig.Settings` fields rather than using direct `os.Getenv()` or `viper.GetString()` calls. The Atmos pattern is: viper.BindEnv in cmd/root.go binds environment variables → Viper unmarshals into atmosConfig.Settings via mapstructure → business logic reads from the Settings struct. This provides centralized config management, respects precedence, and enables testability. Example: `atmosConfig.Settings.AtmosGithubToken` instead of `os.Getenv("ATMOS_GITHUB_TOKEN")` in functions like `getGHCRAuth` in internal/exec/oci_utils.go.Learnt from: osterman Repo: cloudposse/atmos PR: 1752 File: pkg/profile/list/formatter_table.go:27-29 Timestamp: 2025-11-09T19:06:58.470Z Learning: In the cloudposse/atmos repository, performance tracking with `defer perf.Track()` is enforced on all functions via linting, including high-frequency utility functions, formatters, and renderers. This is a repository-wide policy to maintain consistency and avoid making case-by-case judgment calls about which functions should have profiling.Learnt from: aknysh Repo: cloudposse/atmos PR: 1821 File: pkg/merge/deferred.go:34-48 Timestamp: 2025-11-30T04:16:24.155Z Learning: In the cloudposse/atmos repository, the `defer perf.Track()` guideline applies to functions that perform meaningful work (I/O, computation, external calls), but explicitly excludes trivial accessors/mutators (e.g., simple getters, setters with single integer increments, string joins, or map appends) where the tracking overhead would exceed the actual method cost and provide no actionable performance data. Hot-path methods called in tight loops should especially avoid perf.Track() if they perform only trivial operations.Learnt from: haitham911 Repo: cloudposse/atmos PR: 736 File: pkg/config/const.go:6-6 Timestamp: 2024-10-20T13:12:46.499Z Learning: In `cmd/cmd_utils.go`, it's acceptable to have hardcoded references to `atmos.yaml` in logs, and it's not necessary to update them to use the `CliConfigFileName` constant.Learnt from: Listener430 Repo: cloudposse/atmos PR: 844 File: cmd/helmfile.go:37-37 Timestamp: 2024-12-11T18:40:12.808Z Learning: In the atmos project, `cliConfig` is initialized within the `cmd` package in `root.go` and can be used in other command files.Learnt from: osterman Repo: cloudposse/atmos PR: 727 File: internal/exec/terraform_clean.go:329-332 Timestamp: 2024-10-28T01:51:30.811Z Learning: In the Atmos Go code, when deleting directories or handling file paths (e.g., in `terraform_clean.go`), always resolve the absolute path using `filepath.Abs` and use the logger `u.LogWarning` for logging messages instead of using `fmt.Printf`.Learnt from: aknysh Repo: cloudposse/atmos PR: 1622 File: pkg/perf/perf.go:140-184 Timestamp: 2025-10-13T18:13:54.020Z Learning: In pkg/perf/perf.go, the `trackWithSimpleStack` function intentionally skips ownership checks at call stack depth > 1 to avoid expensive `getGoroutineID()` calls on every nested function. This is a performance optimization for the common single-goroutine execution case (most Atmos commands), accepting the rare edge case of potential metric corruption if multi-goroutine execution occurs at depth > 1. The ~19× performance improvement justifies this trade-off.docs/prd/import-adapter-registry.md (1)
118-164: LGTM!The two-layer architecture design is sound. Preserving go-getter for standard downloads while adding transformation adapters for special cases is a pragmatic approach that maintains backward compatibility.
|
Warning Changelog Entry RequiredThis PR is labeled Action needed: Add a new blog post in Example filename: Alternatively: If this change doesn't require a changelog entry, remove the |
- Update PRD documentation to match actual implementation: - Scheme() → Schemes() returning []string - Resolve() with 6 parameters (added currentDepth, maxDepth) - Add ErrDownloadRemoteConfig sentinel error to errors/errors.go - Use static error wrapping in gogetter_adapter.go 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
pkg/config/adapters/gogetter_adapter.go (1)
113-117: Consider documenting the fallback timeout implications.The 5-second default timeout is defensive but might be insufficient for large configs or slow networks. Since callers should provide proper contexts, consider documenting this fallback behavior or verifying that all call sites provide appropriate contexts.
Want me to generate a script to verify all Resolve() call sites provide proper contexts?
docs/prd/import-adapter-registry.md (1)
1-779: Strong PRD with minor optional improvements.The PRD is comprehensive and well-structured. Static analysis suggests some minor typographical refinements (em dashes, quote styles) that could be addressed in a future documentation polish pass if desired.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
docs/prd/import-adapter-registry.md(1 hunks)errors/errors.go(1 hunks)pkg/config/adapters/gogetter_adapter.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
docs/prd/**/*.md
📄 CodeRabbit inference engine (CLAUDE.md)
All Product Requirement Documents (PRDs) MUST be placed in docs/prd/ with kebab-case filenames
Files:
docs/prd/import-adapter-registry.md
**/*.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
**/*.go: Use Viper for managing configuration, environment variables, and flags in CLI commands
Use interfaces for external dependencies to facilitate mocking and consider using testify/mock for creating mock implementations
All code must pass golangci-lint checks
Follow Go's error handling idioms: use meaningful error messages, wrap errors with context usingfmt.Errorf("context: %w", err), and consider using custom error types for domain-specific errors
Follow standard Go coding style: usegofmtandgoimportsto format code, prefer short descriptive variable names, use kebab-case for command-line flags, and snake_case for environment variables
Document all exported functions, types, and methods following Go's documentation conventions
Document complex logic with inline comments in Go code
Support configuration via files, environment variables, and flags following the precedence order: flags > environment variables > config file > defaults
Provide clear error messages to users, include troubleshooting hints when appropriate, and log detailed errors for debugging
**/*.go: NEVER use fmt.Fprintf(os.Stdout/Stderr) or fmt.Println(); use data.* or ui.* functions instead
All comments must end with periods (enforced by godot linter)
Organize imports in three groups separated by blank lines, sorted alphabetically: 1) Go stdlib, 2) 3rd-party (NOT cloudposse/atmos), 3) Atmos packages; maintain aliases: cfg, log, u, errUtils
Adddefer perf.Track(atmosConfig, "pkg.FuncName")()+ blank line to all public functions for performance tracking; use nil if no atmosConfig param
All errors MUST be wrapped using static errors defined in errors/errors.go; use errors.Join for combining multiple errors; use fmt.Errorf with %w for adding string context; use error builder for complex errors; use errors.Is() for error checking; NEVER use dynamic errors directly
Use go.uber.org/mock/mockgen with //go:generate directives for mock generation; never create manual mocks
Keep files small...
Files:
errors/errors.gopkg/config/adapters/gogetter_adapter.go
🧠 Learnings (17)
📓 Common learnings
Learnt from: aknysh
Repo: cloudposse/atmos PR: 768
File: internal/exec/vendor_model_component.go:3-20
Timestamp: 2024-11-18T13:59:10.824Z
Learning: When replacing significant dependencies like `go-getter` that require extensive changes, prefer to address them in separate PRs.
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T18:20:55.630Z
Learning: Define interfaces for all major functionality and use dependency injection for testability; generate mocks with go.uber.org/mock/mockgen
📚 Learning: 2025-04-03T11:00:27.980Z
Learnt from: haitham911
Repo: cloudposse/atmos PR: 1091
File: pkg/config/imports.go:0-0
Timestamp: 2025-04-03T11:00:27.980Z
Learning: The Atmos configuration system intentionally supports both HTTP and HTTPS protocols for remote imports, as confirmed by the project maintainers. This is implemented in the `isRemoteImport` and `processRemoteImport` functions in pkg/config/imports.go.
Applied to files:
docs/prd/import-adapter-registry.md
📚 Learning: 2025-12-16T18:20:55.630Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T18:20:55.630Z
Learning: Applies to **/*_test.go : Prefer unit tests with mocks over integration tests; use interfaces + dependency injection for testability; generate mocks with go.uber.org/mock/mockgen; use table-driven tests; target >80% coverage
Applied to files:
docs/prd/import-adapter-registry.md
📚 Learning: 2025-12-16T18:20:55.630Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T18:20:55.630Z
Learning: Define interfaces for all major functionality and use dependency injection for testability; generate mocks with go.uber.org/mock/mockgen
Applied to files:
docs/prd/import-adapter-registry.md
📚 Learning: 2025-12-13T06:07:34.794Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1686
File: docs/prd/tool-dependencies-integration.md:58-64
Timestamp: 2025-12-13T06:07:34.794Z
Learning: For docs in the cloudposse/atmos repository under docs/prd/, markdownlint issues MD040, MD010, and MD034 should be deferred to a separate documentation cleanup commit and must not block the current PR. If needed, address these issues in a follow-up PR dedicated to documentation improvements.
Applied to files:
docs/prd/import-adapter-registry.md
📚 Learning: 2025-10-02T19:17:51.630Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1504
File: pkg/profiler/profiler.go:20-31
Timestamp: 2025-10-02T19:17:51.630Z
Learning: In pkg/profiler/profiler.go, profiler-specific errors (ErrUnsupportedProfileType, ErrStartCPUProfile, ErrStartTraceProfile, ErrCreateProfileFile) must remain local and cannot be moved to errors/errors.go due to an import cycle: pkg/profiler → errors → pkg/schema → pkg/profiler. This is a valid exception to the centralized errors policy.
Applied to files:
errors/errors.go
📚 Learning: 2025-12-16T18:20:55.630Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T18:20:55.630Z
Learning: Applies to **/*.go : All errors MUST be wrapped using static errors defined in errors/errors.go; use errors.Join for combining multiple errors; use fmt.Errorf with %w for adding string context; use error builder for complex errors; use errors.Is() for error checking; NEVER use dynamic errors directly
Applied to files:
errors/errors.gopkg/config/adapters/gogetter_adapter.go
📚 Learning: 2024-11-18T13:59:10.824Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 768
File: internal/exec/vendor_model_component.go:3-20
Timestamp: 2024-11-18T13:59:10.824Z
Learning: When replacing significant dependencies like `go-getter` that require extensive changes, prefer to address them in separate PRs.
Applied to files:
pkg/config/adapters/gogetter_adapter.go
📚 Learning: 2024-12-02T21:26:32.337Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 808
File: pkg/config/config.go:478-483
Timestamp: 2024-12-02T21:26:32.337Z
Learning: In the 'atmos' project, when reviewing Go code like `pkg/config/config.go`, avoid suggesting file size checks after downloading remote configs if such checks aren't implemented elsewhere in the codebase.
Applied to files:
pkg/config/adapters/gogetter_adapter.go
📚 Learning: 2025-11-24T17:35:37.209Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-11-24T17:35:37.209Z
Learning: Applies to **/*.go : Follow Go's error handling idioms: use meaningful error messages, wrap errors with context using `fmt.Errorf("context: %w", err)`, and consider using custom error types for domain-specific errors
Applied to files:
pkg/config/adapters/gogetter_adapter.go
📚 Learning: 2024-10-23T21:36:40.262Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 740
File: cmd/cmd_utils.go:340-359
Timestamp: 2024-10-23T21:36:40.262Z
Learning: In the Go codebase for Atmos, when reviewing functions like `checkAtmosConfig` in `cmd/cmd_utils.go`, avoid suggesting refactoring to return errors instead of calling `os.Exit` if such changes would significantly increase the scope due to the need to update multiple call sites.
Applied to files:
pkg/config/adapters/gogetter_adapter.go
📚 Learning: 2025-09-10T22:38:42.212Z
Learnt from: Benbentwo
Repo: cloudposse/atmos PR: 1475
File: pkg/auth/identities/aws/user.go:141-145
Timestamp: 2025-09-10T22:38:42.212Z
Learning: The user confirmed that the errors package has an error string wrapping format, contradicting the previous learning about ErrWrappingFormat being invalid. The current usage of fmt.Errorf(errUtils.ErrWrappingFormat, errUtils.ErrAuthAwsFileManagerFailed, err) appears to be the correct pattern.
Applied to files:
pkg/config/adapters/gogetter_adapter.go
📚 Learning: 2025-11-24T17:35:37.209Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-11-24T17:35:37.209Z
Learning: Applies to **/*.go : Provide clear error messages to users, include troubleshooting hints when appropriate, and log detailed errors for debugging
Applied to files:
pkg/config/adapters/gogetter_adapter.go
📚 Learning: 2025-12-13T06:10:13.688Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1686
File: errors/errors.go:184-203
Timestamp: 2025-12-13T06:10:13.688Z
Learning: cloudposse/atmos: For toolchain work, duplicate/unused error sentinels in errors/errors.go should be cleaned up in a separate refactor PR and not block feature PRs; canonical toolchain sentinels live under toolchain/registry with re-exports in toolchain/errors.go.
Applied to files:
pkg/config/adapters/gogetter_adapter.go
📚 Learning: 2025-01-07T20:38:09.618Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 896
File: cmd/editor_config.go:37-40
Timestamp: 2025-01-07T20:38:09.618Z
Learning: Error handling suggestion for `cmd.Help()` in `cmd/editor_config.go` was deferred as the code is planned for future modifications.
Applied to files:
pkg/config/adapters/gogetter_adapter.go
📚 Learning: 2025-10-22T14:55:44.014Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1695
File: pkg/auth/manager.go:169-171
Timestamp: 2025-10-22T14:55:44.014Z
Learning: Go 1.20+ supports multiple %w verbs in fmt.Errorf, which returns an error implementing Unwrap() []error. This is valid and does not panic. Atmos uses Go 1.24.8 and configures errorlint with errorf-multi: true to validate this pattern.
Applied to files:
pkg/config/adapters/gogetter_adapter.go
📚 Learning: 2025-09-10T22:38:42.212Z
Learnt from: Benbentwo
Repo: cloudposse/atmos PR: 1475
File: pkg/auth/identities/aws/user.go:141-145
Timestamp: 2025-09-10T22:38:42.212Z
Learning: ErrWrappingFormat is correctly defined as "%w: %w" in the errors package and is used throughout the codebase to wrap two error types together. The usage fmt.Errorf(errUtils.ErrWrappingFormat, errUtils.ErrAuthAwsFileManagerFailed, err) is the correct pattern when both arguments are error types.
Applied to files:
pkg/config/adapters/gogetter_adapter.go
🧬 Code graph analysis (1)
pkg/config/adapters/gogetter_adapter.go (6)
pkg/config/imports.go (3)
ResolvedPaths(64-68)REMOTE(57-57)ProcessImportsFromAdapter(121-123)pkg/logger/log.go (1)
Debug(24-26)pkg/flags/parser.go (2)
GetStringSlice(129-138)GetString(117-126)pkg/http/client.go (2)
Client(19-22)Get(54-97)pkg/downloader/file_downloader_interface.go (1)
ClientModeFile(53-53)errors/errors.go (1)
ErrDownloadRemoteConfig(523-523)
🪛 LanguageTool
docs/prd/import-adapter-registry.md
[typographical] ~12-~12: To join two clauses or introduce examples, consider using an em dash.
Context: ...ts many schemes: - http://, https:// - HTTP(S) downloads - git:: - Git reposi...
(DASH_RULE)
[typographical] ~58-~58: Consider using a typographic opening quote here.
Context: ...mes/prefixes the adapter handles (e.g., ["http://", "https://"]) | | FR-1.3 | The...
(EN_QUOTES)
[typographical] ~83-~83: Consider using a typographic opening quote here.
Context: ...xtract the scheme from paths containing "://" | | FR-3.5 | The extractScheme() ...
(EN_QUOTES)
[typographical] ~83-~83: Consider using a typographic opening quote here.
Context: ...ct the scheme from paths containing "://" | | FR-3.5 | The extractScheme() func...
(EN_QUOTES)
[grammar] ~125-~125: Please add a punctuation mark at the end of paragraph.
Context: ...nt transformation before becoming valid YAML ### Architecture Overview ```text ┌──...
(PUNCTUATION_PARAGRAPH_END)
[style] ~750-~750: This word has been used in one of the immediately preceding sentences. Using a synonym could make your text more interesting to read, unless the repetition is intentional.
Context: ...port adapters are only for schemes that need special processing (content transformat...
(EN_REPEATEDWORDS_NEED)
[grammar] ~765-~765: Please add a punctuation mark at the end of paragraph.
Context: ...::, oci://, etc.) 3. Fall back to local filesystem ## References - [Command Registry Pat...
(PUNCTUATION_PARAGRAPH_END)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Acceptance Tests (macos)
- GitHub Check: Acceptance Tests (linux)
- GitHub Check: Acceptance Tests (windows)
- GitHub Check: Summary
🔇 Additional comments (5)
errors/errors.go (1)
523-523: LGTM! Error sentinel correctly added.The new
ErrDownloadRemoteConfigsentinel follows the established pattern and integrates cleanly with the adapter's error handling strategy.pkg/config/adapters/gogetter_adapter.go (3)
22-49: LGTM! Comprehensive scheme list.The Schemes() method correctly lists all major go-getter URL schemes. The hardcoded approach is reasonable given go-getter's stable API.
51-102: LGTM! Well-structured import resolution.The Resolve() method correctly handles remote config download, nested imports, and base_path overrides. Error handling and performance tracking are properly implemented.
126-129: LGTM! Error wrapping correctly implemented.The error handling now uses static error sentinels from errors/errors.go with proper wrapping, addressing the previous review feedback.
docs/prd/import-adapter-registry.md (1)
226-268: LGTM! Interface documentation now matches implementation.The
ImportAdapterinterface definition correctly reflects the implemented signatures withSchemes()returning[]stringandResolve()accepting depth parameters. Previous review feedback has been addressed.
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
…gistry Tests were failing because import adapters weren't being registered during test execution. Added setupTestAdapters() calls to 5 test functions to register the testLocalAdapter before running import-related tests. Also fixed lint issues in the adapters package: - Use sentinel error ErrMockImportFailure instead of dynamic error - Use constant mockConfigFilePerms instead of magic number 0o600 - Add nolint comments for argument-limit on interface implementations - Refactor LocalAdapter.Resolve to reduce function length 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1897 +/- ##
==========================================
+ Coverage 73.94% 73.98% +0.04%
==========================================
Files 756 760 +4
Lines 68573 68654 +81
==========================================
+ Hits 50708 50797 +89
+ Misses 14440 14437 -3
+ Partials 3425 3420 -5
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
Warning Release Documentation RequiredThis PR is labeled
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
pkg/config/adapters/mock_adapter.go (1)
3-14: Add blank line to separate import groups.Per coding guidelines, organize imports with blank lines: stdlib, then 3rd-party, then Atmos packages.
🔎 Proposed fix
import ( "context" "fmt" "os" "path/filepath" "strings" "time" + errUtils "github.com/cloudposse/atmos/errors" "github.com/cloudposse/atmos/pkg/config" "github.com/cloudposse/atmos/pkg/perf" )Based on learnings, import organization is enforced across all Go files.
pkg/config/adapters/gogetter_adapter.go (1)
111-112: Consider UUID-based filenames and configurable timeout.A few optional improvements to consider:
Filename uniqueness (line 111): While
UnixNano()provides good uniqueness, using a UUID orcrypto/randwould eliminate any theoretical collision risk.Hardcoded timeout (line 117): The 5-second timeout might be too short for large files or slow connections. Consider making this configurable or using a more generous default.
Nil context pattern (lines 115-119): Accepting and replacing nil context is defensive but unusual. Typically, callers should provide a valid context. This works but diverges from common Go patterns.
🔎 Example with UUID-based filename
+ "github.com/google/uuid"- fileName := fmt.Sprintf("atmos-import-%d.yaml", time.Now().UnixNano()) + fileName := fmt.Sprintf("atmos-import-%s.yaml", uuid.New().String())Also applies to: 115-119
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (9)
cmd/root.goerrors/errors.gopkg/config/adapters/gogetter_adapter.gopkg/config/adapters/local_adapter.gopkg/config/adapters/mock_adapter.gopkg/config/command_merge_core_test.gopkg/config/command_merging_behavior_test.gopkg/config/import_adapter_registry.gopkg/config/import_commands_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- cmd/root.go
- errors/errors.go
- pkg/config/adapters/local_adapter.go
🧰 Additional context used
📓 Path-based instructions (3)
**/*.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
**/*.go: Use Viper for managing configuration, environment variables, and flags in CLI commands
Use interfaces for external dependencies to facilitate mocking and consider using testify/mock for creating mock implementations
All code must pass golangci-lint checks
Follow Go's error handling idioms: use meaningful error messages, wrap errors with context usingfmt.Errorf("context: %w", err), and consider using custom error types for domain-specific errors
Follow standard Go coding style: usegofmtandgoimportsto format code, prefer short descriptive variable names, use kebab-case for command-line flags, and snake_case for environment variables
Document all exported functions, types, and methods following Go's documentation conventions
Document complex logic with inline comments in Go code
Support configuration via files, environment variables, and flags following the precedence order: flags > environment variables > config file > defaults
Provide clear error messages to users, include troubleshooting hints when appropriate, and log detailed errors for debugging
**/*.go: All comments must end with periods (enforced bygodotlinter) in Go code
Organize imports into three groups separated by blank lines, sorted alphabetically: Go stdlib, 3rd-party (NOT cloudposse/atmos), then Atmos packages with maintained aliases (cfg,log,u,errUtils)
All errors MUST be wrapped using static errors defined inerrors/errors.go- useerrors.Joinfor combining errors,fmt.Errorfwith%wfor context, anderrors.Is()for error checking
Never manually create mocks - usego.uber.org/mock/mockgenwith//go:generatedirectives in Go code
Keep files small and focused - under 600 lines with one cmd/impl per file, co-locate tests, never use//revive:disable:file-length-limit
Use colors frompkg/ui/theme/colors.gofor all UI theming in Go code
Code must be compatible with Linux, macOS, and Windows - use SDKs over binaries, usefilepath.Join()instead of h...
Files:
pkg/config/import_commands_test.gopkg/config/command_merging_behavior_test.gopkg/config/command_merge_core_test.gopkg/config/adapters/gogetter_adapter.gopkg/config/adapters/mock_adapter.gopkg/config/import_adapter_registry.go
**/*_test.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
**/*_test.go: Every new feature must include comprehensive unit tests targeting >80% code coverage for all packages
Use table-driven tests for testing multiple scenarios in Go
Include integration tests for command flows and test CLI end-to-end when possible with test fixturesPrefer unit tests with mocks over integration tests - use interfaces and dependency injection for testability, generate mocks with
go.uber.org/mock/mockgen, use table-driven tests, target >80% coverage
Files:
pkg/config/import_commands_test.gopkg/config/command_merging_behavior_test.gopkg/config/command_merge_core_test.go
**/{pkg,internal,cmd}/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Add
defer perf.Track(atmosConfig, "pkg.FuncName")()plus blank line to all public functions, usingnilif no atmosConfig param - exceptions: trivial getters/setters, command constructors, simple factories, functions delegating to tracked functions
Files:
pkg/config/import_commands_test.gopkg/config/command_merging_behavior_test.gopkg/config/command_merge_core_test.gopkg/config/adapters/gogetter_adapter.gopkg/config/adapters/mock_adapter.gopkg/config/import_adapter_registry.go
🧠 Learnings (34)
📓 Common learnings
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T00:55:21.698Z
Learning: Follow multi-provider registry pattern: define interface, implement per provider, register implementations, generate mocks - see `pkg/store/` example
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-11-24T17:35:37.209Z
Learning: Applies to **/*.go : Use interfaces for external dependencies to facilitate mocking and consider using testify/mock for creating mock implementations
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T00:55:21.698Z
Learning: Define interfaces for all major functionality and use dependency injection for testability in Go code
📚 Learning: 2025-11-24T17:35:37.209Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-11-24T17:35:37.209Z
Learning: Applies to **/*_test.go : Include integration tests for command flows and test CLI end-to-end when possible with test fixtures
Applied to files:
pkg/config/import_commands_test.gopkg/config/command_merging_behavior_test.gopkg/config/command_merge_core_test.go
📚 Learning: 2025-11-24T17:35:37.209Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-11-24T17:35:37.209Z
Learning: Applies to **/*_test.go : Use table-driven tests for testing multiple scenarios in Go
Applied to files:
pkg/config/import_commands_test.gopkg/config/command_merging_behavior_test.go
📚 Learning: 2026-01-04T00:55:21.698Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T00:55:21.698Z
Learning: Applies to cmd/**/*_test.go : Always use `cmd.NewTestKit(t)` for cmd tests to auto-clean RootCmd state
Applied to files:
pkg/config/import_commands_test.gopkg/config/command_merging_behavior_test.gopkg/config/command_merge_core_test.go
📚 Learning: 2025-12-21T04:10:29.030Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1891
File: internal/exec/describe_affected.go:468-468
Timestamp: 2025-12-21T04:10:29.030Z
Learning: In Go, package-level declarations (constants, variables, types, and functions) are visible to all files in the same package without imports. During reviews in cloudposse/atmos (and similar Go codebases), before suggesting to declare a new identifier, first check if it already exists in another file of the same package. If it exists, you can avoid adding a new declaration; if not, proceed with a proper package-level declaration.
Applied to files:
pkg/config/import_commands_test.gopkg/config/command_merging_behavior_test.gopkg/config/command_merge_core_test.gopkg/config/adapters/gogetter_adapter.gopkg/config/adapters/mock_adapter.gopkg/config/import_adapter_registry.go
📚 Learning: 2025-12-10T18:32:51.237Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1808
File: cmd/terraform/backend/backend_delete_test.go:9-23
Timestamp: 2025-12-10T18:32:51.237Z
Learning: In cmd subpackages (e.g., cmd/terraform/backend/), tests cannot use cmd.NewTestKit(t) due to Go's test visibility rules (NewTestKit is in a parent package test file). These tests only need TestKit if they execute commands through RootCmd or modify RootCmd state. Structural tests that only verify command structure/flags without touching RootCmd don't require TestKit cleanup.
Applied to files:
pkg/config/command_merging_behavior_test.go
📚 Learning: 2025-05-23T19:51:47.091Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 1255
File: cmd/describe_affected_test.go:15-15
Timestamp: 2025-05-23T19:51:47.091Z
Learning: The atmos codebase has a custom extension to *testing.T that provides a Chdir method, allowing test functions to call t.Chdir() to change working directories during tests. This is used consistently across test files in the codebase.
Applied to files:
pkg/config/command_merging_behavior_test.go
📚 Learning: 2025-11-24T17:35:37.209Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-11-24T17:35:37.209Z
Learning: Applies to cmd/**/*.go : Use Cobra's recommended command structure with a root command and subcommands, implementing each command in a separate file under `cmd/` directory
Applied to files:
pkg/config/command_merging_behavior_test.go
📚 Learning: 2024-11-18T13:59:10.824Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 768
File: internal/exec/vendor_model_component.go:3-20
Timestamp: 2024-11-18T13:59:10.824Z
Learning: When replacing significant dependencies like `go-getter` that require extensive changes, prefer to address them in separate PRs.
Applied to files:
pkg/config/adapters/gogetter_adapter.go
📚 Learning: 2026-01-04T00:55:21.698Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T00:55:21.698Z
Learning: Applies to **/*.go : All errors MUST be wrapped using static errors defined in `errors/errors.go` - use `errors.Join` for combining errors, `fmt.Errorf` with `%w` for context, and `errors.Is()` for error checking
Applied to files:
pkg/config/adapters/gogetter_adapter.go
📚 Learning: 2024-12-02T21:26:32.337Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 808
File: pkg/config/config.go:478-483
Timestamp: 2024-12-02T21:26:32.337Z
Learning: In the 'atmos' project, when reviewing Go code like `pkg/config/config.go`, avoid suggesting file size checks after downloading remote configs if such checks aren't implemented elsewhere in the codebase.
Applied to files:
pkg/config/adapters/gogetter_adapter.gopkg/config/import_adapter_registry.go
📚 Learning: 2025-11-24T17:35:37.209Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-11-24T17:35:37.209Z
Learning: Applies to **/*.go : Follow Go's error handling idioms: use meaningful error messages, wrap errors with context using `fmt.Errorf("context: %w", err)`, and consider using custom error types for domain-specific errors
Applied to files:
pkg/config/adapters/gogetter_adapter.go
📚 Learning: 2024-10-23T21:36:40.262Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 740
File: cmd/cmd_utils.go:340-359
Timestamp: 2024-10-23T21:36:40.262Z
Learning: In the Go codebase for Atmos, when reviewing functions like `checkAtmosConfig` in `cmd/cmd_utils.go`, avoid suggesting refactoring to return errors instead of calling `os.Exit` if such changes would significantly increase the scope due to the need to update multiple call sites.
Applied to files:
pkg/config/adapters/gogetter_adapter.gopkg/config/import_adapter_registry.go
📚 Learning: 2025-09-10T22:38:42.212Z
Learnt from: Benbentwo
Repo: cloudposse/atmos PR: 1475
File: pkg/auth/identities/aws/user.go:141-145
Timestamp: 2025-09-10T22:38:42.212Z
Learning: The user confirmed that the errors package has an error string wrapping format, contradicting the previous learning about ErrWrappingFormat being invalid. The current usage of fmt.Errorf(errUtils.ErrWrappingFormat, errUtils.ErrAuthAwsFileManagerFailed, err) appears to be the correct pattern.
Applied to files:
pkg/config/adapters/gogetter_adapter.go
📚 Learning: 2025-11-24T17:35:37.209Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-11-24T17:35:37.209Z
Learning: Applies to **/*.go : Provide clear error messages to users, include troubleshooting hints when appropriate, and log detailed errors for debugging
Applied to files:
pkg/config/adapters/gogetter_adapter.go
📚 Learning: 2025-12-13T06:10:13.688Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1686
File: errors/errors.go:184-203
Timestamp: 2025-12-13T06:10:13.688Z
Learning: cloudposse/atmos: For toolchain work, duplicate/unused error sentinels in errors/errors.go should be cleaned up in a separate refactor PR and not block feature PRs; canonical toolchain sentinels live under toolchain/registry with re-exports in toolchain/errors.go.
Applied to files:
pkg/config/adapters/gogetter_adapter.gopkg/config/import_adapter_registry.go
📚 Learning: 2025-01-07T20:38:09.618Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 896
File: cmd/editor_config.go:37-40
Timestamp: 2025-01-07T20:38:09.618Z
Learning: Error handling suggestion for `cmd.Help()` in `cmd/editor_config.go` was deferred as the code is planned for future modifications.
Applied to files:
pkg/config/adapters/gogetter_adapter.go
📚 Learning: 2025-10-22T14:55:44.014Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1695
File: pkg/auth/manager.go:169-171
Timestamp: 2025-10-22T14:55:44.014Z
Learning: Go 1.20+ supports multiple %w verbs in fmt.Errorf, which returns an error implementing Unwrap() []error. This is valid and does not panic. Atmos uses Go 1.24.8 and configures errorlint with errorf-multi: true to validate this pattern.
Applied to files:
pkg/config/adapters/gogetter_adapter.go
📚 Learning: 2025-09-10T22:38:42.212Z
Learnt from: Benbentwo
Repo: cloudposse/atmos PR: 1475
File: pkg/auth/identities/aws/user.go:141-145
Timestamp: 2025-09-10T22:38:42.212Z
Learning: ErrWrappingFormat is correctly defined as "%w: %w" in the errors package and is used throughout the codebase to wrap two error types together. The usage fmt.Errorf(errUtils.ErrWrappingFormat, errUtils.ErrAuthAwsFileManagerFailed, err) is the correct pattern when both arguments are error types.
Applied to files:
pkg/config/adapters/gogetter_adapter.go
📚 Learning: 2025-11-24T17:35:37.209Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-11-24T17:35:37.209Z
Learning: Applies to **/*.go : Use interfaces for external dependencies to facilitate mocking and consider using testify/mock for creating mock implementations
Applied to files:
pkg/config/adapters/mock_adapter.go
📚 Learning: 2026-01-04T00:55:21.698Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T00:55:21.698Z
Learning: Applies to **/*_test.go : Prefer unit tests with mocks over integration tests - use interfaces and dependency injection for testability, generate mocks with `go.uber.org/mock/mockgen`, use table-driven tests, target >80% coverage
Applied to files:
pkg/config/adapters/mock_adapter.go
📚 Learning: 2026-01-04T00:55:21.698Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T00:55:21.698Z
Learning: Applies to **/*.go : Never manually create mocks - use `go.uber.org/mock/mockgen` with `//go:generate` directives in Go code
Applied to files:
pkg/config/adapters/mock_adapter.go
📚 Learning: 2026-01-04T00:55:21.698Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T00:55:21.698Z
Learning: Follow multi-provider registry pattern: define interface, implement per provider, register implementations, generate mocks - see `pkg/store/` example
Applied to files:
pkg/config/adapters/mock_adapter.go
📚 Learning: 2026-01-04T00:55:21.698Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T00:55:21.698Z
Learning: Applies to **/*.go : Organize imports into three groups separated by blank lines, sorted alphabetically: Go stdlib, 3rd-party (NOT cloudposse/atmos), then Atmos packages with maintained aliases (`cfg`, `log`, `u`, `errUtils`)
Applied to files:
pkg/config/import_adapter_registry.go
📚 Learning: 2025-07-05T20:59:02.914Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 1363
File: internal/exec/template_utils.go:18-18
Timestamp: 2025-07-05T20:59:02.914Z
Learning: In the Atmos project, gomplate v4 is imported with a blank import (`_ "github.com/hairyhenderson/gomplate/v4"`) alongside v3 imports to resolve AWS SDK version conflicts. V3 uses older AWS SDK versions that conflict with newer AWS modules used by Atmos. A full migration to v4 requires extensive refactoring due to API changes and should be handled in a separate PR.
Applied to files:
pkg/config/import_adapter_registry.go
📚 Learning: 2026-01-04T00:55:21.698Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T00:55:21.698Z
Learning: Applies to **/{pkg,internal,cmd}/**/*.go : Add `defer perf.Track(atmosConfig, "pkg.FuncName")()` plus blank line to all public functions, using `nil` if no atmosConfig param - exceptions: trivial getters/setters, command constructors, simple factories, functions delegating to tracked functions
Applied to files:
pkg/config/import_adapter_registry.go
📚 Learning: 2025-09-10T21:17:55.273Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 1466
File: toolchain/http_client_test.go:3-10
Timestamp: 2025-09-10T21:17:55.273Z
Learning: In the cloudposse/atmos repository, imports should never be changed as per samtholiya's coding guidelines.
Applied to files:
pkg/config/import_adapter_registry.go
📚 Learning: 2025-11-24T17:35:37.209Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-11-24T17:35:37.209Z
Learning: Applies to **/*.go : Follow standard Go coding style: use `gofmt` and `goimports` to format code, prefer short descriptive variable names, use kebab-case for command-line flags, and snake_case for environment variables
Applied to files:
pkg/config/import_adapter_registry.go
📚 Learning: 2025-11-08T19:56:18.660Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1697
File: internal/exec/oci_utils.go:0-0
Timestamp: 2025-11-08T19:56:18.660Z
Learning: In the Atmos codebase, when a function receives an `*schema.AtmosConfiguration` parameter, it should read configuration values from `atmosConfig.Settings` fields rather than using direct `os.Getenv()` or `viper.GetString()` calls. The Atmos pattern is: viper.BindEnv in cmd/root.go binds environment variables → Viper unmarshals into atmosConfig.Settings via mapstructure → business logic reads from the Settings struct. This provides centralized config management, respects precedence, and enables testability. Example: `atmosConfig.Settings.AtmosGithubToken` instead of `os.Getenv("ATMOS_GITHUB_TOKEN")` in functions like `getGHCRAuth` in internal/exec/oci_utils.go.
Applied to files:
pkg/config/import_adapter_registry.go
📚 Learning: 2025-09-24T20:45:40.401Z
Learnt from: Benbentwo
Repo: cloudposse/atmos PR: 1475
File: tests/fixtures/scenarios/atmos-auth/stacks/deploy/nonprod.yaml:3-4
Timestamp: 2025-09-24T20:45:40.401Z
Learning: In Atmos stack files, the correct syntax for importing other stack files is `import:` (singular), not `imports:` (plural). All stack files in the Atmos codebase consistently use `import:` followed by a list of paths to import.
Applied to files:
pkg/config/import_adapter_registry.go
📚 Learning: 2025-11-09T19:06:58.470Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1752
File: pkg/profile/list/formatter_table.go:27-29
Timestamp: 2025-11-09T19:06:58.470Z
Learning: In the cloudposse/atmos repository, performance tracking with `defer perf.Track()` is enforced on all functions via linting, including high-frequency utility functions, formatters, and renderers. This is a repository-wide policy to maintain consistency and avoid making case-by-case judgment calls about which functions should have profiling.
Applied to files:
pkg/config/import_adapter_registry.go
📚 Learning: 2026-01-04T00:55:21.698Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T00:55:21.698Z
Learning: Applies to **/*.go : Keep files small and focused - under 600 lines with one cmd/impl per file, co-locate tests, never use `//revive:disable:file-length-limit`
Applied to files:
pkg/config/import_adapter_registry.go
📚 Learning: 2025-04-04T02:03:23.676Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 1185
File: internal/exec/yaml_func_store.go:26-26
Timestamp: 2025-04-04T02:03:23.676Z
Learning: The Atmos codebase currently uses `log.Fatal` for error handling in multiple places. The maintainers are aware this isn't an ideal pattern (should only be used in main() or init() functions) and plan to address it comprehensively in a separate PR. CodeRabbit should not flag these issues or push for immediate changes until that refactoring is complete.
Applied to files:
pkg/config/import_adapter_registry.go
📚 Learning: 2025-01-17T00:18:57.769Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 944
File: go.mod:206-206
Timestamp: 2025-01-17T00:18:57.769Z
Learning: For indirect dependencies with license compliance issues in the cloudposse/atmos repository, the team prefers to handle them in follow-up PRs rather than blocking the current changes, as these issues often require deeper investigation of the dependency tree.
Applied to files:
pkg/config/import_adapter_registry.go
🧬 Code graph analysis (1)
pkg/config/import_adapter_registry.go (2)
pkg/config/import_adapter.go (1)
ImportAdapter(17-50)pkg/config/imports.go (1)
ResolvedPaths(64-68)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Acceptance Tests (macos)
- GitHub Check: Acceptance Tests (windows)
- GitHub Check: Summary
🔇 Additional comments (17)
pkg/config/command_merge_core_test.go (1)
18-19: LGTM!Proper test adapter initialization before running test cases.
pkg/config/command_merging_behavior_test.go (2)
20-21: LGTM!Test adapters properly initialized before running test cases.
337-338: LGTM!Consistent initialization pattern applied to this test function as well.
pkg/config/import_commands_test.go (2)
16-17: LGTM!Adapter initialization properly placed at test start.
263-264: LGTM!Consistent initialization pattern maintained across edge case tests.
pkg/config/import_adapter_registry.go (7)
11-16: LGTM!Clean singleton pattern with lazy initialization support.
18-24: LGTM!Thread-safe registry design with proper locking primitives.
26-49: LGTM!Thread-safe registration with proper nil handling and performance tracking.
51-69: Lazy initialization pattern implemented correctly.The sync.Once pattern properly avoids circular dependencies. Functions like
SetBuiltinAdaptersInitializerare simple setters (exception per guidelines).
71-104: LGTM!Thread-safe lookup with proper locking, case-insensitive matching, and guaranteed non-nil return.
106-147: LGTM!Clean accessor and mutator functions with proper locking, performance tracking, and defensive copying where appropriate.
149-167: LGTM!Safe fallback adapter implementation. The nolint directive is appropriate for interface compliance.
pkg/config/adapters/mock_adapter.go (4)
16-33: LGTM!Clean design with appropriate file permissions and good documentation for test usage patterns.
35-62: LGTM!Singleton pattern with clean accessor API. These are trivial getters/setters (exception to perf.Track requirement).
64-123: Resolve implementation looks solid.Good performance tracking and proper static error usage. Minor note: Line 103's comment mentions "log error" but the code just returns partial results silently. This might be intentional for testing flexibility, but worth verifying that's the desired behavior.
125-140: LGTM!Clean helper with proper error wrapping and unique file generation.
pkg/config/adapters/gogetter_adapter.go (1)
39-39: No action needed. The GoGetterAdapter correctly lists all schemes that go-getter supports, includingfile://and shorthand schemes. There is no LocalAdapter or ImportAdapterRegistry infrastructure to create routing conflicts.Likely an incorrect or invalid review comment.
|
Important Cloud Posse Engineering Team Review RequiredThis pull request modifies files that require Cloud Posse's review. Please be patient, and a core maintainer will review your changes. To expedite this process, reach out to us on Slack in the |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
pkg/config/adapters/gogetter_adapter.go (2)
64-69: Wrap the download error with a static sentinel.The error from
downloadRemoteConfigis already wrapped internally, but the outer log+return pattern here loses context about what operation failed at the Resolve level.🔎 Suggested improvement
tempFile, err := downloadRemoteConfig(ctx, importPath, tempDir) if err != nil { log.Debug("failed to download remote config", "path", importPath, "error", err) - return nil, err + return nil, err // Already wrapped by downloadRemoteConfig }The current code is acceptable since
downloadRemoteConfigalready wraps withErrDownloadRemoteConfig. Adding a comment clarifies intent.
114-119: Consider making the timeout configurable via environment variable or documenting why 5 seconds is appropriate.The hardcoded 5-second timeout is significantly shorter than go-getter's own defaults (30 seconds for HTTP, 5 minutes for VCS protocols). This could cause legitimate downloads to fail over slower connections without clear visibility into why. Per the codebase patterns, configuration like this should support environment variables (
ATMOS_prefix) to let users adjust for their network conditions.pkg/config/adapters/adapters_test.go (2)
39-54: Test doesn't exercise GoGetterAdapter error path.This test validates viper behavior directly rather than testing how
GoGetterAdapter.Resolvehandles a malformed downloaded file. The actual adapter error path is covered byTestGoGetterAdapter_ResolveWithInvalidYAML(lines 492-509).Consider removing this test or renaming it to clarify it's testing viper behavior as a pre-condition check.
103-107: Clarify expected behavior for invalid YAML.The comment says "should skip files that fail to load" but the test doesn't verify that. The
_ = pathsassignment is unused.🔎 Suggested improvement
paths, err := adapter.Resolve(ctx, "invalid.yaml", tempDir, tempDir, 1, 10) // Should not error but should skip files that fail to load. assert.NoError(t, err) - _ = paths + // Verify we still get a resolved path even though YAML parsing failed. + assert.NotEmpty(t, paths)Or if the expectation is that paths should be empty:
- _ = paths + assert.Empty(t, paths, "Invalid YAML files should be skipped")
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
errors/errors.goexamples/quick-start-advanced/Dockerfilepkg/config/adapters/adapters_test.gopkg/config/adapters/gogetter_adapter.gopkg/config/import_adapter_registry_test.gopkg/devcontainer/lifecycle_rebuild_test.gowebsite/src/data/roadmap.js
🧰 Additional context used
📓 Path-based instructions (4)
**/*.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
**/*.go: Use Viper for managing configuration, environment variables, and flags in CLI commands
Use interfaces for external dependencies to facilitate mocking and consider using testify/mock for creating mock implementations
All code must pass golangci-lint checks
Follow Go's error handling idioms: use meaningful error messages, wrap errors with context usingfmt.Errorf("context: %w", err), and consider using custom error types for domain-specific errors
Follow standard Go coding style: usegofmtandgoimportsto format code, prefer short descriptive variable names, use kebab-case for command-line flags, and snake_case for environment variables
Document all exported functions, types, and methods following Go's documentation conventions
Document complex logic with inline comments in Go code
Support configuration via files, environment variables, and flags following the precedence order: flags > environment variables > config file > defaults
Provide clear error messages to users, include troubleshooting hints when appropriate, and log detailed errors for debugging
**/*.go: All comments must end with periods (enforced bygodotlinter) in Go code
Organize imports into three groups separated by blank lines, sorted alphabetically: Go stdlib, 3rd-party (NOT cloudposse/atmos), then Atmos packages with maintained aliases (cfg,log,u,errUtils)
All errors MUST be wrapped using static errors defined inerrors/errors.go- useerrors.Joinfor combining errors,fmt.Errorfwith%wfor context, anderrors.Is()for error checking
Never manually create mocks - usego.uber.org/mock/mockgenwith//go:generatedirectives in Go code
Keep files small and focused - under 600 lines with one cmd/impl per file, co-locate tests, never use//revive:disable:file-length-limit
Use colors frompkg/ui/theme/colors.gofor all UI theming in Go code
Code must be compatible with Linux, macOS, and Windows - use SDKs over binaries, usefilepath.Join()instead of h...
Files:
errors/errors.gopkg/devcontainer/lifecycle_rebuild_test.gopkg/config/import_adapter_registry_test.gopkg/config/adapters/adapters_test.gopkg/config/adapters/gogetter_adapter.go
**/*_test.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
**/*_test.go: Every new feature must include comprehensive unit tests targeting >80% code coverage for all packages
Use table-driven tests for testing multiple scenarios in Go
Include integration tests for command flows and test CLI end-to-end when possible with test fixturesPrefer unit tests with mocks over integration tests - use interfaces and dependency injection for testability, generate mocks with
go.uber.org/mock/mockgen, use table-driven tests, target >80% coverage
Files:
pkg/devcontainer/lifecycle_rebuild_test.gopkg/config/import_adapter_registry_test.gopkg/config/adapters/adapters_test.go
**/{pkg,internal,cmd}/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Add
defer perf.Track(atmosConfig, "pkg.FuncName")()plus blank line to all public functions, usingnilif no atmosConfig param - exceptions: trivial getters/setters, command constructors, simple factories, functions delegating to tracked functions
Files:
pkg/devcontainer/lifecycle_rebuild_test.gopkg/config/import_adapter_registry_test.gopkg/config/adapters/adapters_test.gopkg/config/adapters/gogetter_adapter.go
website/**
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
website/**: Update website documentation in thewebsite/directory when adding new features, ensure consistency between CLI help text and website documentation, and follow the website's documentation structure and style
Keep website code in thewebsite/directory, follow the existing website architecture and style, and test website changes locally before committing
Keep CLI documentation and website documentation in sync and document new features on the website with examples and use cases
Files:
website/src/data/roadmap.js
🧠 Learnings (49)
📓 Common learnings
Learnt from: osterman
Repo: cloudposse/atmos PR: 1686
File: errors/errors.go:184-203
Timestamp: 2025-12-13T06:10:13.688Z
Learning: cloudposse/atmos: For toolchain work, duplicate/unused error sentinels in errors/errors.go should be cleaned up in a separate refactor PR and not block feature PRs; canonical toolchain sentinels live under toolchain/registry with re-exports in toolchain/errors.go.
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T00:55:21.698Z
Learning: Follow multi-provider registry pattern: define interface, implement per provider, register implementations, generate mocks - see `pkg/store/` example
📚 Learning: 2025-10-02T19:17:51.630Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1504
File: pkg/profiler/profiler.go:20-31
Timestamp: 2025-10-02T19:17:51.630Z
Learning: In pkg/profiler/profiler.go, profiler-specific errors (ErrUnsupportedProfileType, ErrStartCPUProfile, ErrStartTraceProfile, ErrCreateProfileFile) must remain local and cannot be moved to errors/errors.go due to an import cycle: pkg/profiler → errors → pkg/schema → pkg/profiler. This is a valid exception to the centralized errors policy.
Applied to files:
errors/errors.gopkg/config/adapters/gogetter_adapter.go
📚 Learning: 2026-01-04T00:55:21.698Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T00:55:21.698Z
Learning: Applies to **/*.go : All errors MUST be wrapped using static errors defined in `errors/errors.go` - use `errors.Join` for combining errors, `fmt.Errorf` with `%w` for context, and `errors.Is()` for error checking
Applied to files:
errors/errors.gopkg/config/adapters/gogetter_adapter.go
📚 Learning: 2025-12-21T04:10:29.030Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1891
File: internal/exec/describe_affected.go:468-468
Timestamp: 2025-12-21T04:10:29.030Z
Learning: In Go, package-level declarations (constants, variables, types, and functions) are visible to all files in the same package without imports. During reviews in cloudposse/atmos (and similar Go codebases), before suggesting to declare a new identifier, first check if it already exists in another file of the same package. If it exists, you can avoid adding a new declaration; if not, proceed with a proper package-level declaration.
Applied to files:
errors/errors.gopkg/devcontainer/lifecycle_rebuild_test.gopkg/config/import_adapter_registry_test.gopkg/config/adapters/adapters_test.gopkg/config/adapters/gogetter_adapter.go
📚 Learning: 2025-11-24T17:35:37.209Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-11-24T17:35:37.209Z
Learning: Applies to **/*.go : Provide clear error messages to users, include troubleshooting hints when appropriate, and log detailed errors for debugging
Applied to files:
errors/errors.gopkg/config/adapters/gogetter_adapter.go
📚 Learning: 2025-11-24T17:35:37.209Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-11-24T17:35:37.209Z
Learning: Applies to **/*.go : Follow Go's error handling idioms: use meaningful error messages, wrap errors with context using `fmt.Errorf("context: %w", err)`, and consider using custom error types for domain-specific errors
Applied to files:
errors/errors.gopkg/config/adapters/gogetter_adapter.go
📚 Learning: 2024-11-12T03:15:15.627Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 775
File: examples/quick-start-advanced/Dockerfile:9-9
Timestamp: 2024-11-12T03:15:15.627Z
Learning: It is acceptable to set `ARG ATMOS_VERSION` to a future version like `1.105.0` in `examples/quick-start-advanced/Dockerfile` if that will be the next release.
Applied to files:
examples/quick-start-advanced/Dockerfilepkg/devcontainer/lifecycle_rebuild_test.go
📚 Learning: 2024-11-23T00:13:22.004Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 801
File: examples/quick-start-advanced/Dockerfile:9-9
Timestamp: 2024-11-23T00:13:22.004Z
Learning: When updating the `ATMOS_VERSION` in Dockerfiles, the team prefers to pin to the next future version when the PR merges, even if the version is not yet released.
Applied to files:
examples/quick-start-advanced/Dockerfilepkg/devcontainer/lifecycle_rebuild_test.go
📚 Learning: 2025-09-10T21:17:55.273Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 1466
File: toolchain/http_client_test.go:3-10
Timestamp: 2025-09-10T21:17:55.273Z
Learning: In the cloudposse/atmos repository, imports should never be changed as per samtholiya's coding guidelines.
Applied to files:
examples/quick-start-advanced/Dockerfile
📚 Learning: 2025-11-01T20:24:29.557Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1714
File: NOTICE:0-0
Timestamp: 2025-11-01T20:24:29.557Z
Learning: In the cloudposse/atmos repository, the NOTICE file is programmatically generated and should not be manually edited. Issues with dependency license URLs in NOTICE will be resolved when upstream package metadata is corrected.
Applied to files:
examples/quick-start-advanced/Dockerfile
📚 Learning: 2025-03-18T12:26:25.329Z
Learnt from: Listener430
Repo: cloudposse/atmos PR: 1149
File: tests/snapshots/TestCLICommands_atmos_vendor_pull_ssh.stderr.golden:7-7
Timestamp: 2025-03-18T12:26:25.329Z
Learning: In the Atmos project, typos or inconsistencies in test snapshot files (such as "terrafrom" instead of "terraform") may be intentional as they capture the exact output of commands and should not be flagged as issues requiring correction.
Applied to files:
examples/quick-start-advanced/Dockerfile
📚 Learning: 2025-01-25T03:51:57.689Z
Learnt from: Listener430
Repo: cloudposse/atmos PR: 934
File: tests/fixtures/scenarios/docs-generate/README.md.gotmpl:99-118
Timestamp: 2025-01-25T03:51:57.689Z
Learning: For the cloudposse/atmos repository, changes to template contents should be handled in dedicated PRs and are typically considered out of scope for PRs focused on other objectives.
Applied to files:
examples/quick-start-advanced/Dockerfile
📚 Learning: 2025-01-17T00:18:57.769Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 944
File: go.mod:206-206
Timestamp: 2025-01-17T00:18:57.769Z
Learning: For indirect dependencies with license compliance issues in the cloudposse/atmos repository, the team prefers to handle them in follow-up PRs rather than blocking the current changes, as these issues often require deeper investigation of the dependency tree.
Applied to files:
examples/quick-start-advanced/Dockerfile
📚 Learning: 2024-11-25T17:17:15.703Z
Learnt from: RoseSecurity
Repo: cloudposse/atmos PR: 797
File: pkg/list/atmos.yaml:213-214
Timestamp: 2024-11-25T17:17:15.703Z
Learning: The file `pkg/list/atmos.yaml` is primarily intended for testing purposes.
Applied to files:
examples/quick-start-advanced/Dockerfile
📚 Learning: 2025-02-14T23:12:38.030Z
Learnt from: Listener430
Repo: cloudposse/atmos PR: 1061
File: tests/snapshots/TestCLICommands_atmos_vendor_pull_ssh.stderr.golden:8-8
Timestamp: 2025-02-14T23:12:38.030Z
Learning: Test snapshots in the Atmos project, particularly for dry run scenarios, may be updated during the development process, and temporary inconsistencies in their content should not be flagged as issues.
Applied to files:
examples/quick-start-advanced/Dockerfile
📚 Learning: 2025-09-08T01:25:44.958Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1466
File: website/docs/cli/commands/toolchain/usage.mdx:117-121
Timestamp: 2025-09-08T01:25:44.958Z
Learning: The atmos toolchain has been updated to follow XDG Base Directory Specification with helper functions GetXDGCacheDir() and GetXDGTempCacheDir() in toolchain/xdg_cache.go, using XDG_CACHE_HOME when set and falling back to ~/.cache/atmos-toolchain, making it consistent with atmos core's XDG compliance.
Applied to files:
examples/quick-start-advanced/Dockerfilepkg/devcontainer/lifecycle_rebuild_test.go
📚 Learning: 2025-09-10T17:34:52.568Z
Learnt from: Benbentwo
Repo: cloudposse/atmos PR: 1475
File: pkg/auth/providers/github/oidc.go:96-100
Timestamp: 2025-09-10T17:34:52.568Z
Learning: The ATMOS_ environment variable binding guideline applies to Atmos configuration variables, not external service-required environment variables like GitHub Actions OIDC variables (GITHUB_ACTIONS, ACTIONS_ID_TOKEN_*) which must use their standard names.
Applied to files:
examples/quick-start-advanced/Dockerfile
📚 Learning: 2024-10-25T20:26:56.449Z
Learnt from: Cerebrovinny
Repo: cloudposse/atmos PR: 729
File: .goreleaser.yml:26-26
Timestamp: 2024-10-25T20:26:56.449Z
Learning: There is no inconsistency in the version path in `build.sh`; it already uses the correct path `github.com/cloudposse/atmos/pkg/version.Version`.
Applied to files:
examples/quick-start-advanced/Dockerfile
📚 Learning: 2025-11-11T03:47:59.576Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1686
File: toolchain/which_test.go:166-223
Timestamp: 2025-11-11T03:47:59.576Z
Learning: In the cloudposse/atmos repo, tests that manipulate environment variables should use testing.T.Setenv for automatic setup/teardown instead of os.Setenv/Unsetenv.
Applied to files:
examples/quick-start-advanced/Dockerfilepkg/devcontainer/lifecycle_rebuild_test.go
📚 Learning: 2025-09-13T16:39:20.007Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 1466
File: cmd/markdown/atmos_toolchain_aliases.md:2-4
Timestamp: 2025-09-13T16:39:20.007Z
Learning: In the cloudposse/atmos repository, CLI documentation files in cmd/markdown/ follow a specific format that uses " $ atmos command" (with leading space and dollar sign prompt) in code blocks. This is the established project convention and should not be changed to comply with standard markdownlint rules MD040 and MD014.
Applied to files:
examples/quick-start-advanced/Dockerfile
📚 Learning: 2025-12-12T18:52:54.205Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1857
File: Dockerfile:29-29
Timestamp: 2025-12-12T18:52:54.205Z
Learning: In Dockerfiles that install Helm plugins, omit --verify=false for helm plugin install when using Helm 3.x; this flag is only supported in Helm 4.x. Ensure your Helm version and command usage align to avoid errors.
Applied to files:
examples/quick-start-advanced/Dockerfile
📚 Learning: 2025-11-10T23:23:39.771Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1686
File: toolchain/registry/aqua/aqua_test.go:417-442
Timestamp: 2025-11-10T23:23:39.771Z
Learning: In Atmos toolchain AquaRegistry, tests should not hit real GitHub. Use the options pattern via WithGitHubBaseURL to inject an httptest server URL and make GetLatestVersion/GetAvailableVersions deterministic.
Applied to files:
pkg/devcontainer/lifecycle_rebuild_test.gopkg/config/adapters/adapters_test.go
📚 Learning: 2025-07-05T20:59:02.914Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 1363
File: internal/exec/template_utils.go:18-18
Timestamp: 2025-07-05T20:59:02.914Z
Learning: In the Atmos project, gomplate v4 is imported with a blank import (`_ "github.com/hairyhenderson/gomplate/v4"`) alongside v3 imports to resolve AWS SDK version conflicts. V3 uses older AWS SDK versions that conflict with newer AWS modules used by Atmos. A full migration to v4 requires extensive refactoring due to API changes and should be handled in a separate PR.
Applied to files:
pkg/devcontainer/lifecycle_rebuild_test.go
📚 Learning: 2024-12-02T21:26:32.337Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 808
File: pkg/config/config.go:478-483
Timestamp: 2024-12-02T21:26:32.337Z
Learning: In the 'atmos' project, when reviewing Go code like `pkg/config/config.go`, avoid suggesting file size checks after downloading remote configs if such checks aren't implemented elsewhere in the codebase.
Applied to files:
pkg/devcontainer/lifecycle_rebuild_test.gopkg/config/adapters/gogetter_adapter.go
📚 Learning: 2025-11-11T03:47:45.878Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1686
File: toolchain/add_test.go:67-77
Timestamp: 2025-11-11T03:47:45.878Z
Learning: In the cloudposse/atmos codebase, tests should prefer t.Setenv for environment variable setup/teardown instead of os.Setenv/Unsetenv to ensure test-scoped isolation.
Applied to files:
pkg/devcontainer/lifecycle_rebuild_test.go
📚 Learning: 2025-09-08T01:25:44.958Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1466
File: website/docs/cli/commands/toolchain/usage.mdx:117-121
Timestamp: 2025-09-08T01:25:44.958Z
Learning: Final XDG Base Directory Specification implementation for atmos toolchain is complete and verified: toolchain/xdg_cache.go provides GetXDGCacheDir() and GetXDGTempCacheDir() functions, all hardcoded ~/.cache/tools-cache paths have been replaced with XDG-compliant paths using ${XDG_CACHE_HOME}/atmos-toolchain (or ~/.cache/atmos-toolchain fallback), and tests have been updated to expect the new path structure.
Applied to files:
pkg/devcontainer/lifecycle_rebuild_test.go
📚 Learning: 2025-09-08T01:25:44.958Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1466
File: website/docs/cli/commands/toolchain/usage.mdx:117-121
Timestamp: 2025-09-08T01:25:44.958Z
Learning: The atmos toolchain XDG compliance implementation is complete with GetXDGCacheDir() and GetXDGTempCacheDir() functions in toolchain/xdg_cache.go, updated installer.go and toolchain_clean.go to use these helpers, and changed cache paths from ~/.cache/tools-cache to ${XDG_CACHE_HOME}/atmos-toolchain (or ~/.cache/atmos-toolchain when XDG_CACHE_HOME is not set).
Applied to files:
pkg/devcontainer/lifecycle_rebuild_test.go
📚 Learning: 2026-01-04T00:55:21.698Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T00:55:21.698Z
Learning: Applies to **/*.go : Use `viper.BindEnv("ATMOS_VAR", "ATMOS_VAR", "FALLBACK")` for environment variables - ATMOS_ prefix required in Go code
Applied to files:
pkg/devcontainer/lifecycle_rebuild_test.go
📚 Learning: 2024-10-20T13:12:46.499Z
Learnt from: haitham911
Repo: cloudposse/atmos PR: 736
File: pkg/config/const.go:6-6
Timestamp: 2024-10-20T13:12:46.499Z
Learning: In `cmd/cmd_utils.go`, it's acceptable to have hardcoded references to `atmos.yaml` in logs, and it's not necessary to update them to use the `CliConfigFileName` constant.
Applied to files:
pkg/devcontainer/lifecycle_rebuild_test.go
📚 Learning: 2024-12-11T18:40:12.808Z
Learnt from: Listener430
Repo: cloudposse/atmos PR: 844
File: cmd/helmfile.go:37-37
Timestamp: 2024-12-11T18:40:12.808Z
Learning: In the atmos project, `cliConfig` is initialized within the `cmd` package in `root.go` and can be used in other command files.
Applied to files:
pkg/devcontainer/lifecycle_rebuild_test.go
📚 Learning: 2024-10-23T21:36:40.262Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 740
File: cmd/cmd_utils.go:340-359
Timestamp: 2024-10-23T21:36:40.262Z
Learning: In the Go codebase for Atmos, when reviewing functions like `checkAtmosConfig` in `cmd/cmd_utils.go`, avoid suggesting refactoring to return errors instead of calling `os.Exit` if such changes would significantly increase the scope due to the need to update multiple call sites.
Applied to files:
pkg/devcontainer/lifecycle_rebuild_test.gopkg/config/adapters/gogetter_adapter.go
📚 Learning: 2025-11-08T19:56:18.660Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1697
File: internal/exec/oci_utils.go:0-0
Timestamp: 2025-11-08T19:56:18.660Z
Learning: In the Atmos codebase, when a function receives an `*schema.AtmosConfiguration` parameter, it should read configuration values from `atmosConfig.Settings` fields rather than using direct `os.Getenv()` or `viper.GetString()` calls. The Atmos pattern is: viper.BindEnv in cmd/root.go binds environment variables → Viper unmarshals into atmosConfig.Settings via mapstructure → business logic reads from the Settings struct. This provides centralized config management, respects precedence, and enables testability. Example: `atmosConfig.Settings.AtmosGithubToken` instead of `os.Getenv("ATMOS_GITHUB_TOKEN")` in functions like `getGHCRAuth` in internal/exec/oci_utils.go.
Applied to files:
pkg/devcontainer/lifecycle_rebuild_test.gopkg/config/adapters/gogetter_adapter.go
📚 Learning: 2025-08-16T23:32:40.412Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 1405
File: internal/exec/describe_dependents_test.go:455-456
Timestamp: 2025-08-16T23:32:40.412Z
Learning: In the cloudposse/atmos Go codebase, `InitCliConfig` returns a `schema.AtmosConfiguration` value (not a pointer), while `ExecuteDescribeDependents` expects a `*schema.AtmosConfiguration` pointer parameter. Therefore, when passing the result of `InitCliConfig` to `ExecuteDescribeDependents`, use `&atmosConfig` to pass the address of the value.
Applied to files:
pkg/devcontainer/lifecycle_rebuild_test.go
📚 Learning: 2026-01-04T00:55:21.698Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T00:55:21.698Z
Learning: Applies to **/*_test.go : Prefer unit tests with mocks over integration tests - use interfaces and dependency injection for testability, generate mocks with `go.uber.org/mock/mockgen`, use table-driven tests, target >80% coverage
Applied to files:
pkg/config/import_adapter_registry_test.gopkg/config/adapters/adapters_test.go
📚 Learning: 2026-01-04T00:55:21.698Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T00:55:21.698Z
Learning: Follow multi-provider registry pattern: define interface, implement per provider, register implementations, generate mocks - see `pkg/store/` example
Applied to files:
pkg/config/import_adapter_registry_test.go
📚 Learning: 2025-11-24T17:35:37.209Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-11-24T17:35:37.209Z
Learning: Applies to **/*_test.go : Use table-driven tests for testing multiple scenarios in Go
Applied to files:
pkg/config/import_adapter_registry_test.gopkg/config/adapters/adapters_test.go
📚 Learning: 2025-11-24T17:35:37.209Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-11-24T17:35:37.209Z
Learning: Applies to **/*.go : Use interfaces for external dependencies to facilitate mocking and consider using testify/mock for creating mock implementations
Applied to files:
pkg/config/import_adapter_registry_test.gopkg/config/adapters/adapters_test.go
📚 Learning: 2025-11-24T17:35:37.209Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-11-24T17:35:37.209Z
Learning: Applies to **/*_test.go : Include integration tests for command flows and test CLI end-to-end when possible with test fixtures
Applied to files:
pkg/config/import_adapter_registry_test.gopkg/config/adapters/adapters_test.go
📚 Learning: 2025-11-24T17:35:37.209Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-11-24T17:35:37.209Z
Learning: Applies to **/*_test.go : Every new feature must include comprehensive unit tests targeting >80% code coverage for all packages
Applied to files:
pkg/config/adapters/adapters_test.go
📚 Learning: 2024-11-18T13:59:10.824Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 768
File: internal/exec/vendor_model_component.go:3-20
Timestamp: 2024-11-18T13:59:10.824Z
Learning: When replacing significant dependencies like `go-getter` that require extensive changes, prefer to address them in separate PRs.
Applied to files:
pkg/config/adapters/gogetter_adapter.go
📚 Learning: 2025-09-10T22:38:42.212Z
Learnt from: Benbentwo
Repo: cloudposse/atmos PR: 1475
File: pkg/auth/identities/aws/user.go:141-145
Timestamp: 2025-09-10T22:38:42.212Z
Learning: The user confirmed that the errors package has an error string wrapping format, contradicting the previous learning about ErrWrappingFormat being invalid. The current usage of fmt.Errorf(errUtils.ErrWrappingFormat, errUtils.ErrAuthAwsFileManagerFailed, err) appears to be the correct pattern.
Applied to files:
pkg/config/adapters/gogetter_adapter.go
📚 Learning: 2025-12-13T06:10:13.688Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1686
File: errors/errors.go:184-203
Timestamp: 2025-12-13T06:10:13.688Z
Learning: cloudposse/atmos: For toolchain work, duplicate/unused error sentinels in errors/errors.go should be cleaned up in a separate refactor PR and not block feature PRs; canonical toolchain sentinels live under toolchain/registry with re-exports in toolchain/errors.go.
Applied to files:
pkg/config/adapters/gogetter_adapter.go
📚 Learning: 2025-01-07T20:38:09.618Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 896
File: cmd/editor_config.go:37-40
Timestamp: 2025-01-07T20:38:09.618Z
Learning: Error handling suggestion for `cmd.Help()` in `cmd/editor_config.go` was deferred as the code is planned for future modifications.
Applied to files:
pkg/config/adapters/gogetter_adapter.go
📚 Learning: 2025-10-22T14:55:44.014Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1695
File: pkg/auth/manager.go:169-171
Timestamp: 2025-10-22T14:55:44.014Z
Learning: Go 1.20+ supports multiple %w verbs in fmt.Errorf, which returns an error implementing Unwrap() []error. This is valid and does not panic. Atmos uses Go 1.24.8 and configures errorlint with errorf-multi: true to validate this pattern.
Applied to files:
pkg/config/adapters/gogetter_adapter.go
📚 Learning: 2025-09-10T22:38:42.212Z
Learnt from: Benbentwo
Repo: cloudposse/atmos PR: 1475
File: pkg/auth/identities/aws/user.go:141-145
Timestamp: 2025-09-10T22:38:42.212Z
Learning: ErrWrappingFormat is correctly defined as "%w: %w" in the errors package and is used throughout the codebase to wrap two error types together. The usage fmt.Errorf(errUtils.ErrWrappingFormat, errUtils.ErrAuthAwsFileManagerFailed, err) is the correct pattern when both arguments are error types.
Applied to files:
pkg/config/adapters/gogetter_adapter.go
📚 Learning: 2024-10-22T23:00:20.627Z
Learnt from: Cerebrovinny
Repo: cloudposse/atmos PR: 737
File: internal/exec/vendor_utils.go:131-141
Timestamp: 2024-10-22T23:00:20.627Z
Learning: In the `ReadAndProcessVendorConfigFile` function in `internal/exec/vendor_utils.go`, the existence of the vendor config file is already checked, so additional file existence checks may be unnecessary.
Applied to files:
pkg/config/adapters/gogetter_adapter.go
📚 Learning: 2025-04-04T02:03:23.676Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 1185
File: internal/exec/yaml_func_store.go:26-26
Timestamp: 2025-04-04T02:03:23.676Z
Learning: The Atmos codebase currently uses `log.Fatal` for error handling in multiple places. The maintainers are aware this isn't an ideal pattern (should only be used in main() or init() functions) and plan to address it comprehensively in a separate PR. CodeRabbit should not flag these issues or push for immediate changes until that refactoring is complete.
Applied to files:
pkg/config/adapters/gogetter_adapter.go
📚 Learning: 2024-10-28T01:51:30.811Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 727
File: internal/exec/terraform_clean.go:329-332
Timestamp: 2024-10-28T01:51:30.811Z
Learning: In the Atmos Go code, when deleting directories or handling file paths (e.g., in `terraform_clean.go`), always resolve the absolute path using `filepath.Abs` and use the logger `u.LogWarning` for logging messages instead of using `fmt.Printf`.
Applied to files:
pkg/config/adapters/gogetter_adapter.go
📚 Learning: 2026-01-04T00:55:21.698Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T00:55:21.698Z
Learning: PRs labeled `minor` or `major` MUST update `website/src/data/roadmap.js` with shipped milestones, status, changelog link, PR number, and initiative progress percentage
Applied to files:
website/src/data/roadmap.js
🧬 Code graph analysis (2)
pkg/config/import_adapter_registry_test.go (2)
pkg/config/imports.go (2)
ResolvedPaths(64-68)ADAPTER(58-58)pkg/config/import_adapter_registry.go (7)
RegisterImportAdapter(31-49)GetRegisteredAdapters(131-138)GetDefaultAdapter(142-147)SetDefaultAdapter(108-115)FindImportAdapter(76-104)SetBuiltinAdaptersInitializer(57-59)EnsureAdaptersInitialized(63-69)
pkg/config/adapters/gogetter_adapter.go (4)
pkg/config/imports.go (3)
ResolvedPaths(64-68)REMOTE(57-57)ProcessImportsFromAdapter(121-123)pkg/logger/log.go (1)
Debug(24-26)errors/errors.go (4)
ErrWrapFormat(12-12)ErrReadConfig(240-240)ErrProcessNestedImports(577-577)ErrDownloadRemoteConfig(575-575)pkg/downloader/file_downloader_interface.go (1)
ClientModeFile(57-57)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Acceptance Tests (windows)
- GitHub Check: Acceptance Tests (macos)
- GitHub Check: Summary
🔇 Additional comments (19)
examples/quick-start-advanced/Dockerfile (1)
9-9: LGTM - Version bump aligns with new adapter functionality.The increment to 1.204.0 is consistent with the PR's introduction of the import adapter registry system.
pkg/devcontainer/lifecycle_rebuild_test.go (1)
412-412: LGTM - Test expectations updated to match version bump.The test correctly validates that the ATMOS_VERSION build argument flows through the rebuild process with the updated version.
Also applies to: 434-434
pkg/config/adapters/gogetter_adapter.go (5)
1-16: LGTM on imports and package structure.Import groups correctly organized: stdlib, third-party, then Atmos packages with maintained aliases (
errUtils,log).
18-49: Schemes list is comprehensive.Covers HTTP, Git, cloud storage (S3, GCS), OCI, and popular host shorthands. Good coverage for go-getter capabilities.
74-78: Error wrapping now uses static sentinel.Previous review flagged this. Now correctly wraps with
ErrReadConfigper coding guidelines.
94-101: Error wrapping for nested imports looks good.Previous review flagged this. Now correctly wraps with
ErrProcessNestedImports.
121-134: Download implementation is solid.Uses go-getter's
ClientModeFile, generates unique temp filenames, and wraps errors withErrDownloadRemoteConfig.errors/errors.go (1)
575-577: New error sentinels are well-placed and follow conventions.These support the adapter system with clear, descriptive messages. Good alignment with existing patterns in the file.
pkg/config/import_adapter_registry_test.go (5)
10-28: Clean mock adapter for testing.Simple implementation satisfying the ImportAdapter interface. Good use of
ADAPTERimport type.
30-64: Good coverage of registration edge cases.Tests nil adapter handling, scheme-based registration, and default adapter behavior (adapters with nil schemes become default).
109-123: Validates noopAdapter fallback correctly.Tests that when no default adapter is set,
FindImportAdapterreturns a safe no-op adapter that returnsnil, nilfrom Resolve.
174-215: Solid tests for lazy initialization.Verifies
SetBuiltinAdaptersInitializer, handles nil initializer gracefully, and confirmssync.Oncesemantics (called exactly once).
229-241: Good test for registration order priority.Validates that more-specific adapters registered first take precedence over general ones.
pkg/config/adapters/adapters_test.go (6)
1-14: Imports well-organized.Stdlib, third-party (viper, testify), then Atmos packages. Follows conventions.
198-225: Good table-driven test for adapter routing.Covers HTTP, HTTPS, Git, S3, mock, local, and absolute paths. Validates scheme matching and local fallback behavior.
290-322: Solid nested imports test.Validates that LocalAdapter correctly processes files with
import:sections and resolves nested dependencies.
407-425: Context handling tests are valuable.Tests both
context.TODO()andcontext.Background()paths, ensuring no panics occur with different context types.
455-463: Good error path coverage for MockAdapter.Tests the write error case when temp directory doesn't exist.
616-638: Comprehensive downloadRemoteConfig test.Validates local file download via
file://URL and verifies content integrity.
Added the missing changelog field linking to the blog post slug 'import-adapter-registry' as required for shipped milestones. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
Warning Release Documentation RequiredThis PR is labeled
|
Add two new milestones to the Feature Parity with Terragrunt initiative: - Unified import adapter registry (shipped, Q1-2026, PR #1897) - Terragrunt import adapter (planned, Q1-2026) Also add PR #1897 to the migration initiative's PR list. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
These changes were released in v1.204.0-rc.1. |
Add two new milestones to the Feature Parity with Terragrunt initiative: - Unified import adapter registry (shipped, Q1-2026, PR #1897) - Terragrunt import adapter (planned, Q1-2026) Also add PR #1897 to the migration initiative's PR list. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
2 participants
what
why
references
docs/prd/import-adapter-registry.mdSummary by CodeRabbit
New Features
Tests
Documentation
Chores
✏️ Tip: You can customize this high-level summary in your review settings.