feat(vendor): add --stack flag to vendor pull - #1889
feat(vendor): add --stack flag to vendor pull#1889Erik Osterman (Cloud Posse) (osterman) wants to merge 34 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughMoves the vendoring subsystem out of internal/exec into a new pkg/vendor package, exposing public APIs (Pull, VendorComponent, VendorStack), wiring CLI commands (vendor pull/diff), exporting utilities (ProcessOciImage, GetSprigFuncMap) with perf instrumentation, and adding extensive vendoring logic, pattern matching, and tests. Changes
Sequence DiagramsequenceDiagram
rect rgba(200,200,255,0.5)
participant CLI as CLI (cmd/vendor/pull.go)
end
rect rgba(200,255,200,0.5)
participant Vendor as Vendor Orchestration (pkg/vendor/vendor.go)
end
rect rgba(255,220,200,0.5)
participant Config as Config Processor (pkg/vendor/config.go)
end
rect rgba(255,200,200,0.5)
participant Component as Component Handler (pkg/vendor/component.go)
end
rect rgba(240,240,200,0.5)
participant OCI as OCI Utils (internal/exec)
end
CLI->>CLI: parse flags, build PullOptions
CLI->>Vendor: Pull(atmosConfig, opts...)
Vendor->>Vendor: ValidateFlags / decide route
alt using vendor config
Vendor->>Config: ReadAndProcessVendorConfigFile
Config-->>Vendor: VendorConfigResult
end
Vendor->>Vendor: processAtmosVendorSource -> processTargets
loop per target
Vendor->>Component: prepare temp, copy/select files
Component->>OCI: ProcessOciImage (if OCI)
OCI-->>Component: artifact / temp dir
Component-->>Vendor: installed package result
end
Vendor-->>CLI: return success or 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)
✨ 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 |
Erik Osterman (Cloud Posse) (osterman)
added
the
minor
|
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
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (80.80%) is below the target coverage (85.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #1889 +/- ##
==========================================
+ Coverage 81.68% 81.73% +0.04%
==========================================
Files 1778 1783 +5
Lines 170559 170951 +392
==========================================
+ Hits 139319 139724 +405
+ Misses 23508 23488 -20
- Partials 7732 7739 +7
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 Erik Osterman (Cloud Posse) (@osterman)? 🙏 |
mergify
Bot
added
conflict
…olute path The shouldExcludeFile function was matching exclude patterns against the full absolute path (src), causing simple patterns like "README.md" to fail. Now matches against trimmedSrc (relative path), consistent with the fix applied to checkComponentExcludes. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Strings.Split preserves surrounding spaces, so --tags "networking, database" produced " database" which wouldn't match configured tag names. Trim each entry and skip empty strings, consistent with cmd/terraform/generate/files.go. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
💥 This pull request now has conflicts. Could you fix it Erik Osterman (Cloud Posse) (@osterman)? 🙏 |
|
💥 This pull request now has conflicts. Could you fix it Erik Osterman (Cloud Posse) (@osterman)? 🙏 |
|
Tip Atmos Pro
No affected stacks workflow was detected for this pull request. 
|
GitHub Actions
Inactive
|
Warning This PR exceeds the recommended limit of 10,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. |
Resource Changes Found for
|
GitHub Actions
Inactive
…stack-flag # Conflicts: # cmd/vendor/update.go # cmd/vendor/vendor.go # internal/exec/vendor.go # internal/exec/vendor_component_utils_test.go # internal/exec/vendor_exclude_test.go # internal/exec/vendor_model.go # internal/exec/vendor_model_test.go # internal/exec/vendor_utils.go # pkg/vender/component_vendor_test.go # pkg/vender/vendor_config_test.go # pkg/vendor/component.go # pkg/vendor/uri_triple_slash_test.go # pkg/vendor/vendor_integration_test.go # tests/snapshots/TestCLICommands_atmos_vendor_pull_using_SSH.stderr.golden # tests/snapshots/TestCLICommands_atmos_vendor_pull_with_custom_detector_and_handling_credentials_leakage.stderr.golden
…g rewrite Main independently rebuilt the vendor subsystem into pkg/vendoring/ (native component updater PRs, lockfile, SBOM/provenance) while this branch built its own, now-superseded pkg/vendor registry-pattern rewrite. Drops the duplicate rewrite and re-implements the one genuinely missing piece -- vendor pull --stack -- on top of main's current internal/exec/vendor.go + pkg/vendoring/install architecture: it vendors every component in a stack that declares its own component.yaml, bypassing vendor.yaml entirely. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
tests/test-cases/vendor-test.yaml's "atmos vendor pull without configuration"
case still expected this branch's now-deleted errors.go sentinel text
("vendoring is not configured"), silently carried through the earlier merge
instead of main's actual internal/exec/vendor_utils.go message
("Vendoring is not configured. To set up vendoring, please see
https://atmos.tools/core-concepts/vendor/"). CI caught the mismatch on both
linux and macos acceptance runs.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…r subcommands vendor pull already had --stack/--tags; vendor update had --tags only; diff/clean/verify had neither. Adds --labels (stack-resolved metadata.labels, composable with --stack) and brings --tags (vendor.yaml source tags) and --stack to diff/clean/verify, with a shared internal/exec.ResolveVendorComponentSelector and cmd/vendor selector helpers. Fixes a bug where an unresolvable --stack/--tags/--labels selector silently fell through to "act on everything" instead of erroring. diff gains batch mode when a selector matches multiple components. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…--tags internal/exec/vendor.go's parseVendorTagsFlag split --tags on commas without trimming, so "networking, database" produced " database" with a leading space and "a,,b" produced an empty entry. cmd/vendor's splitTags (used by vendor clean/diff/verify) already trims and filters correctly; parseVendorTagsFlag now matches. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
atmos.yaml's InitCliConfig was always called with processStacks=false across vendor pull/diff/clean/verify/update, so atmosConfig.StackConfigFilesAbsolutePaths was never populated and ExecuteDescribeStacksScoped silently read zero stack files. Every --stack/--labels invocation failed with a misleading "not found" error regardless of whether the stack existed, because unit tests mock ExecuteDescribeStacksScoped directly and no CLI-level test exercised these flags end-to-end. Pass processStacks=true only when --stack/--labels is actually used, so the common --component/--tags/--everything paths keep their existing performance characteristics. Add CLI regression tests (atmos_vendor_pull_stack, atmos_vendor_pull_labels) against a new tests/fixtures/scenarios/vendor-stack-labels/ fixture with real component.yaml manifests and stack metadata.labels, and fix TestVendorUpdateCommand_UnknownStackErrors, which relied on an InitCliConfig(false) no-op and needed a real (non-matching) stack file now that stack processing genuinely runs. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
1 participant
what
--stackflag (-s) toatmos vendor pull: vendors every component declared in the given stack that has its owncomponent.yaml, bypassingvendor.yamlentirely. Cannot be combined with--component.why
--componentinvocations.references
internal/exec/vendor*.go→pkg/vendor/). While this branch was in flight,origin/mainindependently shipped a more complete rewrite of the same internals intopkg/vendoring/(lockfile, SBOM/provenance, native component-updater PR workflow — feat(vendor): native component updater PR workflow #2756). That refactor has been dropped from this PR to avoid duplicating it;--stackis now implemented directly on top of main's currentpkg/vendoring/internal/execarchitecture.