refactor: Migrate vendor to registry pattern + implement --stack flag#1889
refactor: Migrate vendor to registry pattern + implement --stack flag#1889
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughLarge-scale restructuring that migrates the vendor subsystem from Changes
Sequence DiagramsequenceDiagram
participant CLI as CLI (cmd/vendor/pull.go)
participant Exec as Vendor Orchestration<br/>(pkg/vendor/vendor.go)
participant Config as Config Processing<br/>(pkg/vendor/config.go)
participant Source as Source Processing<br/>(pkg/vendor/vendor.go)
participant Copy as Component Copy<br/>(pkg/vendor/component.go)
participant OCI as OCI Processing<br/>(internal/exec)
CLI->>CLI: Parse flags via Viper
CLI->>CLI: Create PullOptions
CLI->>Exec: Pull(atmosConfig, opts...)
Exec->>Exec: ValidateFlags
Exec->>Exec: ShouldSetEverythingDefault
alt Route by Component/Stack/Global
Exec->>Config: ReadAndProcessVendorConfigFile
Config->>Config: Resolve config path
Config->>Config: Load and merge YAMLs
Config->>Config: Expand imports recursively
Config-->>Exec: VendorConfigResult
end
Exec->>Source: processAtmosVendorSource
Source->>Source: Filter by component/tags
Source->>Source: Process targets (templates)
Source->>Source: determineSourceType
Source-->>Exec: []pkgAtmosVendor
Exec->>Copy: For each source/target
Copy->>OCI: ProcessOciImage (if OCI)
OCI-->>Copy: Downloaded temp dir
Copy->>Copy: generateSkipFunction
Copy->>Copy: Copy with pattern filtering
Copy-->>Exec: Installed packages
Exec-->>CLI: Success/Error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
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 |
|
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 |
d84fce9 to
bafce28
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/vender/component_vendor_test.go (1)
1-1: Critical: Fix package name typo.The package is declared as
venderbut should bevendorto match the directory name and other vendor package files. Additionally, the filename itself contains the same typo (pkg/vender/should bepkg/vendor/).This will prevent compilation and breaks the module structure.
🧹 Nitpick comments (17)
pkg/vendor/types.go (1)
42-54: Consider consistent field naming.
IsComponentandIsMixins(lines 51-52) are exported names in an unexported struct. While valid Go, mixing casing can be confusing. ConsiderisComponentandisMixinsfor consistency with other fields likenameanduri.🔎 Suggested diff
type pkgComponentVendor struct { uri string name string sourceIsLocalFile bool pkgType pkgType version string vendorComponentSpec *schema.VendorComponentSpec componentPath string - IsComponent bool - IsMixins bool + isComponent bool + isMixins bool mixinFilename string }internal/exec/template_utils.go (1)
45-52: Consider tracking only the expensive initialization.The
defer perf.Trackon Line 46 fires on every call toGetSprigFuncMap(), but only the first call (insidesync.Once) is expensive (~173MB). Subsequent calls just return a cached value. For a hot-path function, you could move the tracking inside the Once block to measure only the meaningful operation:func GetSprigFuncMap() template.FuncMap { - defer perf.Track(nil, "exec.GetSprigFuncMap")() - sprigFuncMapCacheOnce.Do(func() { + defer perf.Track(nil, "exec.GetSprigFuncMap.init")() sprigFuncMapCache = sprig.FuncMap() }) return sprigFuncMapCache }That said,
perf.Trackreturns a no-op when tracking is disabled, so the overhead is negligible. This is an optional optimization.pkg/vendor/config.go (3)
21-50: Missing performance tracking in public function.Per coding guidelines, public functions should include
defer perf.Track(atmosConfig, "vendor.ReadAndProcessVendorConfigFile")()at the start.🔎 Suggested fix:
+import ( + "github.com/cloudposse/atmos/pkg/perf" + // ... other imports +) func ReadAndProcessVendorConfigFile( atmosConfig *schema.AtmosConfiguration, vendorConfigFile string, checkGlobalConfig bool, ) (schema.AtmosVendorConfig, bool, string, error) { + defer perf.Track(atmosConfig, "vendor.ReadAndProcessVendorConfigFile")() + var vendorConfig schema.AtmosVendorConfig
107-144: Consider logging duplicate import detection.When a duplicate import is detected at line 137-140, the code silently skips it. A debug log would help troubleshooting.
🔎 Optional enhancement:
for _, imp := range currentConfig.Spec.Imports { if !importMap[imp] { importMap[imp] = true vendorConfig.Spec.Imports = append(vendorConfig.Spec.Imports, imp) + } else { + log.Debug("Skipping duplicate import", "import", imp, "file", configFile) } }
261-312: Missing performance tracking in public function.
ReadAndProcessComponentVendorConfigFileis a public function and should include perf tracking per guidelines.🔎 Suggested fix:
func ReadAndProcessComponentVendorConfigFile( atmosConfig *schema.AtmosConfiguration, component string, componentType string, ) (schema.VendorComponentConfig, string, error) { + defer perf.Track(atmosConfig, "vendor.ReadAndProcessComponentVendorConfigFile")() + var componentBasePath stringpkg/vendor/vendor_integration_test.go (1)
107-114: Cleanup could leave orphaned parent directories.The cleanup removes individual file directories, but parent directories (e.g.,
components/terraform) may remain. Consider using a more thorough cleanup or tracking the top-level created directory.🔎 Optional improvement:
t.Cleanup(func() { - for _, file := range expectedFiles { - // Remove individual files and their parent directories. - dir := filepath.Dir(file) - os.RemoveAll(dir) - } + // Clean up the entire components directory tree created by the test + os.RemoveAll("./components") })pkg/vendor/stack.go (2)
146-156: Default case falls through to terraform base path.The default case on line 155 silently uses terraform's base path. This could mask configuration issues for unsupported component types.
🔎 Consider explicit handling:
case cfg.PackerComponentType: componentBasePath = atmosConfig.Components.Packer.BasePath default: - componentBasePath = atmosConfig.Components.Terraform.BasePath + log.Debug("Unknown component type, defaulting to terraform base path", "type", componentType) + componentBasePath = atmosConfig.Components.Terraform.BasePath }
231-233: Silent return on empty URI could mask config issues.When
vendorComponentSpec.Source.Uriis empty, the function returnsnil, nilwithout logging. A debug log would help users understand why a component wasn't vendored.🔎 Optional enhancement:
if vendorComponentSpec.Source.Uri == "" { + log.Debug("Skipping component with empty URI", "component", componentName) return nil, nil // No URI, nothing to vendor }pkg/vendor/model.go (2)
534-561: generateSkipFunction modifies input parameter.Lines 543-544 reassign
tempDirandsrcafter converting to slash format. This works but could be confusing. Consider using local variables instead.🔎 Optional clarity improvement:
return func(srcInfo os.FileInfo, src, dest string) (bool, error) { // Skip .git directories if filepath.Base(src) == ".git" { return true, nil } // Normalize paths - tempDir = filepath.ToSlash(tempDir) - src = filepath.ToSlash(src) - trimmedSrc := u.TrimBasePathFromPath(tempDir+"/", src) + normalizedTempDir := filepath.ToSlash(tempDir) + normalizedSrc := filepath.ToSlash(src) + trimmedSrc := u.TrimBasePathFromPath(normalizedTempDir+"/", normalizedSrc) // Check if the file should be excluded if len(s.ExcludedPaths) > 0 { - return shouldExcludeFile(src, s.ExcludedPaths, trimmedSrc) + return shouldExcludeFile(normalizedSrc, s.ExcludedPaths, trimmedSrc) } // Only include the files that match the 'included_paths' patterns if len(s.IncludedPaths) > 0 { - return shouldIncludeFile(src, s.IncludedPaths, trimmedSrc) + return shouldIncludeFile(normalizedSrc, s.IncludedPaths, trimmedSrc) }
293-298: Replace custommaxwith stdlib version.Project targets Go 1.25.2, which includes
maxin the stdlib. The custommaxfunction at lines 293-298 can be replaced with the built-in, removing unnecessary code duplication.pkg/vendor/component.go (3)
61-67: Missing performance tracking on public function.Per coding guidelines, public functions should include
defer perf.Track(atmosConfig, "vendor.ExecuteComponentVendorInternal")()followed by a blank line. TheatmosConfigparameter is available here.🔎 Suggested fix
func ExecuteComponentVendorInternal( atmosConfig *schema.AtmosConfiguration, vendorComponentSpec *schema.VendorComponentSpec, component string, componentPath string, dryRun bool, ) error { + defer perf.Track(atmosConfig, "vendor.ExecuteComponentVendorInternal")() + if vendorComponentSpec.Source.Uri == "" {You'll also need to add the perf import:
"github.com/cloudposse/atmos/pkg/perf"
291-298: Inconsistent temp directory creation.
installMixinusesos.MkdirTempdirectly (line 293), bypassing thecreateTempDirhelper that applies0o700permission restrictions. This creates inconsistency withinstallComponent(line 233) and misses the permission hardening.🔎 Suggested fix
func installMixin(p *pkgComponentVendor, atmosConfig *schema.AtmosConfiguration) error { - tempDir, err := os.MkdirTemp("", "atmos-vendor-mixin") - if err != nil { - return fmt.Errorf("Failed to create temp directory %w", err) - } + tempDir, err := createTempDir() + if err != nil { + return err + } defer removeTempDir(tempDir)
325-339: Consider extracting common copy options.The same
cp.Optionsconfiguration (PreserveTimes, PreserveOwner, OnSymlink) is repeated inhandlePkgTypeLocalComponent(lines 270-278),installMixin(lines 325-339), andcopyComponentToDestination(lines 351-367). A helper function could reduce duplication.pkg/vendor/vendor.go (4)
3-17: Import grouping could be consolidated.Per guidelines, imports should be organized as: stdlib, 3rd-party, then atmos packages (including
internal/). Currentlyinternal/execis in a separate group from other atmos packages.🔎 Suggested grouping
import ( "fmt" "net/url" "path/filepath" "strings" "github.com/samber/lo" cfg "github.com/cloudposse/atmos/pkg/config" + "github.com/cloudposse/atmos/internal/exec" "github.com/cloudposse/atmos/pkg/perf" "github.com/cloudposse/atmos/pkg/schema" u "github.com/cloudposse/atmos/pkg/utils" - - "github.com/cloudposse/atmos/internal/exec" )
110-112: Use available atmosConfig in perf.Track.
params.atmosConfigis available and used throughout this function, butnilis passed toperf.Track. This loses performance tracking context.🔎 Suggested fix
func executeAtmosVendorInternal(params *executeVendorOptions) error { - defer perf.Track(nil, "vendor.executeAtmosVendorInternal")() + defer perf.Track(params.atmosConfig, "vendor.executeAtmosVendorInternal")()
314-336: Missing perf.Track on public function with atmosConfig.
HandleVendorConfigNotExistis public and receivesatmosConfigbut lacks performance tracking.🔎 Suggested fix
func HandleVendorConfigNotExist(atmosConfig *schema.AtmosConfiguration, component, componentType string, dryRun bool) error { + defer perf.Track(atmosConfig, "vendor.HandleVendorConfigNotExist")() + if componentType == "" {
252-285: In-out pointer pattern works but could be clearer.
determineSourceTypemutates*uriin place (line 269, 278). Returning(uri string, useOci, useLocal, isFile bool, err error)would be more idiomatic Go, but the current approach is functional.
Codecov Report❌ Patch coverage is ❌ Your patch check has failed because the patch coverage (79.50%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #1889 +/- ##
==========================================
+ Coverage 75.37% 75.71% +0.34%
==========================================
Files 792 798 +6
Lines 73435 73788 +353
==========================================
+ Hits 55350 55868 +518
+ Misses 14574 14418 -156
+ Partials 3511 3502 -9
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
website/blog/2025-01-17-vendor-stack-flag.mdx (1)
44-44: Clarify vendor configuration file terminology.This line still references
component.yamlwhen the PR objectives note that vendor pull routing now prefersvendor.yaml. Update to reference the preferred file or clarify backward compatibility.🔎 Suggested fix:
-The command will identify all components in the stack that have vendor configurations (via `component.yaml`) and vendor them together. +The command will identify all components in the stack that have `vendor.yaml` configurations and vendor them together.
🧹 Nitpick comments (3)
pkg/vendor/stack.go (1)
147-156: Consider removing the default fallback.The default case silently uses Terraform's base path for unknown component types. Since
extractVendorableComponentsonly passesterraformandhelmfile, this fallback may mask bugs. Either return an error or remove the default case entirely.🔎 Suggested fix:
switch componentType { case cfg.TerraformComponentType: componentBasePath = atmosConfig.Components.Terraform.BasePath case cfg.HelmfileComponentType: componentBasePath = atmosConfig.Components.Helmfile.BasePath - case cfg.PackerComponentType: - componentBasePath = atmosConfig.Components.Packer.BasePath - default: - componentBasePath = atmosConfig.Components.Terraform.BasePath + default: + return nil, skipped, fmt.Errorf("unsupported component type: %s", componentType) }pkg/vendor/model.go (1)
18-26:pkgpackage importing frominternalbreaks module boundaries.The
pkg/vendorpackage importsgithub.com/cloudposse/atmos/internal/exec. This creates a dependency from public API (pkg/) to implementation details (internal/). Consider:
- Moving the needed functions (
ProcessOciImage,SanitizeFileName) topkg/- Using interfaces to invert the dependency
This isn't blocking but worth addressing to maintain clean architecture.
pkg/vendor/pattern.go (1)
158-175: Permissive directory traversal is intentional but could be documented better.The unconditional
return trueat line 174 after the required dirs check seems redundant. IfcontainsAnyreturns true, we already returned. If it returns false, we still return true. The comment helps, but consider simplifying:🔎 Suggested simplification:
func checkDoublestarPrefixPattern(dirPath, pattern string) bool { dirParts := strings.Split(strings.TrimPrefix(dirPath, "/"), "/") requiredDirs := extractRequiredDirs(pattern) - if len(requiredDirs) == 0 { - return false - } - - // If any directory segment matches a required directory, include it. - if containsAny(dirParts, requiredDirs) { - return true - } - - // We must traverse ALL directories to find required ones. - // Files will be filtered later, so empty directories might be created but that's OK. - return true + // For patterns like "**/foo/**", we must traverse all directories + // to potentially find "foo". Files are filtered later. + // Only skip if we have specific required dirs and none match AND + // the directory can't possibly lead to them (not implemented - would require path prefix analysis). + return len(requiredDirs) == 0 || containsAny(dirParts, requiredDirs) || true }Actually, if the intent is to always return true for
**/patterns (to traverse everything), the function could just returntruewith a clear comment. The current logic is effectively: return true unless requiredDirs is empty.
📜 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)
cmd/vendor/pull.go(1 hunks)pkg/vendor/config.go(1 hunks)pkg/vendor/model.go(13 hunks)pkg/vendor/pattern.go(1 hunks)pkg/vendor/stack.go(1 hunks)pkg/vendor/uri.go(2 hunks)website/blog/2025-01-17-vendor-stack-flag.mdx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/vendor/pull.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: 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:
pkg/vendor/uri.gopkg/vendor/stack.gopkg/vendor/model.gopkg/vendor/pattern.gopkg/vendor/config.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/blog/2025-01-17-vendor-stack-flag.mdx
website/blog/**/*.mdx
📄 CodeRabbit inference engine (CLAUDE.md)
website/blog/**/*.mdx: Follow PR template (what/why/references); PRs labeled minor/major MUST include blog post at website/blog/YYYY-MM-DD-feature-name.mdx with YAML front matter, after intro, and only tags from website/blog/tags.yml
Blog posts MUST use only tags defined in website/blog/tags.yml and authors defined in website/blog/authors.yml; valid tags are: feature, enhancement, bugfix, dx, breaking-change, security, documentation, deprecation, core; never invent new tags
Files:
website/blog/2025-01-17-vendor-stack-flag.mdx
🧠 Learnings (45)
📓 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: osterman
Repo: cloudposse/atmos PR: 1686
File: docs/prd/tool-dependencies-integration.md:58-64
Timestamp: 2025-12-13T06:07:37.766Z
Learning: cloudposse/atmos: For PRD docs (docs/prd/*.md), markdownlint issues like MD040/MD010/MD034 can be handled in a separate documentation cleanup commit and should not block the current PR.
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: 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.
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.mod,go.sum} : Manage dependencies with Go modules and keep dependencies up to date while minimizing external dependencies
📚 Learning: 2025-02-05T11:10:51.031Z
Learnt from: mss
Repo: cloudposse/atmos PR: 1024
File: internal/exec/go_getter_utils.go:31-33
Timestamp: 2025-02-05T11:10:51.031Z
Learning: The path traversal check in `ValidateURI` function in `internal/exec/go_getter_utils.go` is intentionally kept despite potentially blocking valid Git URLs, as this validation is planned to be addressed in a separate ticket.
Applied to files:
pkg/vendor/uri.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/vendor/uri.gopkg/vendor/stack.gopkg/vendor/model.gowebsite/blog/2025-01-17-vendor-stack-flag.mdxpkg/vendor/config.go
📚 Learning: 2024-11-13T21:37:07.852Z
Learnt from: Cerebrovinny
Repo: cloudposse/atmos PR: 764
File: internal/exec/describe_stacks.go:289-295
Timestamp: 2024-11-13T21:37:07.852Z
Learning: In the `internal/exec/describe_stacks.go` file of the `atmos` project written in Go, avoid extracting the stack name handling logic into a helper function within the `ExecuteDescribeStacks` method, even if the logic appears duplicated.
Applied to files:
pkg/vendor/stack.go
📚 Learning: 2024-12-07T16:16:13.038Z
Learnt from: Listener430
Repo: cloudposse/atmos PR: 825
File: internal/exec/helmfile_generate_varfile.go:28-31
Timestamp: 2024-12-07T16:16:13.038Z
Learning: In `internal/exec/helmfile_generate_varfile.go`, the `--help` command (`./atmos helmfile generate varfile --help`) works correctly without requiring stack configurations, and the only change needed was to make `ProcessCommandLineArgs` exportable by capitalizing its name.
Applied to files:
pkg/vendor/stack.gowebsite/blog/2025-01-17-vendor-stack-flag.mdx
📚 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/vendor/stack.gopkg/vendor/model.gopkg/vendor/config.go
📚 Learning: 2024-10-20T00:57:53.500Z
Learnt from: haitham911
Repo: cloudposse/atmos PR: 731
File: internal/exec/validate_stacks.go:0-0
Timestamp: 2024-10-20T00:57:53.500Z
Learning: In `internal/exec/validate_stacks.go`, when downloading the Atmos JSON Schema file to the temp directory, the temporary file is overwritten each time, so explicit removal is not necessary.
Applied to files:
pkg/vendor/stack.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:
pkg/vendor/stack.gopkg/vendor/model.gopkg/vendor/config.go
📚 Learning: 2025-10-13T18:13:54.020Z
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.
Applied to files:
pkg/vendor/stack.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/vendor/stack.gopkg/vendor/config.go
📚 Learning: 2025-01-09T22:27:25.538Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 914
File: cmd/validate_stacks.go:20-23
Timestamp: 2025-01-09T22:27:25.538Z
Learning: The validate commands in Atmos can have different help handling implementations. Specifically, validate_component.go and validate_stacks.go are designed to handle help requests differently, with validate_stacks.go including positional argument checks while validate_component.go does not.
Applied to files:
pkg/vendor/stack.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:
pkg/vendor/stack.gopkg/vendor/model.gopkg/vendor/config.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/vendor/model.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/vendor/model.gopkg/vendor/config.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/vendor/model.gopkg/vendor/pattern.go
📚 Learning: 2025-02-21T20:56:20.761Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 1077
File: pkg/downloader/custom_github_detector_test.go:0-0
Timestamp: 2025-02-21T20:56:20.761Z
Learning: The `github.com/charmbracelet/log` package should be imported with the alias `log`, not `clog`.
Applied to files:
pkg/vendor/model.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/vendor/model.gopkg/vendor/config.go
📚 Learning: 2025-04-04T02:03:21.906Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 1185
File: internal/exec/yaml_func_store.go:71-72
Timestamp: 2025-04-04T02:03:21.906Z
Learning: The codebase currently uses `log.Fatal` for error handling in library functions, which terminates the program. There is a plan to refactor this approach in a separate PR to improve API design by returning error messages instead of terminating execution.
Applied to files:
pkg/vendor/model.go
📚 Learning: 2025-02-19T05:50:35.853Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 1068
File: tests/snapshots/TestCLICommands_atmos_terraform_apply_--help.stdout.golden:0-0
Timestamp: 2025-02-19T05:50:35.853Z
Learning: Backtick formatting should only be applied to flag descriptions in Go source files, not in golden test files (test snapshots) as they are meant to capture the raw command output.
Applied to files:
pkg/vendor/model.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/vendor/model.gopkg/vendor/config.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/vendor/model.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:
pkg/vendor/model.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/vendor/model.go
📚 Learning: 2025-11-30T04:16:24.155Z
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.
Applied to files:
pkg/vendor/model.go
📚 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:
pkg/vendor/model.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 colors from pkg/ui/theme/colors.go for all UI output
Applied to files:
pkg/vendor/model.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/vendor/model.gopkg/vendor/config.go
📚 Learning: 2025-04-10T21:33:06.447Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 1147
File: pkg/filematch/filematch.go:32-32
Timestamp: 2025-04-10T21:33:06.447Z
Learning: The replacement of "*/*" with an empty string in recursive glob patterns (containing "**") is intentional behavior in the filematch package, as confirmed by the developer. This is necessary for proper handling of recursive directory matching when using the gobwas/glob library.
Applied to files:
pkg/vendor/pattern.go
📚 Learning: 2025-04-10T21:33:06.447Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 1147
File: pkg/filematch/filematch.go:32-32
Timestamp: 2025-04-10T21:33:06.447Z
Learning: The replacement of "*/*" with an empty string in recursive glob patterns (containing "**") is intentional behavior in the filematch package, as confirmed by the developer.
Applied to files:
pkg/vendor/pattern.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:
website/blog/2025-01-17-vendor-stack-flag.mdx
📚 Learning: 2025-12-13T06:07:37.766Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1686
File: docs/prd/tool-dependencies-integration.md:58-64
Timestamp: 2025-12-13T06:07:37.766Z
Learning: cloudposse/atmos: For PRD docs (docs/prd/*.md), markdownlint issues like MD040/MD010/MD034 can be handled in a separate documentation cleanup commit and should not block the current PR.
Applied to files:
website/blog/2025-01-17-vendor-stack-flag.mdx
📚 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 website/docs/cli/commands/**/*.mdx : All CLI commands/flags need Docusaurus documentation in website/docs/cli/commands/ with specific structure: frontmatter, Intro component, Screengrab component, Usage section, Arguments/Flags using <dl><dt>/<dd>, and Examples section
Applied to files:
website/blog/2025-01-17-vendor-stack-flag.mdx
📚 Learning: 2025-01-19T15:49:15.593Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 955
File: tests/snapshots/TestCLICommands_atmos_validate_editorconfig_--help.stdout.golden:0-0
Timestamp: 2025-01-19T15:49:15.593Z
Learning: In future commits, the help text for Atmos CLI commands should be limited to only show component and stack parameters for commands that actually use them. This applies to the example usage section in command help text.
Applied to files:
website/blog/2025-01-17-vendor-stack-flag.mdx
📚 Learning: 2025-10-10T23:51:36.597Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1599
File: internal/exec/terraform.go:394-402
Timestamp: 2025-10-10T23:51:36.597Z
Learning: In Atmos (internal/exec/terraform.go), when adding OpenTofu-specific flags like `--var-file` for `init`, do not gate them based on command name (e.g., checking if `info.Command == "tofu"` or `info.Command == "opentofu"`) because command names don't reliably indicate the actual binary being executed (symlinks, aliases). Instead, document the OpenTofu requirement in code comments and documentation, trusting users who enable the feature (e.g., `PassVars`) to ensure their terraform command points to an OpenTofu binary.
Applied to files:
website/blog/2025-01-17-vendor-stack-flag.mdx
📚 Learning: 2024-11-12T13:06:56.194Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 768
File: website/docs/cheatsheets/vendoring.mdx:70-70
Timestamp: 2024-11-12T13:06:56.194Z
Learning: In `atmos vendor pull --everything`, the `--everything` flag uses the TTY for TUI but is not interactive.
Applied to files:
website/blog/2025-01-17-vendor-stack-flag.mdx
📚 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/vendor/config.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/vendor/config.go
📚 Learning: 2025-04-11T22:06:46.999Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 1147
File: internal/exec/validate_schema.go:42-57
Timestamp: 2025-04-11T22:06:46.999Z
Learning: The "ExecuteAtmosValidateSchemaCmd" function in internal/exec/validate_schema.go has been reviewed and confirmed to have acceptable cognitive complexity despite static analysis warnings. The function uses a clean structure with only three if statements for error handling and delegates complex operations to helper methods.
Applied to files:
pkg/vendor/config.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/vendor/config.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/vendor/config.go
📚 Learning: 2025-02-03T06:00:11.419Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 959
File: cmd/describe_config.go:20-20
Timestamp: 2025-02-03T06:00:11.419Z
Learning: The `describe config` command should use `PrintErrorMarkdownAndExit` with empty title and suggestion for consistency with other commands.
Applied to files:
pkg/vendor/config.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/vendor/config.go
📚 Learning: 2024-11-30T22:07:08.610Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 810
File: internal/exec/yaml_func_terraform_output.go:35-40
Timestamp: 2024-11-30T22:07:08.610Z
Learning: In the Go function `processTagTerraformOutput` in `internal/exec/yaml_func_terraform_output.go`, parameters cannot contain spaces. The code splits the input by spaces, and if the parameters contain spaces, `len(parts) != 3` will fail and show an error to the user.
Applied to files:
pkg/vendor/config.go
📚 Learning: 2024-12-12T15:17:45.245Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 808
File: examples/demo-atmos.d/atmos.d/tools/helmfile.yml:10-10
Timestamp: 2024-12-12T15:17:45.245Z
Learning: In `examples/demo-atmos.d/atmos.d/tools/helmfile.yml`, when suggesting changes to `kubeconfig_path`, ensure that the values use valid Go template syntax.
Applied to files:
pkg/vendor/config.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:
pkg/vendor/config.go
🧬 Code graph analysis (3)
pkg/vendor/stack.go (6)
pkg/perf/perf.go (1)
Track(121-138)errors/errors.go (1)
ErrStackNotFound(467-467)pkg/config/const.go (4)
TerraformComponentType(57-57)HelmfileComponentType(58-58)PackerComponentType(59-59)ComponentVendorConfigFileName(61-61)pkg/logger/log.go (1)
Debug(24-26)pkg/utils/yaml_utils.go (1)
UnmarshalYAML(687-689)pkg/schema/vendor_component.go (2)
VendorComponentConfig(30-35)VendorComponentSpec(20-23)
pkg/vendor/pattern.go (2)
pkg/logger/log.go (1)
Debug(24-26)pkg/utils/glob_utils.go (1)
PathMatch(74-106)
pkg/vendor/config.go (7)
pkg/logger/log.go (1)
Debug(24-26)pkg/vendor/errors.go (15)
ErrVendoringNotConfigured(25-25)ErrPermissionDenied(26-26)ErrNoYAMLConfigFiles(29-29)ErrDuplicateComponentsFound(32-32)ErrDuplicateImport(31-31)ErrVendorConfigSelfImport(21-21)ErrMissingVendorConfigDefinition(22-22)ErrSourceMissing(19-19)ErrTargetsMissing(20-20)ErrNoComponentsWithTags(28-28)ErrDuplicateComponents(30-30)ErrComponentNotDefined(33-33)ErrFolderNotFound(44-44)ErrInvalidComponentKind(45-45)ErrComponentConfigFileNotFound(43-43)pkg/utils/slice_utils.go (1)
SliceContainsString(12-21)pkg/schema/vendor_component.go (1)
VendorComponentConfig(30-35)pkg/config/const.go (4)
TerraformComponentType(57-57)HelmfileComponentType(58-58)PackerComponentType(59-59)ComponentVendorConfigFileName(61-61)pkg/utils/file_utils.go (2)
IsDirectory(20-28)FileExists(31-39)pkg/utils/yaml_utils.go (1)
UnmarshalYAML(687-689)
⏰ 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). (2)
- GitHub Check: Acceptance Tests (windows)
- GitHub Check: Summary
🔇 Additional comments (14)
pkg/vendor/uri.go (3)
1-10: Imports and package structure look good.Clean organization with stdlib first, 3rd-party next, and Atmos packages last. The logger alias
logfollows project conventions.
274-299: Normalization logic is sound.The function correctly prioritizes special URI handling before applying triple-slash normalization and double-slash-dot appending. The early returns for file/OCI/S3/local/archive URIs prevent unintended modifications.
301-330: Triple-slash normalization handles edge cases well.Good use of go-getter's
SourceDirSubdirfor robust parsing. The separation of query parameters before reconstruction prevents malformed URIs.pkg/vendor/config.go (2)
107-144: Merge logic is solid.Good use of maps for O(1) duplicate detection. The error messages include the config file path for easier debugging.
146-192: Recursive import handling is well-designed.The chain-based duplicate detection prevents import cycles. Setting
Fileon each source provides good traceability for debugging vendor issues.pkg/vendor/stack.go (3)
1-16: Imports are well-organized.Three groups (stdlib, 3rd-party, Atmos) with correct aliases. Good to see
perfincluded for tracking.
18-65: Stack vendoring orchestration looks correct.Good flow: describe stacks → extract vendorable components → execute vendor model. The empty stack handling with skipped count logging is helpful for debugging.
221-273: Package creation logic is clean.Good separation between component and mixin package creation. The early return for empty URI prevents unnecessary processing.
pkg/vendor/model.go (3)
56-85: TUI model execution is well-structured.Good TTY detection fallback, clear error propagation, and the failed package count check ensures proper exit status.
456-511: Component installation logic is correct.Clean separation between dry-run and real installation paths. The fallback error for unknown package types provides a safety net.
532-575: Skip function generation is well-organized.Good structure: .git exclusion first, then include/exclude pattern processing. The comment at lines 545-549 clearly explains the logic.
pkg/vendor/pattern.go (3)
1-19: Clean constants and imports.Good practice defining log keys as constants for consistency across debug messages.
21-29: Clean routing between file and directory handling.Simple dispatch pattern that keeps the complexity in the specialized functions.
309-316: File detection heuristic may have edge cases.The dot-based file detection (
!strings.Contains(part, ".")) would exclude legitimate directory names containing dots (e.g.,v1.2.3). For pattern extraction purposes this is likely acceptable since such directories would still be traversed via the permissivecheckDoublestarPrefixPattern.Consider adding a test case for directory names with dots to confirm expected behavior.
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/vendor/component.go (1)
344-346: Capitalize error message.🔎 Suggested fix:
- if err := cp.Copy(tempDir, p.componentPath, copyOptions); err != nil { - return fmt.Errorf("Failed to copy package %s error %w", p.name, err) + if err := cp.Copy(tempDir, p.componentPath, copyOptions); err != nil { + return fmt.Errorf("failed to copy package %s: %w", p.name, err) }
♻️ Duplicate comments (2)
pkg/vendor/config.go (2)
28-58: Missing perf.Track on public function.Per coding guidelines, public functions need performance tracking.
🔎 Suggested fix:
+import ( + ... + "github.com/cloudposse/atmos/pkg/perf" + ... +) func ReadAndProcessVendorConfigFile( atmosConfig *schema.AtmosConfiguration, vendorConfigFile string, checkGlobalConfig bool, ) (VendorConfigResult, error) { + defer perf.Track(atmosConfig, "vendor.ReadAndProcessVendorConfigFile")() + result := VendorConfigResult{}
300-333: Missing perf.Track on public function.Same issue as
ReadAndProcessVendorConfigFile.🔎 Suggested fix:
func ReadAndProcessComponentVendorConfigFile( atmosConfig *schema.AtmosConfiguration, component string, componentType string, ) (schema.VendorComponentConfig, string, error) { + defer perf.Track(atmosConfig, "vendor.ReadAndProcessComponentVendorConfigFile")() + var componentConfig schema.VendorComponentConfig
🧹 Nitpick comments (13)
pkg/vender/vendor_config_test.go (1)
32-33: Consider usingrequire.NoErrorfor error assertions.When an error would invalidate subsequent assertions,
require.NoErrorfails fast and provides clearer test output. Currently usingassert.Nil(t, err)in multiple places.🔎 Suggested pattern:
-err := os.MkdirAll(componentPath, 0o755) -assert.Nil(t, err) +err := os.MkdirAll(componentPath, 0o755) +require.NoError(t, err)Also applies to: 51-52, 88-89, 130-131
pkg/vendor/model.go (2)
60-73: Missingperf.Trackon public function.Per coding guidelines, public functions should include performance tracking. This function orchestrates the TUI model execution.
🔎 Add performance tracking:
func executeVendorModel[T pkgComponentVendor | pkgAtmosVendor]( packages []T, dryRun bool, atmosConfig *schema.AtmosConfiguration, ) error { + defer perf.Track(atmosConfig, "vendor.executeVendorModel")() + if len(packages) == 0 { return nil }
97-147: Missingperf.Trackon public functionnewModelVendor.Same guideline applies here. Add tracking at function entry.
🔎 Add performance tracking:
func newModelVendor[T pkgComponentVendor | pkgAtmosVendor]( pkgs []T, dryRun bool, atmosConfig *schema.AtmosConfiguration, ) (modelVendor, error) { + defer perf.Track(atmosConfig, "vendor.newModelVendor")() + p := progress.New(pkg/vendor/vendor_test.go (1)
444-489: Consider removing unused local file setup.Lines 448-451 create a local file that isn't used by any test case. The
localFilevariable is written but never referenced in the test table.🔎 Remove unused file creation:
func TestDetermineSourceType_Extended(t *testing.T) { // Additional test cases for determineSourceType. - tempDir := t.TempDir() - - // Create a local file for testing. - localFile := tempDir + "/local.tf" - err := os.WriteFile(localFile, []byte("# test"), 0o644) - assert.NoError(t, err) - tests := []struct {pkg/vendor/model_test.go (2)
279-301: This test is tautological and doesn't test actual behavior.Lines 289-300 just verify string comparisons (
key == "ctrl+c") rather than testinghandleKeyPress. The actual behavior test is at lines 655-697 (TestHandleKeyPress_QuitKeys), making this test redundant.🔎 Consider removing or refactoring:
-func TestModelVendor_HandleKeyPress(t *testing.T) { - model := &modelVendor{ - packages: []pkgVendor{{name: "test"}}, - } - - // Verify model is properly initialized for key handling. - assert.NotNil(t, model) - assert.Equal(t, "test", model.packages[0].name) - - // Test quit key detection logic (the handleKeyPress function checks these keys). - quitKeys := []string{"ctrl+c", "esc", "q"} - for _, key := range quitKeys { - isQuitKey := key == "ctrl+c" || key == "esc" || key == "q" - assert.True(t, isQuitKey, "Key %s should be a quit key", key) - } - - // Non-quit keys should not trigger quit. - nonQuitKeys := []string{"x", "a", "enter", "space"} - for _, key := range nonQuitKeys { - isQuitKey := key == "ctrl+c" || key == "esc" || key == "q" - assert.False(t, isQuitKey, "Key %s should not be a quit key", key) - } -}
261-267: Consider refactoring to use Go's builtinmaxfunction.Go 1.21 and later versions include a builtin
maxfunction, and the project uses Go 1.25.2. While the custommax(a, b int)inpkg/vendor/model.gois actively used and functioning correctly, migrating to the builtin would reduce maintenance overhead and align with standard Go practices.pkg/vendor/stack_test.go (1)
104-230: Component extraction tests look good, butsetupComponentfield is unused.The
setupComponentfield in the test struct (line 141) is never checked or used in the test logic. Consider removing it or implementing the conditional setup.🔎 Either use or remove the field:
tests := []struct { name string stacksMap map[string]any expectedCount int expectedSkip int expectError bool - setupComponent bool }{pkg/vendor/stack.go (2)
1-16: Import organization issue.Per coding guidelines, imports should be in three groups: stdlib, 3rd-party, Atmos packages. Here,
errors(Atmos package) is mixed before 3rd-party imports would normally go. Since there are no 3rd-party imports, it's acceptable, but the order within Atmos packages should be alphabetical.🔎 Alphabetize Atmos imports:
import ( "fmt" "os" "path/filepath" "strings" - "github.com/cloudposse/atmos/errors" - "github.com/cloudposse/atmos/internal/exec" cfg "github.com/cloudposse/atmos/pkg/config" + "github.com/cloudposse/atmos/errors" + "github.com/cloudposse/atmos/internal/exec" log "github.com/cloudposse/atmos/pkg/logger" "github.com/cloudposse/atmos/pkg/perf" "github.com/cloudposse/atmos/pkg/schema" u "github.com/cloudposse/atmos/pkg/utils" )
193-265: Implementation is clean with one note.
createComponentPackageshas unused parameters (_ *schema.AtmosConfiguration,_ stringfor componentType). If these aren't needed for future expansion, consider removing them to simplify the signature.pkg/vendor/vendor.go (3)
110-112: Use available atmosConfig instead of nil for perf.Track.
params.atmosConfigis available but you're passingnil. This loses context for performance tracking.🔎 Suggested fix:
func executeAtmosVendorInternal(params *executeVendorOptions) error { - defer perf.Track(nil, "vendor.executeAtmosVendorInternal")() + defer perf.Track(params.atmosConfig, "vendor.executeAtmosVendorInternal")()
256-284: determineSourceType mutates the uri pointer in-place.This side effect could be surprising. Consider returning the modified URI as part of the result struct instead. However, I see the pattern is intentional to avoid multiple returns.
313-335: HandleVendorConfigNotExist duplicates VendorComponent logic.This function does the same thing as
VendorComponent(lines 69-96). Consider consolidating to avoid drift.🔎 Suggested refactor:
func HandleVendorConfigNotExist(atmosConfig *schema.AtmosConfiguration, component, componentType string, dryRun bool) error { if componentType == "" { componentType = "terraform" } - - config, path, err := ReadAndProcessComponentVendorConfigFile( - atmosConfig, - component, - componentType, - ) - if err != nil { - return err - } - - return ExecuteComponentVendorInternal( - atmosConfig, - &config.Spec, - component, - path, - dryRun, - ) + return VendorComponent(atmosConfig, component, + WithComponentDryRun(dryRun), + WithComponentComponentType(componentType), + ) }pkg/vendor/component.go (1)
294-299: Inconsistent temp directory creation.
installMixinusesos.MkdirTempdirectly instead ofcreateTempDirhelper. This skips the permission hardening done increateTempDir.🔎 Suggested fix:
func installMixin(p *pkgComponentVendor, atmosConfig *schema.AtmosConfiguration) error { - tempDir, err := os.MkdirTemp("", "atmos-vendor-mixin") + tempDir, err := createTempDir() if err != nil { - return fmt.Errorf("Failed to create temp directory %w", err) + return fmt.Errorf("failed to create temp directory: %w", err) }
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (2)
pkg/vendor/config.go (2)
21-58: Missing performance tracking on public function.Per coding guidelines,
ReadAndProcessVendorConfigFileshould havedefer perf.Track(atmosConfig, "vendor.ReadAndProcessVendorConfigFile")()at the start.🔎 Suggested fix:
func ReadAndProcessVendorConfigFile( atmosConfig *schema.AtmosConfiguration, vendorConfigFile string, checkGlobalConfig bool, ) (VendorConfigResult, error) { + defer perf.Track(atmosConfig, "vendor.ReadAndProcessVendorConfigFile")() + result := VendorConfigResult{}
300-333: Missing performance tracking on public function.Same as above -
ReadAndProcessComponentVendorConfigFileneedsdefer perf.Track(atmosConfig, "vendor.ReadAndProcessComponentVendorConfigFile")().🔎 Suggested fix:
func ReadAndProcessComponentVendorConfigFile( atmosConfig *schema.AtmosConfiguration, component string, componentType string, ) (schema.VendorComponentConfig, string, error) { + defer perf.Track(atmosConfig, "vendor.ReadAndProcessComponentVendorConfigFile")() + var componentConfig schema.VendorComponentConfig
🧹 Nitpick comments (8)
cmd/vendor/pull_test.go (1)
157-177: Consider consolidating edge case tests into the main table.The
TestParsePullOptions_EmptyTagsandTestParsePullOptions_SingleTagtests could be incorporated as additional rows in the main table-driven test to reduce duplication.🔎 Suggested consolidation:
{ name: "all options set", viperSetup: func(v *viper.Viper) { v.Set("component", "vpc") v.Set("tags", "core,networking") v.Set("dry-run", true) v.Set("type", "terraform") }, expectedOpts: &PullOptions{ Component: "vpc", Stack: "", Tags: []string{"core", "networking"}, DryRun: true, Everything: false, ComponentType: "terraform", }, }, + { + name: "empty tags string yields nil", + viperSetup: func(v *viper.Viper) { + v.Set("tags", "") + }, + expectedOpts: &PullOptions{ + Tags: nil, + }, + }, + { + name: "single tag", + viperSetup: func(v *viper.Viper) { + v.Set("tags", "networking") + }, + expectedOpts: &PullOptions{ + Tags: []string{"networking"}, + }, + },pkg/vendor/vendor_test.go (1)
444-489: Unused local file created in TestDetermineSourceType_Extended.The
localFilevariable at line 449-451 is created but never used in the test cases. Consider removing it or adding a test case that uses it.🔎 Either remove unused file or add a test case:
func TestDetermineSourceType_Extended(t *testing.T) { // Additional test cases for determineSourceType. - tempDir := t.TempDir() - - // Create a local file for testing. - localFile := tempDir + "/local.tf" - err := os.WriteFile(localFile, []byte("# test"), 0o644) - assert.NoError(t, err) tests := []struct {pkg/vendor/stack.go (1)
126-138: Consider returning error for unknown component type instead of defaulting.
getComponentBasePathsilently defaults to terraform for unknown types. This could mask configuration issues.🔎 Alternative with explicit error handling:
func getComponentBasePath(atmosConfig *schema.AtmosConfiguration, componentType string) string { switch componentType { case cfg.TerraformComponentType: return atmosConfig.Components.Terraform.BasePath case cfg.HelmfileComponentType: return atmosConfig.Components.Helmfile.BasePath case cfg.PackerComponentType: return atmosConfig.Components.Packer.BasePath default: + log.Debug("Unknown component type, defaulting to terraform", "type", componentType) return atmosConfig.Components.Terraform.BasePath } }At minimum, a debug log would help troubleshooting.
pkg/vendor/config_test.go (1)
279-301: Consider testing actualhandleKeyPressbehavior instead of duplicating logic.The test at lines 289-300 manually checks if keys match quit conditions rather than invoking
handleKeyPress. This tests string comparison logic you wrote in the test, not the actual function behavior.🔎 Consider this approach:
func TestModelVendor_HandleKeyPress(t *testing.T) { model := &modelVendor{ packages: []pkgVendor{{name: "test"}}, } - // Verify model is properly initialized for key handling. - assert.NotNil(t, model) - assert.Equal(t, "test", model.packages[0].name) - - // Test quit key detection logic (the handleKeyPress function checks these keys). - quitKeys := []string{"ctrl+c", "esc", "q"} - for _, key := range quitKeys { - isQuitKey := key == "ctrl+c" || key == "esc" || key == "q" - assert.True(t, isQuitKey, "Key %s should be a quit key", key) - } - - // Non-quit keys should not trigger quit. - nonQuitKeys := []string{"x", "a", "enter", "space"} - for _, key := range nonQuitKeys { - isQuitKey := key == "ctrl+c" || key == "esc" || key == "q" - assert.False(t, isQuitKey, "Key %s should not be a quit key", key) - } + // Test quit keys return a command + cmd := model.handleKeyPress(tea.KeyMsg{Type: tea.KeyCtrlC}) + assert.NotNil(t, cmd, "ctrl+c should return quit command") + + // Test non-quit keys return nil + cmd = model.handleKeyPress(tea.KeyMsg{Type: tea.KeyRunes, Runes: []rune("x")}) + assert.Nil(t, cmd, "x should not return a command") }Note:
TestHandleKeyPress_QuitKeysat line 655 already covers this properly, so this test could be removed or simplified.pkg/vendor/model.go (1)
295-300: Custommaxfunction may be redundant with Go 1.21+.Go 1.21 introduced builtin
max. If Go 1.21+ is required, this can be removed. If supporting older versions, add a build constraint or comment.🔎 If Go 1.21+ is required:
-func max(a, b int) int { - if a > b { - return a - } - return b -}The builtin
maxhandles this automatically.pkg/vendor/component.go (1)
44-49: Consider returning error fromremoveTempDirfor critical cleanup.Currently logs a warning on failure. For temp directories with potentially sensitive data, consider if callers need to know cleanup failed.
The current approach is acceptable for non-sensitive temp data.
pkg/vendor/vendor.go (2)
3-17: Import organization needs adjustment.Per guidelines, imports should be: 1) stdlib, 2) 3rd-party, 3) Atmos packages. The
internal/execimport at line 16 is an Atmos package but placed after the other Atmos packages.🔎 Reorder imports:
import ( "fmt" "net/url" "path/filepath" "strings" "github.com/samber/lo" + "github.com/cloudposse/atmos/internal/exec" cfg "github.com/cloudposse/atmos/pkg/config" "github.com/cloudposse/atmos/pkg/perf" "github.com/cloudposse/atmos/pkg/schema" u "github.com/cloudposse/atmos/pkg/utils" - - "github.com/cloudposse/atmos/internal/exec" )
110-157: Consider usingparams.atmosConfigin perf.Track.Line 112 passes
niltoperf.Track. Sinceparams.atmosConfigis available, using it would provide better context for profiling.🔎 Use atmosConfig for tracking:
func executeAtmosVendorInternal(params *executeVendorOptions) error { - defer perf.Track(nil, "vendor.executeAtmosVendorInternal")() + defer perf.Track(params.atmosConfig, "vendor.executeAtmosVendorInternal")()
|
💥 This pull request now has conflicts. Could you fix it @osterman? 🙏 |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@cmd/vendor/pull.go`:
- Around line 89-110: Replace the raw fmt.Errorf wrapping of the
cfg.InitCliConfig error with the sentinel ErrInitializingCLIConfig so callers
can detect the failure; in the block that calls cfg.InitCliConfig (around
atmosConfigPtr/atmosConfig handling) return the error combined with
errors.ErrInitializingCLIConfig (using fmt.Errorf with %w or errors.Join)
instead of fmt.Errorf("failed to initialize CLI config: %w", err) so the
InitCliConfig failure is wrapped by the sentinel error.
In `@cmd/vendor/vendor.go`:
- Around line 41-80: The VendorCommandProvider struct is missing the
IsExperimental method required by the CommandProvider interface, causing the
build failure; implement a method func (v *VendorCommandProvider)
IsExperimental() bool on VendorCommandProvider that returns false (or true only
if this command is experimental) so the type satisfies the interface and builds
successfully.
📜 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 (17)
CLAUDE.mdcmd/root.gocmd/vendor.gocmd/vendor/diff.gocmd/vendor/pull.gocmd/vendor/pull_test.gocmd/vendor/vendor.gocmd/vendor_diff.gocmd/vendor_pull.gocmd/vendor_test.goerrors/errors.gointernal/exec/oci_utils.gointernal/exec/oci_utils_test.gointernal/exec/template_processing_test.gointernal/exec/template_utils.gointernal/exec/vendor.gointernal/exec/vendor_utils.go
💤 Files with no reviewable changes (6)
- cmd/vendor_diff.go
- internal/exec/vendor.go
- internal/exec/vendor_utils.go
- cmd/vendor_test.go
- cmd/vendor_pull.go
- cmd/vendor.go
✅ Files skipped from review due to trivial changes (1)
- CLAUDE.md
🚧 Files skipped from review as they are similar to previous changes (4)
- errors/errors.go
- internal/exec/oci_utils_test.go
- cmd/vendor/pull_test.go
- cmd/vendor/diff.go
🧰 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:
internal/exec/oci_utils.gocmd/vendor/pull.gointernal/exec/template_utils.gocmd/root.gointernal/exec/template_processing_test.gocmd/vendor/vendor.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:
internal/exec/oci_utils.gocmd/vendor/pull.gointernal/exec/template_utils.gocmd/root.gointernal/exec/template_processing_test.gocmd/vendor/vendor.go
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
Files:
cmd/vendor/pull.gocmd/root.gocmd/vendor/vendor.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:
internal/exec/template_processing_test.go
🧠 Learnings (39)
📓 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: 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.
Learnt from: osterman
Repo: cloudposse/atmos PR: 1686
File: docs/prd/tool-dependencies-integration.md:58-64
Timestamp: 2025-12-13T06:07:37.766Z
Learning: cloudposse/atmos: For PRD docs (docs/prd/*.md), markdownlint issues like MD040/MD010/MD034 can be handled in a separate documentation cleanup commit and should not block the current PR.
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.
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: .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
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.
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 : Provide comprehensive help text for all commands and flags, include examples in command help, and follow Go's documentation conventions in Cobra command definitions
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
Learnt from: Listener430
Repo: cloudposse/atmos PR: 825
File: internal/exec/helmfile_generate_varfile.go:28-31
Timestamp: 2024-12-07T16:16:13.038Z
Learning: In `internal/exec/helmfile_generate_varfile.go`, the `--help` command (`./atmos helmfile generate varfile --help`) works correctly without requiring stack configurations, and the only change needed was to make `ProcessCommandLineArgs` exportable by capitalizing its name.
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.mod,go.sum} : Manage dependencies with Go modules and keep dependencies up to date while minimizing external dependencies
📚 Learning: 2026-01-04T00:55:21.720Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T00:55:21.720Z
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:
internal/exec/oci_utils.gointernal/exec/template_utils.gocmd/root.gocmd/vendor/vendor.go
📚 Learning: 2025-10-13T18:13:54.020Z
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.
Applied to files:
internal/exec/oci_utils.go
📚 Learning: 2025-11-30T04:16:24.155Z
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.
Applied to files:
internal/exec/oci_utils.gointernal/exec/template_utils.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:
internal/exec/oci_utils.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:
internal/exec/oci_utils.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:
internal/exec/oci_utils.gointernal/exec/template_utils.gocmd/root.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:
internal/exec/oci_utils.gocmd/vendor/pull.gointernal/exec/template_utils.gocmd/root.gointernal/exec/template_processing_test.gocmd/vendor/vendor.go
📚 Learning: 2025-11-30T04:16:01.899Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 1821
File: pkg/merge/deferred.go:50-59
Timestamp: 2025-11-30T04:16:01.899Z
Learning: In the cloudposse/atmos repository, performance tracking with `defer perf.Track()` should NOT be added to trivial O(1) getter methods that only return field references or check map lengths (e.g., `GetDeferredValues()`, `HasDeferredValues()`). The guideline to add perf tracking to "all public functions" applies to functions that do meaningful work (I/O, computation, external calls), not to trivial accessors where the tracking overhead would exceed the operation time and pollute performance reports.
Applied to files:
internal/exec/oci_utils.gointernal/exec/template_utils.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:
internal/exec/oci_utils.gocmd/vendor/pull.gointernal/exec/template_utils.gocmd/root.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:
internal/exec/oci_utils.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 : Provide comprehensive help text for all commands and flags, include examples in command help, and follow Go's documentation conventions in Cobra command definitions
Applied to files:
cmd/vendor/pull.gocmd/vendor/vendor.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/vendor/pull.gointernal/exec/template_utils.gocmd/root.gocmd/vendor/vendor.go
📚 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/vendor/pull.gocmd/root.gocmd/vendor/vendor.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/vendor/pull.gointernal/exec/template_utils.gocmd/root.gocmd/vendor/vendor.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:
cmd/vendor/pull.gocmd/root.gocmd/vendor/vendor.go
📚 Learning: 2025-10-10T23:51:36.597Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1599
File: internal/exec/terraform.go:394-402
Timestamp: 2025-10-10T23:51:36.597Z
Learning: In Atmos (internal/exec/terraform.go), when adding OpenTofu-specific flags like `--var-file` for `init`, do not gate them based on command name (e.g., checking if `info.Command == "tofu"` or `info.Command == "opentofu"`) because command names don't reliably indicate the actual binary being executed (symlinks, aliases). Instead, document the OpenTofu requirement in code comments and documentation, trusting users who enable the feature (e.g., `PassVars`) to ensure their terraform command points to an OpenTofu binary.
Applied to files:
cmd/vendor/pull.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:
cmd/vendor/pull.gocmd/root.gocmd/vendor/vendor.go
📚 Learning: 2024-12-07T16:16:13.038Z
Learnt from: Listener430
Repo: cloudposse/atmos PR: 825
File: internal/exec/helmfile_generate_varfile.go:28-31
Timestamp: 2024-12-07T16:16:13.038Z
Learning: In `internal/exec/helmfile_generate_varfile.go`, the `--help` command (`./atmos helmfile generate varfile --help`) works correctly without requiring stack configurations, and the only change needed was to make `ProcessCommandLineArgs` exportable by capitalizing its name.
Applied to files:
cmd/vendor/pull.gocmd/root.gocmd/vendor/vendor.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:
cmd/vendor/pull.go
📚 Learning: 2025-11-10T03:03:31.505Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 0
File: :0-0
Timestamp: 2025-11-10T03:03:31.505Z
Learning: In the Atmos codebase, commands using the `StandardParser` flag pattern (from pkg/flags) do NOT need explicit `viper.BindPFlag()` calls in their code. The StandardParser encapsulates flag binding internally: flags are registered via `parser.RegisterFlags(cmd)` in init(), and bound via `parser.BindFlagsToViper(cmd, v)` in RunE, which internally calls viper.BindPFlag for each flag. This pattern is used throughout Atmos (e.g., cmd/toolchain/get.go, cmd/toolchain/info.go, cmd/toolchain/install.go, cmd/toolchain/path.go). Do not flag missing viper.BindPFlag calls when StandardParser is used.
Applied to files:
cmd/vendor/pull.gocmd/vendor/vendor.go
📚 Learning: 2026-01-04T00:55:21.720Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T00:55:21.720Z
Learning: Commands MUST use `flags.NewStandardParser()` for command-specific flags and NEVER call `viper.BindEnv()` or `viper.BindPFlag()` directly
Applied to files:
cmd/vendor/pull.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/vendor/pull.gocmd/root.gocmd/vendor/vendor.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:
cmd/vendor/pull.gocmd/vendor/vendor.go
📚 Learning: 2026-01-04T00:55:21.720Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T00:55:21.720Z
Learning: New config templates support Go templating with `FuncMap()` from `internal/exec/template_funcs.go`
Applied to files:
internal/exec/template_utils.gointernal/exec/template_processing_test.go
📚 Learning: 2025-01-25T03:49:03.951Z
Learnt from: Listener430
Repo: cloudposse/atmos PR: 934
File: internal/exec/template_utils.go:268-271
Timestamp: 2025-01-25T03:49:03.951Z
Learning: The `ProcessTmplWithDatasourcesGomplate` function in `internal/exec/template_utils.go` is used for documentation generation purposes, where simple environment variable handling is acceptable and thread-safety concerns are not critical.
Applied to files:
internal/exec/template_utils.go
📚 Learning: 2025-04-11T22:06:46.999Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 1147
File: internal/exec/validate_schema.go:42-57
Timestamp: 2025-04-11T22:06:46.999Z
Learning: The "ExecuteAtmosValidateSchemaCmd" function in internal/exec/validate_schema.go has been reviewed and confirmed to have acceptable cognitive complexity despite static analysis warnings. The function uses a clean structure with only three if statements for error handling and delegates complex operations to helper methods.
Applied to files:
cmd/root.go
📚 Learning: 2026-01-04T00:55:21.720Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T00:55:21.720Z
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:
cmd/root.gocmd/vendor/vendor.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:
cmd/root.gocmd/vendor/vendor.go
📚 Learning: 2025-10-07T00:25:16.333Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1498
File: website/src/components/Screengrabs/atmos-terraform-metadata--help.html:25-55
Timestamp: 2025-10-07T00:25:16.333Z
Learning: In Atmos CLI, subcommands inherit flags from their parent commands via Cobra's command inheritance. For example, `atmos terraform metadata --help` shows `--affected` and related flags inherited from the parent `terraform` command (defined in cmd/terraform.go), even though the metadata subcommand doesn't explicitly define these flags. This is expected Cobra behavior and auto-generated help screengrabs accurately reflect this inheritance.
Applied to files:
cmd/root.gocmd/vendor/vendor.go
📚 Learning: 2025-01-09T22:37:01.004Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 914
File: cmd/terraform_commands.go:260-265
Timestamp: 2025-01-09T22:37:01.004Z
Learning: In the terraform commands implementation (cmd/terraform_commands.go), the direct use of `os.Args[2:]` for argument handling is intentionally preserved to avoid extensive refactoring. While it could be improved to use cobra's argument parsing, such changes should be handled in a dedicated PR to maintain focus and minimize risk.
Applied to files:
cmd/root.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:
cmd/root.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:
cmd/root.go
📚 Learning: 2025-01-30T19:30:59.120Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 959
File: cmd/workflow.go:74-74
Timestamp: 2025-01-30T19:30:59.120Z
Learning: Error handling for `cmd.Usage()` is not required in the Atmos CLI codebase, as confirmed by the maintainer.
Applied to files:
cmd/root.go
📚 Learning: 2025-09-05T14:57:37.360Z
Learnt from: RoseSecurity
Repo: cloudposse/atmos PR: 1448
File: cmd/ansible.go:26-28
Timestamp: 2025-09-05T14:57:37.360Z
Learning: The Atmos codebase uses a consistent pattern for commands that delegate to external tools: `PersistentFlags().Bool("", false, doubleDashHint)` where doubleDashHint provides help text about using double dashes to separate Atmos options from native command arguments. This pattern is used across terraform, packer, helmfile, atlantis, aws, and ansible commands.
Applied to files:
cmd/root.go
📚 Learning: 2025-08-29T20:57:35.423Z
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.
Applied to files:
cmd/root.gocmd/vendor/vendor.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-09-29T02:20:11.636Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 1540
File: internal/exec/validate_component.go:117-118
Timestamp: 2025-09-29T02:20:11.636Z
Learning: The ValidateComponent function in internal/exec/validate_component.go had its componentSection parameter type refined from `any` to `map[string]any` without adding new parameters. This is a type safety improvement, not a signature change requiring call site updates.
Applied to files:
internal/exec/template_processing_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:
cmd/vendor/vendor.go
🧬 Code graph analysis (4)
internal/exec/template_utils.go (3)
internal/exec/template_funcs.go (1)
FuncMap(21-34)pkg/perf/perf.go (1)
Track(121-138)pkg/schema/schema.go (1)
ConfigAndStacksInfo(732-829)
cmd/root.go (5)
cmd/vendor/vendor.go (1)
SetAtmosConfig(17-19)cmd/terraform/backend/backend.go (1)
SetAtmosConfig(14-16)cmd/terraform/workdir/workdir.go (1)
SetAtmosConfig(17-19)cmd/theme/theme.go (1)
SetAtmosConfig(17-19)cmd/version/version.go (1)
SetAtmosConfig(32-34)
internal/exec/template_processing_test.go (1)
internal/exec/template_utils.go (1)
GetSprigFuncMap(45-52)
cmd/vendor/vendor.go (4)
pkg/schema/schema.go (1)
AtmosConfiguration(54-102)pkg/flags/positional_args_builder.go (1)
PositionalArgsBuilder(52-54)pkg/flags/compat/compatibility_flags.go (1)
CompatibilityFlag(33-37)cmd/internal/command.go (1)
CommandAlias(11-31)
🪛 GitHub Actions: Tests
cmd/vendor/vendor.go
[error] 38-38: cannot use &VendorCommandProvider{} as "github.com/cloudposse/atmos/cmd/internal".CommandProvider value in argument to internal.Register: *VendorCommandProvider does not implement "github.com/cloudposse/atmos/cmd/internal".CommandProvider (missing method IsExperimental)
🪛 GitHub Check: Build (linux)
cmd/vendor/vendor.go
[failure] 38-38:
cannot use &VendorCommandProvider{} (value of type *VendorCommandProvider) as "github.com/cloudposse/atmos/cmd/internal".CommandProvider value in argument to internal.Register: *VendorCommandProvider does not implement "github.com/cloudposse/atmos/cmd/internal".CommandProvider (missing method IsExperimental)
🪛 GitHub Check: Build (macos)
cmd/vendor/vendor.go
[failure] 38-38:
cannot use &VendorCommandProvider{} (value of type *VendorCommandProvider) as "github.com/cloudposse/atmos/cmd/internal".CommandProvider value in argument to internal.Register: *VendorCommandProvider does not implement "github.com/cloudposse/atmos/cmd/internal".CommandProvider (missing method IsExperimental)
🪛 GitHub Check: Build (windows)
cmd/vendor/vendor.go
[failure] 38-38:
cannot use &VendorCommandProvider{} (value of type *VendorCommandProvider) as "github.com/cloudposse/atmos/cmd/internal".CommandProvider value in argument to internal.Register: *VendorCommandProvider does not implement "github.com/cloudposse/atmos/cmd/internal".CommandProvider (missing method IsExperimental)
⏰ 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). (1)
- GitHub Check: Summary
🔇 Additional comments (14)
internal/exec/oci_utils.go (2)
21-21: LGTM!perf import correctly placed in the Atmos packages group.
33-38: LGTM!Clean export of
ProcessOciImagewith proper perf instrumentation. The thin wrapper pattern keeps the implementation testable viaprocessOciImageWithFSwhile exposing the public API with tracking. Matches the coding guideline format exactly.internal/exec/template_processing_test.go (3)
323-333: Good alignment with exportedGetSprigFuncMap.
Tests now use the public API while preserving intent.
373-380: Concurrency test update looks correct.
Using the exported cache accessor keeps the thread-safety test in sync with production usage.
471-478: Benchmark update is consistent with the refactor.
Benchmarks now exercise the public cache accessor directly.internal/exec/template_utils.go (3)
41-52: Exported cache accessor is well-documented and consistent.
Clear docstring and safe caching behavior retained.
67-73: Template function map uses the new cache accessor cleanly.
Keeps the combined func map logic unchanged while aligning with the exported API.
182-186: Sprig function map reuse looks good.
Reuses the cached map as intended inside the datasources path.cmd/root.go (1)
1367-1376: Nice: vendor config is wired into root execution.
This keeps the vendor command aligned with other subcommands that receive the initialized config before RunE.cmd/vendor/vendor.go (1)
21-39: LGTM: vendor command is registered via the provider pattern.
Clear wiring and registry registration.cmd/vendor/pull.go (4)
23-31: PullOptions struct is clear and scoped well.
Good separation of global vs command-specific flags.
33-54: Help text and examples are solid.
The command usage reads well and the examples cover key cases, including--stack.
124-142: parsePullOptions is clean and consistent.
Straightforward mapping from Viper to options.
145-167: Parser setup looks good.
StandardParser usage and env var bindings align with the command pattern.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
|
💥 This pull request now has conflicts. Could you fix it @osterman? 🙏 |
f1e6463 to
e5a4f5c
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@cmd/vendor/diff.go`:
- Around line 18-22: The RunE in vendor.diff uses an undefined package-level
variable atmosConfigPtr; add a package-level variable and setter in the vendor
package (e.g., declare var atmosConfigPtr *schema.AtmosConfiguration and
implement SetAtmosConfig(cfg *schema.AtmosConfiguration)) so RunE can reference
atmosConfigPtr, and ensure the application initialization calls
vendor.SetAtmosConfig(...) with the root atmos config so the variable is
populated before vendor.Diff() runs; update references to the vendor package
symbols accordingly.
In `@cmd/vendor/pull.go`:
- Around line 55-56: The build fails because RunE in vendor.pull.RunE calls
perf.Track(atmosConfigPtr, ...) but atmosConfigPtr is not declared in this
package; fix by using the correct config variable or declaring the pointer used
elsewhere (same fix applied in diff.go). Update the perf.Track call in RunE
(cmd/vendor/pull.go) to pass the existing package config variable (for example
atmosConfig) or introduce a properly named atmosConfigPtr that references the
package's configuration, ensuring the symbol you pass to perf.Track matches the
actual declared config pointer in this package.
♻️ Duplicate comments (1)
cmd/vendor/pull.go (1)
89-111: Build failure continues here, plus sentinel error not applied.Multiple uses of undefined
atmosConfigPtr(lines 90, 110, 114). Additionally, as noted in a previous review, line 108 should wrap the error with a sentinel:- return fmt.Errorf("failed to initialize CLI config: %w", err) + return fmt.Errorf("%w: %w", errUtils.ErrInitializeCLIConfig, err)This requires importing
errUtils "github.com/cloudposse/atmos/errors".
- Move vendor business logic from internal/exec/ to new focused pkg/vendor/ package - Migrate CLI commands from flat cmd/vendor*.go files to registry pattern in cmd/vendor/ - Implement --stack flag for vendor pull command (NEW feature!) - Export required functions from internal/exec/ to support the new package structure - Created 9 new files in pkg/vendor/ (options, types, errors, uri, config, component, model, vendor, stack) - Created 3 new files in cmd/vendor/ (vendor parent, pull subcommand, diff subcommand) - Deleted 18 old files from cmd/ and internal/exec/ - Updated cmd/root.go to import vendor commands via registry pattern - Updated pkg/vender/ tests to use new pkg/vendor package - All existing tests pass with 80%+ coverage maintained The --stack flag implementation loads the specified stack, extracts components with vendor configs, resolves component path aliasing, and vendors all identified components using the TUI model. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
…t.yaml When using --component flag, vendor.yaml should take precedence and filter by component name. Only fall back to component.yaml if vendor.yaml doesn't exist. This matches the original behavior. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
The vendor refactoring (commit 0199c9f) migrated code from internal/exec/vendor_utils.go to pkg/vendor/uri.go but removed debug logging statements that were present in the original code. This restores the following debug logs: - "Added //. to Git URL without subdirectory" in normalizeVendorURI() - "Normalized triple-slash to double-slash-dot for repository root" in normalizeTripleSlash() - "Normalized triple-slash to double-slash with path" in normalizeTripleSlash() 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Changes: - Add ProcessCommandLineArgs and After ParseFlags debug logs to new vendor pull command (matching legacy behavior) - Fix error message casing: "vendoring" -> "Vendoring" - Fix snapshot typo: "Added /." -> "Added //." (double slash) - Remove duplicate "Found config ENV" lines from snapshots (these were artifacts of inefficient config loading in old architecture) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Changes: - Replace github.com/charmbracelet/log with cloudposse/atmos/pkg/logger in cmd/vendor/pull.go and pkg/vendor/uri.go - Add ErrVendoringNotConfigured sentinel to errors/errors.go - Refactor pkg/vendor/config.go to use error builder pattern with hint - Deprecate pkg/vendor/errors.go ErrVendoringNotConfigured in favor of the central errors package 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
The generateSkipFunction was not correctly handling the precedence of
included_paths patterns. When included_paths was set, files would only
match if they passed both the included pattern check AND the excluded
pattern check, but directories were being excluded prematurely without
checking if nested files could match the pattern.
Fixed by:
1. Checking included_paths first, then excluded_paths
2. Adding couldMatchNestedPath to determine if directories should be
traversed because patterns might match files underneath them
3. Properly handling patterns with leading '/' (like /weather/*.md)
Pattern matching logic has been extracted to a new file pattern.go to
keep model.go within the file length limit.
This fixes the vendor-globs test where patterns like "/weather/*.md"
and "**/{demo-library,demo-stacks}/**/*.{tf,md}" were not working
correctly.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Propagate global flags (--base-path, --config, --config-path, --profile) to InitCliConfig in vendor pull command - Fix error message spacing in config.go (add space after colon) - Fix grammatically awkward error message in stack.go - Clarify vendor configuration terminology in blog post 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Add unit tests to increase test coverage for the vendor package: - stack_test.go: Tests for stack vendoring functions - pattern_test.go: Tests for glob pattern matching - vendor_test.go: Tests for flag validation and source filtering - config_test.go: Enhanced tests for config processing - model_test.go: Tests for TUI model operations - component_test.go: Tests for component handling - options_test.go: Tests for option patterns - types_test.go: Tests for type utilities - cmd/vendor/pull_test.go: Tests for pull command Also includes lint fixes: - Refactor functions to return structs instead of 4+ values - Add named constants for magic numbers and strings - Extract helper functions to reduce function length - Fix error string capitalization - Mark unused parameters appropriately 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Add perf.Track to ReadAndProcessVendorConfigFile and ReadAndProcessComponentVendorConfigFile functions - Fix blog post date from 2025-01-17 to 2025-12-18 - Fix cross-platform path separator issues in tests using filepath.ToSlash for Windows compatibility - Fix error message formatting (add space after colon, use lowercase) - Fix log message format (remove printf placeholders, remove newlines) - Add platform-specific guards for permission test on Windows - Fix pre-existing lint errors in pkg/config/load.go: - Use errors.As instead of type switch for wrapped errors - Add nolint directive for intentional os.Getenv usage 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Add comprehensive tests to increase vendor package test coverage from 75.9% to 77.3%: - TestPkgTypeString: Tests for pkgType String() method including edge cases - TestValidateRemoteURI: Tests for URI validation with various error paths - TestExecuteAtmosVendorInternal: Tests for internal vendor execution errors - TestProcessAtmosVendorSource: Tests for source processing and filtering - TestParseMixinURI_Extended: Tests for mixin URI template parsing errors - TestLoadComponentConfig: Tests for component config loading and validation 🤖 Generated with [Claude Code](https://claude.ai/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Adds additional unit tests to improve coverage from 77.3% to 82.6%: - Added tests for processVendorImports edge cases (self-import, empty import, nested imports) - Added tests for needsCustomDetection with additional URL schemes - Added tests for executeInstall with no valid package - Added tests for model spinner and progress message handling - Added tests for copyToTargetWithPatterns with subdirectories - Added tests for processTargets with multiple targets and no component - Added tests for processAtmosVendorSource with component filtering - Added tests for Pull function (no vendor config, with component) - Added tests for HandleVendorConfigNotExist 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
The vendor pull test expected "Vendoring is not configured" but the actual sentinel error uses lowercase "vendoring is not configured", following the codebase convention for error messages. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Mark the vendor registry pattern + --stack flag milestone as shipped and update progress from 40% to 50%. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
The vendor pull routing now prefers vendor.yaml, so the documentation should reflect this. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
…cture - Replace raw fmt.Errorf with errors.Join(errUtils.ErrInitializingCLIConfig, err) for proper error taxonomy per CodeRabbit feedback - Fix vendor.go to properly define atmosConfigPtr and SetAtmosConfig - Fix vendor.go to add pullCmd and diffCmd as subcommands - Update vendor_test.go to reference pullCmd (was vendorPullCmd) - Update test to expect "Configuration Management" group Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
e5a4f5c to
dfd2468
Compare
what
internal/exec/vendor*.goto focusedpkg/vendor/packagecmd/vendor*.gofiles to registry pattern incmd/vendor/with StandardParser flag handler--stackflag forvendor pullcommand to vendor all components in a specified stackProcessOciImage()andGetSprigFuncMap()frominternal/exec/to support pkg/vendor packagewhy
pkg/packages, CLI setup in thincmd/wrappers--stackflag requested to vendor all components in a stack without specifying each individuallyreferences
Implements plan from
docs/prd/vendor-refactoring.md. Addresses issue requesting--stackflag for vendor operations.Summary by CodeRabbit
New Features
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.