feat: implement source provisioner for JIT component vendoring#1877
feat: implement source provisioner for JIT component vendoring#1877
Conversation
|
Warning This PR exceeds the recommended limit of 1,000 lines.Large PRs are difficult to review and may be rejected due to their size. Please verify that this PR does not address multiple issues. |
Dependency Review✅ No vulnerabilities or license issues found.Scanned FilesNone |
📝 WalkthroughWalkthroughAdds a Source Provisioner feature: top-level Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant CLI as atmos CLI
participant Parser as CLI Helpers
participant Provisioner as SourceProvisioner
participant Downloader
participant GitHub as GitHub API
participant FS as Filesystem
User->>CLI: atmos terraform source pull --stack S --component C
CLI->>Parser: parse flags, init config & auth
Parser->>Provisioner: Provision(ctx, ProvisionParams{Component:C, Stack:S, Force:...})
Provisioner->>Provisioner: ExtractSource(componentConfig)
alt no source configured
Provisioner-->>CLI: ErrSourceMissing / no-op
else source present
Provisioner->>Downloader: NewGoGetterDownloader(opts: Retry/HTTPClient)
Downloader->>GitHub: CheckRateLimit (for GitHub HTTP URLs)
alt need to wait
GitHub-->>Downloader: WaitForRateLimit (spinner)
end
Downloader->>Downloader: perform download (git ops with retries)
Downloader->>FS: write temp download dir
Provisioner->>FS: copyToTarget(tempDir -> targetDir, apply include/exclude)
FS-->>Provisioner: target ready
Provisioner-->>CLI: success
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (5)
pkg/provisioner/source/source.go (2)
33-33: Pass AtmosConfig to perf.Track for proper tracking.
params.AtmosConfigis available butnilis passed instead. Per guidelines, use the config when available.- defer perf.Track(nil, "source.Provision")() + defer perf.Track(params.AtmosConfig, "source.Provision")()Note: Move this after the nil check on line 35-39 to avoid potential nil dereference.
118-119: Missing blank line after perf.Track.Per coding guidelines, add a blank line after the
defer perf.Track(...)()call.defer perf.Track(atmosConfig, "source.DetermineTargetDirectory")() + // Check for working_directory override.cmd/terraform/source/helpers.go (3)
71-73: Use error builder pattern for consistency.Per coding guidelines, errors should be wrapped using static errors from errors/errors.go with
errUtils.Build.if err != nil { - return nil, nil, fmt.Errorf("failed to load component config: %w", err) + return nil, nil, errUtils.Build(errUtils.ErrSourceProvision). + WithCause(err). + WithExplanation("Failed to load component configuration"). + WithContext("component", component). + WithContext("stack", stack). + Err() }Same applies to lines 77-79.
62-73: Duplicate component describe calls in the workflow.
InitConfigAndAuthcallsExecuteDescribeComponentto get component config for auth merging. Then command handlers (e.g., create.go) callDescribeComponentagain. Consider returning the component config fromInitConfigAndAuthto avoid the duplicate work.-func InitConfigAndAuth(component, stack, identity string) (*schema.AtmosConfiguration, *schema.AuthContext, error) { +func InitConfigAndAuth(component, stack, identity string) (*schema.AtmosConfiguration, map[string]any, *schema.AuthContext, error) {Then return
componentConfigalongside the other values.
99-109: DescribeComponent duplicates InitConfigAndAuth's internal call.This function exists separately but performs the same
ExecuteDescribeComponentcall thatInitConfigAndAuthalready does internally. If you refactorInitConfigAndAuthto return component config, this function may become unnecessary.
📜 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 (16)
cmd/terraform/source/create.go(1 hunks)cmd/terraform/source/delete.go(1 hunks)cmd/terraform/source/describe.go(1 hunks)cmd/terraform/source/helpers.go(1 hunks)cmd/terraform/source/list.go(1 hunks)cmd/terraform/source/source.go(1 hunks)cmd/terraform/source/update.go(1 hunks)cmd/terraform/terraform.go(2 hunks)docs/prd/source-provisioner.md(1 hunks)errors/errors.go(1 hunks)pkg/provisioner/source/extract.go(1 hunks)pkg/provisioner/source/extract_test.go(1 hunks)pkg/provisioner/source/source.go(1 hunks)pkg/provisioner/source/source_test.go(1 hunks)pkg/provisioner/source/vendor.go(1 hunks)pkg/provisioner/source/vendor_test.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
docs/prd/**/*.md
📄 CodeRabbit inference engine (CLAUDE.md)
All Product Requirement Documents (PRDs) MUST be placed in docs/prd/ with kebab-case filenames
Files:
docs/prd/source-provisioner.md
cmd/**/*.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
cmd/**/*.go: Use Cobra's recommended command structure with a root command and subcommands, implementing each command in a separate file undercmd/directory
Provide comprehensive help text for all commands and flags, include examples in command help, and follow Go's documentation conventions in Cobra command definitions
Provide meaningful feedback to users and include progress indicators for long-running operations in CLI commands
cmd/**/*.go: Commands MUST use flags.NewStandardParser() for command-specific flags; NEVER call viper.BindEnv() or viper.BindPFlag() directly (Forbidigo enforces this); see cmd/version/version.go for reference
Embed CLI examples from cmd/markdown/*_usage.md using //go:embed; render with utils.PrintfMarkdown()
Files:
cmd/terraform/source/source.gocmd/terraform/source/update.gocmd/terraform/terraform.gocmd/terraform/source/delete.gocmd/terraform/source/create.gocmd/terraform/source/list.gocmd/terraform/source/helpers.gocmd/terraform/source/describe.go
**/*.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
**/*.go: Use Viper for managing configuration, environment variables, and flags in CLI commands
Use interfaces for external dependencies to facilitate mocking and consider using testify/mock for creating mock implementations
All code must pass golangci-lint checks
Follow Go's error handling idioms: use meaningful error messages, wrap errors with context usingfmt.Errorf("context: %w", err), and consider using custom error types for domain-specific errors
Follow standard Go coding style: usegofmtandgoimportsto format code, prefer short descriptive variable names, use kebab-case for command-line flags, and snake_case for environment variables
Document all exported functions, types, and methods following Go's documentation conventions
Document complex logic with inline comments in Go code
Support configuration via files, environment variables, and flags following the precedence order: flags > environment variables > config file > defaults
Provide clear error messages to users, include troubleshooting hints when appropriate, and log detailed errors for debugging
**/*.go: NEVER use fmt.Fprintf(os.Stdout/Stderr) or fmt.Println(); use data.* or ui.* functions instead
All comments must end with periods (enforced by godot linter)
Organize imports in three groups separated by blank lines, sorted alphabetically: 1) Go stdlib, 2) 3rd-party (NOT cloudposse/atmos), 3) Atmos packages; maintain aliases: cfg, log, u, errUtils
Adddefer perf.Track(atmosConfig, "pkg.FuncName")()+ blank line to all public functions for performance tracking; use nil if no atmosConfig param
All errors MUST be wrapped using static errors defined in errors/errors.go; use errors.Join for combining multiple errors; use fmt.Errorf with %w for adding string context; use error builder for complex errors; use errors.Is() for error checking; NEVER use dynamic errors directly
Use go.uber.org/mock/mockgen with //go:generate directives for mock generation; never create manual mocks
Keep files small...
Files:
cmd/terraform/source/source.gopkg/provisioner/source/extract.gopkg/provisioner/source/source_test.gopkg/provisioner/source/extract_test.gopkg/provisioner/source/vendor_test.gocmd/terraform/source/update.gocmd/terraform/terraform.gocmd/terraform/source/delete.gocmd/terraform/source/create.goerrors/errors.gocmd/terraform/source/list.gopkg/provisioner/source/source.gocmd/terraform/source/helpers.gocmd/terraform/source/describe.gopkg/provisioner/source/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 fixtures
**/*_test.go: Prefer unit tests with mocks over integration tests; use interfaces + dependency injection for testability; generate mocks with go.uber.org/mock/mockgen; use table-driven tests; target >80% coverage
Test behavior, not implementation; never test stub functions; avoid tautological tests; make code testable via DI; no coverage theater; remove always-skipped tests; use errors.Is() for error checking
Files:
pkg/provisioner/source/source_test.gopkg/provisioner/source/extract_test.gopkg/provisioner/source/vendor_test.go
🧠 Learnings (48)
📓 Common learnings
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: 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: 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.
📚 Learning: 2024-10-31T01:22:09.586Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 737
File: examples/demo-vendoring/vendor.d/vendor1.yaml:10-11
Timestamp: 2024-10-31T01:22:09.586Z
Learning: In `examples/demo-vendoring/vendor.d/vendor1.yaml`, when specifying the source for the `ipinfo` component, it's acceptable to use `main` as the version reference if pinning to a specific version is not practical.
Applied to files:
docs/prd/source-provisioner.mdpkg/provisioner/source/vendor.go
📚 Learning: 2025-09-08T01:25:44.958Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1466
File: website/docs/cli/commands/toolchain/usage.mdx:117-121
Timestamp: 2025-09-08T01:25:44.958Z
Learning: Final XDG Base Directory Specification implementation for atmos toolchain is complete and verified: toolchain/xdg_cache.go provides GetXDGCacheDir() and GetXDGTempCacheDir() functions, all hardcoded ~/.cache/tools-cache paths have been replaced with XDG-compliant paths using ${XDG_CACHE_HOME}/atmos-toolchain (or ~/.cache/atmos-toolchain fallback), and tests have been updated to expect the new path structure.
Applied to files:
docs/prd/source-provisioner.md
📚 Learning: 2025-09-08T01:25:44.958Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1466
File: website/docs/cli/commands/toolchain/usage.mdx:117-121
Timestamp: 2025-09-08T01:25:44.958Z
Learning: XDG Base Directory Specification compliance implementation for atmos toolchain is complete: created toolchain/xdg_cache.go with GetXDGCacheDir() and GetXDGTempCacheDir() functions, updated toolchain/installer.go and cmd/toolchain_clean.go to use these XDG helpers, and changed all cache paths from hardcoded ~/.cache/tools-cache to XDG-compliant ${XDG_CACHE_HOME}/atmos-toolchain (or ~/.cache/atmos-toolchain fallback).
Applied to files:
docs/prd/source-provisioner.md
📚 Learning: 2025-12-13T06:07:34.794Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1686
File: docs/prd/tool-dependencies-integration.md:58-64
Timestamp: 2025-12-13T06:07:34.794Z
Learning: For docs in the cloudposse/atmos repository under docs/prd/, markdownlint issues MD040, MD010, and MD034 should be deferred to a separate documentation cleanup commit and must not block the current PR. If needed, address these issues in a follow-up PR dedicated to documentation improvements.
Applied to files:
docs/prd/source-provisioner.md
📚 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/terraform/source/source.gocmd/terraform/source/update.gocmd/terraform/terraform.gocmd/terraform/source/create.gocmd/terraform/source/list.gocmd/terraform/source/helpers.gocmd/terraform/source/describe.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/terraform/source/source.gocmd/terraform/source/update.gocmd/terraform/terraform.gocmd/terraform/source/delete.gocmd/terraform/source/list.gocmd/terraform/source/helpers.gocmd/terraform/source/describe.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/terraform/source/source.gocmd/terraform/source/update.gocmd/terraform/terraform.gocmd/terraform/source/helpers.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/terraform/source/source.gocmd/terraform/source/delete.gocmd/terraform/source/describe.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/terraform/source/source.gocmd/terraform/terraform.gocmd/terraform/source/helpers.go
📚 Learning: 2025-12-13T03:21:27.620Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1813
File: cmd/terraform/shell.go:28-73
Timestamp: 2025-12-13T03:21:27.620Z
Learning: In Atmos, when initializing CLI config via cfg.InitCliConfig, always first populate the ConfigAndStacksInfo struct with global flag values by calling flags.ParseGlobalFlags(cmd, v) before LoadConfig. LoadConfig (pkg/config/load.go) reads config selection fields (AtmosConfigFilesFromArg, AtmosConfigDirsFromArg, BasePath, ProfilesFromArg) from the ConfigAndStacksInfo struct, not from Viper. Passing an empty struct causes the --base-path, --config, --config-path, and --profile flags to be ignored. Recommended pattern: parse flags → populate struct → call InitCliConfig. See cmd/terraform/plan_diff.go for reference implementation.
Applied to files:
cmd/terraform/source/source.gocmd/terraform/source/update.gocmd/terraform/terraform.gocmd/terraform/source/delete.gocmd/terraform/source/create.gocmd/terraform/source/list.gocmd/terraform/source/helpers.gocmd/terraform/source/describe.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/terraform/source/source.gocmd/terraform/source/update.gocmd/terraform/terraform.gocmd/terraform/source/delete.gocmd/terraform/source/create.gocmd/terraform/source/list.gocmd/terraform/source/helpers.gocmd/terraform/source/describe.go
📚 Learning: 2025-11-24T17:35:37.209Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-11-24T17:35:37.209Z
Learning: Applies to **/*_test.go : Every new feature must include comprehensive unit tests targeting >80% code coverage for all packages
Applied to files:
pkg/provisioner/source/source_test.gopkg/provisioner/source/extract_test.gopkg/provisioner/source/vendor_test.go
📚 Learning: 2025-05-23T19:51:47.091Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 1255
File: cmd/describe_affected_test.go:15-15
Timestamp: 2025-05-23T19:51:47.091Z
Learning: The atmos codebase has a custom extension to *testing.T that provides a Chdir method, allowing test functions to call t.Chdir() to change working directories during tests. This is used consistently across test files in the codebase.
Applied to files:
pkg/provisioner/source/source_test.gopkg/provisioner/source/vendor_test.go
📚 Learning: 2025-11-24T17:35:37.209Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-11-24T17:35:37.209Z
Learning: Applies to **/*_test.go : Include integration tests for command flows and test CLI end-to-end when possible with test fixtures
Applied to files:
pkg/provisioner/source/source_test.gopkg/provisioner/source/vendor_test.go
📚 Learning: 2025-12-16T18:20:55.614Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T18:20:55.614Z
Learning: Applies to **/*_test.go : Test behavior, not implementation; never test stub functions; avoid tautological tests; make code testable via DI; no coverage theater; remove always-skipped tests; use errors.Is() for error checking
Applied to files:
pkg/provisioner/source/source_test.gopkg/provisioner/source/extract_test.gopkg/provisioner/source/vendor_test.go
📚 Learning: 2025-11-24T17:35:37.209Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-11-24T17:35:37.209Z
Learning: Applies to **/*_test.go : Use table-driven tests for testing multiple scenarios in Go
Applied to files:
pkg/provisioner/source/source_test.gopkg/provisioner/source/vendor_test.go
📚 Learning: 2025-05-23T19:51:47.091Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 1255
File: cmd/describe_affected_test.go:15-15
Timestamp: 2025-05-23T19:51:47.091Z
Learning: In the atmos codebase, t.Chdir() is a valid method call on *testing.T objects and works correctly for changing directories in tests. This is implemented through custom testing framework extensions and is used consistently throughout the test suite.
Applied to files:
pkg/provisioner/source/source_test.go
📚 Learning: 2025-05-23T19:51:47.091Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 1255
File: cmd/describe_affected_test.go:15-15
Timestamp: 2025-05-23T19:51:47.091Z
Learning: In the atmos codebase, t.Chdir() is a valid method that can be called on *testing.T objects. This functionality is implemented through custom testing framework extensions and is used consistently throughout the test suite for changing working directories during tests.
Applied to files:
pkg/provisioner/source/source_test.go
📚 Learning: 2025-05-23T19:51:47.091Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 1255
File: cmd/describe_affected_test.go:15-15
Timestamp: 2025-05-23T19:51:47.091Z
Learning: In the atmos codebase, t.Chdir() is a valid method call on *testing.T objects and works correctly for changing directories in tests.
Applied to files:
pkg/provisioner/source/source_test.go
📚 Learning: 2025-12-10T18:32:51.237Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1808
File: cmd/terraform/backend/backend_delete_test.go:9-23
Timestamp: 2025-12-10T18:32:51.237Z
Learning: In cmd subpackages (e.g., cmd/terraform/backend/), tests cannot use cmd.NewTestKit(t) due to Go's test visibility rules (NewTestKit is in a parent package test file). These tests only need TestKit if they execute commands through RootCmd or modify RootCmd state. Structural tests that only verify command structure/flags without touching RootCmd don't require TestKit cleanup.
Applied to files:
pkg/provisioner/source/source_test.gocmd/terraform/terraform.go
📚 Learning: 2025-11-08T19:56:18.660Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1697
File: internal/exec/oci_utils.go:0-0
Timestamp: 2025-11-08T19:56:18.660Z
Learning: In the Atmos codebase, when a function receives an `*schema.AtmosConfiguration` parameter, it should read configuration values from `atmosConfig.Settings` fields rather than using direct `os.Getenv()` or `viper.GetString()` calls. The Atmos pattern is: viper.BindEnv in cmd/root.go binds environment variables → Viper unmarshals into atmosConfig.Settings via mapstructure → business logic reads from the Settings struct. This provides centralized config management, respects precedence, and enables testability. Example: `atmosConfig.Settings.AtmosGithubToken` instead of `os.Getenv("ATMOS_GITHUB_TOKEN")` in functions like `getGHCRAuth` in internal/exec/oci_utils.go.
Applied to files:
pkg/provisioner/source/source_test.gocmd/terraform/source/helpers.go
📚 Learning: 2025-11-11T03:47:59.576Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1686
File: toolchain/which_test.go:166-223
Timestamp: 2025-11-11T03:47:59.576Z
Learning: In the cloudposse/atmos repo, tests that manipulate environment variables should use testing.T.Setenv for automatic setup/teardown instead of os.Setenv/Unsetenv.
Applied to files:
pkg/provisioner/source/source_test.go
📚 Learning: 2025-11-11T03:47:45.878Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1686
File: toolchain/add_test.go:67-77
Timestamp: 2025-11-11T03:47:45.878Z
Learning: In the cloudposse/atmos codebase, tests should prefer t.Setenv for environment variable setup/teardown instead of os.Setenv/Unsetenv to ensure test-scoped isolation.
Applied to files:
pkg/provisioner/source/source_test.go
📚 Learning: 2025-11-10T23:23:39.771Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1686
File: toolchain/registry/aqua/aqua_test.go:417-442
Timestamp: 2025-11-10T23:23:39.771Z
Learning: In Atmos toolchain AquaRegistry, tests should not hit real GitHub. Use the options pattern via WithGitHubBaseURL to inject an httptest server URL and make GetLatestVersion/GetAvailableVersions deterministic.
Applied to files:
pkg/provisioner/source/source_test.gopkg/provisioner/source/vendor_test.go
📚 Learning: 2025-12-16T18:20:55.614Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T18:20:55.614Z
Learning: Applies to **/*_test.go : Prefer unit tests with mocks over integration tests; use interfaces + dependency injection for testability; generate mocks with go.uber.org/mock/mockgen; use table-driven tests; target >80% coverage
Applied to files:
pkg/provisioner/source/vendor_test.go
📚 Learning: 2025-11-24T17:35:37.209Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-11-24T17:35:37.209Z
Learning: Applies to **/*.go : Use interfaces for external dependencies to facilitate mocking and consider using testify/mock for creating mock implementations
Applied to files:
pkg/provisioner/source/vendor_test.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/provisioner/source/vendor_test.gopkg/provisioner/source/vendor.go
📚 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/provisioner/source/vendor_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 : 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/terraform/source/update.gocmd/terraform/source/delete.gocmd/terraform/source/list.gocmd/terraform/source/helpers.gocmd/terraform/source/describe.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.mod,go.sum} : Manage dependencies with Go modules and keep dependencies up to date while minimizing external dependencies
Applied to files:
cmd/terraform/source/update.go
📚 Learning: 2025-07-05T20:59:02.914Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 1363
File: internal/exec/template_utils.go:18-18
Timestamp: 2025-07-05T20:59:02.914Z
Learning: In the Atmos project, gomplate v4 is imported with a blank import (`_ "github.com/hairyhenderson/gomplate/v4"`) alongside v3 imports to resolve AWS SDK version conflicts. V3 uses older AWS SDK versions that conflict with newer AWS modules used by Atmos. A full migration to v4 requires extensive refactoring due to API changes and should be handled in a separate PR.
Applied to files:
cmd/terraform/terraform.gopkg/provisioner/source/vendor.go
📚 Learning: 2024-10-23T21:36:40.262Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 740
File: cmd/cmd_utils.go:340-359
Timestamp: 2024-10-23T21:36:40.262Z
Learning: In the Go codebase for Atmos, when reviewing functions like `checkAtmosConfig` in `cmd/cmd_utils.go`, avoid suggesting refactoring to return errors instead of calling `os.Exit` if such changes would significantly increase the scope due to the need to update multiple call sites.
Applied to files:
cmd/terraform/terraform.go
📚 Learning: 2025-12-16T18:20:55.614Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T18:20:55.614Z
Learning: Applies to **/*.go : Organize imports in three groups separated by blank lines, sorted alphabetically: 1) Go stdlib, 2) 3rd-party (NOT cloudposse/atmos), 3) Atmos packages; maintain aliases: cfg, log, u, errUtils
Applied to files:
cmd/terraform/terraform.gocmd/terraform/source/helpers.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:
cmd/terraform/terraform.go
📚 Learning: 2024-10-20T13:12:46.499Z
Learnt from: haitham911
Repo: cloudposse/atmos PR: 736
File: pkg/config/const.go:6-6
Timestamp: 2024-10-20T13:12:46.499Z
Learning: In `cmd/cmd_utils.go`, it's acceptable to have hardcoded references to `atmos.yaml` in logs, and it's not necessary to update them to use the `CliConfigFileName` constant.
Applied to files:
cmd/terraform/terraform.go
📚 Learning: 2024-11-16T17:30:52.893Z
Learnt from: pkbhowmick
Repo: cloudposse/atmos PR: 786
File: internal/exec/shell_utils.go:159-162
Timestamp: 2024-11-16T17:30:52.893Z
Learning: For the `atmos terraform shell` command in `internal/exec/shell_utils.go`, input validation for the custom shell prompt is not required, as users will use this as a CLI tool and any issues will impact themselves.
Applied to files:
cmd/terraform/terraform.go
📚 Learning: 2024-11-02T15:35:09.958Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 759
File: internal/exec/terraform.go:366-368
Timestamp: 2024-11-02T15:35:09.958Z
Learning: In `internal/exec/terraform.go`, the workspace cleaning code under both the general execution path and within the `case "init":` block is intentionally duplicated because the code execution paths are different. The `.terraform/environment` file should be deleted before executing `terraform init` in both scenarios to ensure a clean state.
Applied to files:
cmd/terraform/source/delete.go
📚 Learning: 2024-11-12T03:16:02.910Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 775
File: internal/exec/template_funcs_component.go:157-159
Timestamp: 2024-11-12T03:16:02.910Z
Learning: In the Go code for `componentFunc` in `internal/exec/template_funcs_component.go`, the function `cleanTerraformWorkspace` does not return errors, and it's acceptable if the file does not exist. Therefore, error handling for `cleanTerraformWorkspace` is not needed.
Applied to files:
cmd/terraform/source/delete.go
📚 Learning: 2024-10-27T04:41:49.199Z
Learnt from: haitham911
Repo: cloudposse/atmos PR: 727
File: internal/exec/terraform_clean.go:215-223
Timestamp: 2024-10-27T04:41:49.199Z
Learning: In `internal/exec/terraform_clean.go`, the function `determineCleanPath` is necessary and should not be removed.
Applied to files:
cmd/terraform/source/delete.go
📚 Learning: 2024-10-31T19:25:41.298Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 727
File: internal/exec/terraform_clean.go:233-235
Timestamp: 2024-10-31T19:25:41.298Z
Learning: When specifying color values in functions like `confirmDeleteTerraformLocal` in `internal/exec/terraform_clean.go`, avoid hardcoding color values. Instead, use predefined color constants or allow customization through configuration settings to improve accessibility and user experience across different terminals and themes.
Applied to files:
cmd/terraform/source/delete.go
📚 Learning: 2024-10-21T17:51:53.976Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 727
File: internal/exec/terraform.go:114-118
Timestamp: 2024-10-21T17:51:53.976Z
Learning: When `atmos terraform clean --everything` is used without specifying a component and without the `--force` flag, prompt the user for confirmation before deleting all components. Use the `--force` flag to skip the confirmation prompt.
Applied to files:
cmd/terraform/source/delete.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:
errors/errors.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/terraform/source/helpers.go
📚 Learning: 2025-06-23T02:14:30.937Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 1327
File: cmd/terraform.go:111-117
Timestamp: 2025-06-23T02:14:30.937Z
Learning: In cmd/terraform.go, flags for the DescribeAffected function are added dynamically at runtime when info.Affected is true. This is intentional to avoid exposing internal flags like "file", "format", "verbose", "include-spacelift-admin-stacks", "include-settings", and "upload" in the terraform command interface, while still providing them for the shared DescribeAffected function used by both `atmos describe affected` and `atmos terraform apply --affected`.
Applied to files:
cmd/terraform/source/helpers.gocmd/terraform/source/describe.go
📚 Learning: 2024-12-07T16:19:01.683Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 825
File: internal/exec/terraform.go:30-30
Timestamp: 2024-12-07T16:19:01.683Z
Learning: In `internal/exec/terraform.go`, skipping stack validation when help flags are present is not necessary.
Applied to files:
cmd/terraform/source/helpers.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/terraform/source/helpers.go
📚 Learning: 2025-01-25T03:59:47.777Z
Learnt from: Listener430
Repo: cloudposse/atmos PR: 934
File: internal/exec/docs_generate.go:220-248
Timestamp: 2025-01-25T03:59:47.777Z
Learning: In the `downloadSource` function in `internal/exec/docs_generate.go`, the code is intentionally kept concise without additional security measures like content validation, download size limits, or domain validation to maintain simplicity and focus on core functionality.
Applied to files:
pkg/provisioner/source/vendor.go
🧬 Code graph analysis (10)
cmd/terraform/source/source.go (1)
pkg/schema/schema.go (1)
AtmosConfiguration(53-96)
pkg/provisioner/source/extract.go (4)
pkg/schema/vendor_component.go (1)
VendorComponentSource(5-11)pkg/perf/perf.go (1)
Track(121-138)errors/builder.go (1)
Build(24-37)errors/errors.go (1)
ErrSourceInvalidSpec(666-666)
pkg/provisioner/source/extract_test.go (2)
errors/errors.go (1)
ErrSourceInvalidSpec(666-666)pkg/provisioner/source/extract.go (2)
ExtractMetadataSource(14-46)HasMetadataSource(97-108)
pkg/provisioner/source/vendor_test.go (1)
pkg/schema/vendor_component.go (1)
VendorComponentSource(5-11)
cmd/terraform/source/update.go (3)
cmd/terraform/source/helpers.go (5)
ParseCommonFlags(30-48)InitConfigAndAuth(52-97)DescribeComponent(100-109)ProvisionSource(122-132)ProvisionSourceOptions(112-119)errors/errors.go (1)
ErrMetadataSourceMissing(670-670)pkg/provisioner/source/extract.go (1)
HasMetadataSource(97-108)
cmd/terraform/terraform.go (1)
cmd/terraform/source/source.go (1)
GetSourceCommand(48-50)
cmd/terraform/source/list.go (5)
pkg/flags/standard_parser.go (2)
StandardParser(18-22)NewStandardParser(34-40)pkg/perf/perf.go (1)
Track(121-138)pkg/flags/parser.go (1)
GetString(117-126)errors/errors.go (2)
ErrRequiredFlagNotProvided(220-220)ErrNotImplemented(28-28)errors/builder.go (1)
Build(24-37)
pkg/provisioner/source/source.go (6)
pkg/schema/schema.go (3)
AtmosConfiguration(53-96)Terraform(367-379)Helmfile(393-400)pkg/perf/perf.go (1)
Track(121-138)errors/builder.go (1)
Build(24-37)errors/errors.go (1)
ErrNilParam(562-562)pkg/provisioner/source/extract.go (1)
ExtractMetadataSource(14-46)pkg/provisioner/source/vendor.go (1)
VendorSource(28-89)
cmd/terraform/source/helpers.go (9)
pkg/flags/global_registry.go (1)
ParseGlobalFlags(33-77)errors/errors.go (2)
ErrRequiredFlagNotProvided(220-220)ErrFailedToInitConfig(429-429)pkg/schema/schema.go (3)
AtmosConfiguration(53-96)AuthContext(594-604)ConfigAndStacksInfo(646-742)pkg/config/config.go (1)
InitCliConfig(28-67)internal/exec/describe_component.go (1)
ExecuteDescribeComponentParams(203-210)pkg/auth/config_helpers.go (1)
MergeComponentAuthFromConfig(138-160)pkg/config/const.go (1)
AuthSectionName(85-85)pkg/auth/manager_helpers.go (1)
CreateAndAuthenticateManager(184-192)pkg/provisioner/source/source.go (2)
Provision(32-84)ProvisionParams(19-27)
cmd/terraform/source/describe.go (5)
errors/errors.go (3)
ErrDescribeComponent(96-96)ErrMetadataSourceMissing(670-670)ErrOutputFormat(165-165)cmd/terraform/source/helpers.go (1)
DescribeComponent(100-109)errors/builder.go (1)
Build(24-37)pkg/provisioner/source/extract.go (1)
ExtractMetadataSource(14-46)pkg/utils/yaml_utils.go (1)
ConvertToYAML(561-586)
🪛 GitHub Check: CodeQL
cmd/terraform/source/describe.go
[failure] 100-100: Clear-text logging of sensitive information
Sensitive data returned by an access to passwdParts flows to a logging call.
🪛 LanguageTool
docs/prd/source-provisioner.md
[typographical] ~329-~329: In American English, use a period after an abbreviation.
Context: ...ceProvider ``` --- ## When to Use CLI vs Automatic Automatic (via hooks): `...
(MISSING_PERIOD_AFTER_ABBREVIATION)
[typographical] ~408-~408: To join two clauses or introduce examples, consider using an em dash.
Context: ...- ## Schema Design ### metadata.source - String Form When metadata.source is a...
(DASH_RULE)
[grammar] ~410-~410: Use a hyphen to join words.
Context: ... string, it's interpreted as a go-getter compatible URI: ```yaml metadata: sou...
(QB_NEW_EN_HYPHEN)
[typographical] ~424-~424: To join two clauses or introduce examples, consider using an em dash.
Context: .../vpc?ref=1.2.3" ``` ### metadata.source - Map Form When metadata.source is a ma...
(DASH_RULE)
[grammar] ~716-~716: Use a hyphen to join words.
Context: ...## Supported Source Types All go-getter supported schemes work: | Scheme | Exam...
(QB_NEW_EN_HYPHEN)
[uncategorized] ~721-~721: The official name of this software platform is spelled with a capital “H”.
Context: ...ps://github.com/org/repo.git//path| |github.com|github.com/org/repo//path?ref=v...
(GITHUB)
[typographical] ~960-~960: To join two clauses or introduce examples, consider using an em dash.
Context: ... 1. cmd/terraform/source/source.go - Source subcommand parent, GetSourceComma...
(DASH_RULE)
[typographical] ~974-~974: To join two clauses or introduce examples, consider using an em dash.
Context: ... 12. cmd/helmfile/source/source.go - Source subcommand for helmfile 13. **`cm...
(DASH_RULE)
[typographical] ~975-~975: To join two clauses or introduce examples, consider using an em dash.
Context: ...lmfile 13. cmd/helmfile/source/... - Same structure as terraform **Component...
(DASH_RULE)
[typographical] ~979-~979: To join two clauses or introduce examples, consider using an em dash.
Context: .... pkg/component/source_provider.go - Optional SourceProvider interface 15. **...
(DASH_RULE)
[typographical] ~984-~984: To join two clauses or introduce examples, consider using an em dash.
Context: .... pkg/provisioner/source/source.go - Main provisioner implementation 17. **`p...
(DASH_RULE)
[typographical] ~996-~996: To join two clauses or introduce examples, consider using an em dash.
Context: ...ify 1. cmd/terraform/terraform.go - Add `terraformCmd.AddCommand(source.GetS...
(DASH_RULE)
[typographical] ~997-~997: To join two clauses or introduce examples, consider using an em dash.
Context: ...nd()) 2. **cmd/helmfile/helmfile.go** - Add helmfileCmd.AddCommand(source.GetSo...
(DASH_RULE)
[typographical] ~998-~998: To join two clauses or introduce examples, consider using an em dash.
Context: ...rceCommand()) 3. **errors/errors.go`** - Add sentinel errors (ErrSourceProvision,...
(DASH_RULE)
[grammar] ~1024-~1024: Please add a punctuation mark at the end of paragraph.
Context: ...tches**: git fetch only downloads new objects ### Architecture The cache uses the X...
(PUNCTUATION_PARAGRAPH_END)
[grammar] ~1072-~1072: Please add a punctuation mark at the end of paragraph.
Context: ...ing: Cache location matches CLI command hierarchy Environment Variable Precedence: 1...
(PUNCTUATION_PARAGRAPH_END)
[grammar] ~1139-~1139: Please add a punctuation mark at the end of paragraph.
Context: ...No custom tooling, uses Git's optimized internals Worktree Naming Convention: Since ...
(PUNCTUATION_PARAGRAPH_END)
[grammar] ~1249-~1249: Please add a punctuation mark at the end of paragraph.
Context: ...g (skip .git, symlinks) - No additional dependencies Performance Characteristics: - Use...
(PUNCTUATION_PARAGRAPH_END)
[style] ~1254-~1254: Since ownership is already implied, this phrasing may be redundant.
Context: ... - Concurrent-safe: each component gets its own copy - No file locking issues **When C...
(PRP_OWN)
[grammar] ~1255-~1255: Please add a punctuation mark at the end of paragraph.
Context: ...ent gets its own copy - No file locking issues When Copying Occurs: | Scenario |...
(PUNCTUATION_PARAGRAPH_END)
[style] ~1276-~1276: Since ownership is already implied, this phrasing may be redundant.
Context: ...licts! ``` Each component instance has its own target directory, avoiding the write co...
(PRP_OWN)
[grammar] ~1324-~1324: Use a hyphen to join words.
Context: ...isioner might: - Handle component.yaml based vendoring - Support more complex d...
(QB_NEW_EN_HYPHEN)
[grammar] ~1326-~1326: Please add a punctuation mark at the end of paragraph.
Context: ... dependency resolution - Cache vendored components ### Version Pinning Consider adding l...
(PUNCTUATION_PARAGRAPH_END)
[typographical] ~1366-~1366: To join two clauses or introduce examples, consider using an em dash.
Context: ...ioner System](./provisioner-system.md)** - Generic provisioner infrastructure - **[...
(DASH_RULE)
[style] ~1366-~1366: Using four (or more) nouns in a row may decrease readability.
Context: ...m](./provisioner-system.md)** - Generic provisioner infrastructure - Backend Provisioner - Backend ...
(FOUR_NN)
[typographical] ~1367-~1367: To join two clauses or introduce examples, consider using an em dash.
Context: ...Provisioner](./backend-provisioner.md)** - Backend provisioner reference (CRUD patt...
(DASH_RULE)
[typographical] ~1368-~1368: To join two clauses or introduce examples, consider using an em dash.
Context: ...on](./terraform-registry-migration.md)** - Command structure pattern (PR #1813) - *...
(DASH_RULE)
[typographical] ~1369-~1369: To join two clauses or introduce examples, consider using an em dash.
Context: ...zation](./vendor-uri-normalization.md)** - URI handling details - **[Component Regi...
(DASH_RULE)
[typographical] ~1370-~1370: To join two clauses or introduce examples, consider using an em dash.
Context: ...tern](./component-registry-pattern.md)** - Component provider pattern - **[XDG Base...
(DASH_RULE)
[typographical] ~1371-~1371: To join two clauses or introduce examples, consider using an em dash.
Context: ..../xdg-base-directory-specification.md)** - Cache location conventions
(DASH_RULE)
🪛 markdownlint-cli2 (0.18.1)
docs/prd/source-provisioner.md
1143-1143: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ 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
|
💥 This pull request now has conflicts. Could you fix it @osterman? 🙏 |
3044a69 to
54eddfd
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1877 +/- ##
==========================================
+ Coverage 73.57% 73.69% +0.11%
==========================================
Files 643 656 +13
Lines 59723 60769 +1046
==========================================
+ Hits 43943 44783 +840
- Misses 12771 12946 +175
- Partials 3009 3040 +31
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: 0
🧹 Nitpick comments (6)
cmd/terraform/source/helpers_test.go (1)
1-149: Test coverage is solid.The tests appropriately validate helper behavior: flag parsing with required flags, all flag combinations, and provisioning scenarios (no source, existing target). Error checking uses
errors.Is()correctly, and tests don't use NewTestKit since they're structural tests not touching RootCmd.The ParseCommonFlags tests (lines 19-81) could be consolidated into a table-driven test to reduce duplication:
func TestParseCommonFlags(t *testing.T) { tests := []struct { name string args []string wantErr error wantStack string wantForce bool }{ { name: "missing stack", args: []string{}, wantErr: errUtils.ErrRequiredFlagNotProvided, }, { name: "with stack", args: []string{"--stack", "dev-us-east-1"}, wantStack: "dev-us-east-1", }, { name: "all flags", args: []string{"--stack", "prod-us-west-2", "--identity", "my-identity", "--force"}, wantStack: "prod-us-west-2", wantForce: true, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { // Test logic here }) } }cmd/terraform/source/describe_test.go (1)
1-102: Describe command tests are valid but use a different pattern.Tests validate behavior correctly and use
errors.Is()as required. However, these tests bind flags to Viper manually (lines 34-42, 63-70), while create_test.go and helpers_test.go use theParseCommonFlagshelper. This inconsistency adds complexity.Consider using the same pattern as create_test.go for consistency:
func TestDescribeCommand_MissingStack(t *testing.T) { cmd := &cobra.Command{ Use: "describe <component>", Args: cobra.ExactArgs(1), RunE: func(cmd *cobra.Command, args []string) error { opts, err := ParseCommonFlags(cmd, describeParser) if err != nil { return err } _ = opts return nil }, } describeParser.RegisterFlags(cmd) cmd.SetArgs([]string{"vpc"}) err := cmd.Execute() require.Error(t, err) assert.ErrorIs(t, err, errUtils.ErrRequiredFlagNotProvided) }This would align with the pattern used in create_test.go and reduce manual Viper wiring.
pkg/provisioner/source/extract.go (1)
85-94: Consider warning when non-string elements are silently dropped.
toStringSlicequietly skips non-string items. If a user misconfiguresincluded_pathswith integers or nested objects, they won't know their patterns were ignored.func toStringSlice(items []any) []string { result := make([]string, 0, len(items)) for _, item := range items { if s, ok := item.(string); ok { result = append(result, s) + } else if item != nil { + // Non-string items in path lists are likely config errors. + _ = ui.Warningf("ignoring non-string path pattern: %v", item) } } return result }You'd need to add
"github.com/cloudposse/atmos/pkg/ui"to imports.pkg/provisioner/source/source.go (2)
95-115:needsVendoringreturns true on permission errors.If
os.Statfails due to permission issues (notIsNotExist), the function returnstrue, which could trigger unnecessary vendoring attempts that will also fail.Consider distinguishing permission errors:
func needsVendoring(targetDir string) bool { info, err := os.Stat(targetDir) if os.IsNotExist(err) { return true } if err != nil { - return true // Error accessing, assume needs vendoring. + // Log the error but assume vendoring is needed. + _ = ui.Warningf("error checking target directory %s: %v", targetDir, err) + return true }
117-152: Target directory resolution logic is sound.Priority order (metadata.working_directory → settings.working_directory → default) is reasonable. Error case for missing base path is handled.
Per guidelines, add a blank line after the
perf.Trackcall on Line 127.cmd/terraform/source/helpers.go (1)
52-112: Consider usingerrUtils.Buildfor error wrapping consistency.Lines 87 and 93 use
fmt.Errorfwhile other errors useerrUtils.Build. For consistency with the codebase pattern:- return nil, nil, fmt.Errorf("failed to load component config: %w", err) + return nil, nil, errUtils.Build(errUtils.ErrSourceProvision). + WithCause(err). + WithExplanation("failed to load component config"). + WithContext("component", component). + WithContext("stack", stack). + Err()Same for line 93. This also enables
errors.Is()checks on the sentinel.
|
💥 This pull request now has conflicts. Could you fix it @osterman? 🙏 |
There was a problem hiding this comment.
Actionable comments posted: 2
📜 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 (2)
pkg/utils/yaml_utils.go(2 hunks)pkg/utils/yaml_utils_test.go(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.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/utils/yaml_utils.gopkg/utils/yaml_utils_test.go
**/*_test.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
**/*_test.go: Every new feature must include comprehensive unit tests targeting >80% code coverage for all packages
Use table-driven tests for testing multiple scenarios in Go
Include integration tests for command flows and test CLI end-to-end when possible with test fixtures
**/*_test.go: Prefer unit tests with mocks over integration tests; use interfaces + dependency injection for testability; generate mocks with go.uber.org/mock/mockgen; use table-driven tests; target >80% coverage
Test behavior, not implementation; never test stub functions; avoid tautological tests; make code testable via DI; no coverage theater; remove always-skipped tests; use errors.Is() for error checking
Files:
pkg/utils/yaml_utils_test.go
🧠 Learnings (29)
📓 Common learnings
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: 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: 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.
📚 Learning: 2025-03-12T21:38:42.699Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1139
File: pkg/config/go-homedir/homedir.go:183-196
Timestamp: 2025-03-12T21:38:42.699Z
Learning: The code in pkg/config/go-homedir is a direct fork of the mitchellh/go-homedir package and was intentionally imported as-is without modifications to maintain consistency with the original source. Security concerns or other improvements may be addressed in future PRs.
Applied to files:
pkg/utils/yaml_utils.go
📚 Learning: 2024-12-25T20:28:19.618Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 887
File: internal/exec/workflow_utils.go:167-169
Timestamp: 2024-12-25T20:28:19.618Z
Learning: The user plans to revert the change from `path.Join` to `filepath.Join` in this PR due to testing gaps and will open a new PR to safely handle the migration without breaking `main`.
Applied to files:
pkg/utils/yaml_utils.gopkg/utils/yaml_utils_test.go
📚 Learning: 2025-09-08T01:25:44.958Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1466
File: website/docs/cli/commands/toolchain/usage.mdx:117-121
Timestamp: 2025-09-08T01:25:44.958Z
Learning: The atmos toolchain XDG compliance implementation is complete with GetXDGCacheDir() and GetXDGTempCacheDir() functions in toolchain/xdg_cache.go, updated installer.go and toolchain_clean.go to use these helpers, and changed cache paths from ~/.cache/tools-cache to ${XDG_CACHE_HOME}/atmos-toolchain (or ~/.cache/atmos-toolchain when XDG_CACHE_HOME is not set).
Applied to files:
pkg/utils/yaml_utils.go
📚 Learning: 2025-09-08T01:25:44.958Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1466
File: website/docs/cli/commands/toolchain/usage.mdx:117-121
Timestamp: 2025-09-08T01:25:44.958Z
Learning: XDG Base Directory Specification compliance implementation for atmos toolchain is complete: created toolchain/xdg_cache.go with GetXDGCacheDir() and GetXDGTempCacheDir() functions, updated toolchain/installer.go and cmd/toolchain_clean.go to use these XDG helpers, and changed all cache paths from hardcoded ~/.cache/tools-cache to XDG-compliant ${XDG_CACHE_HOME}/atmos-toolchain (or ~/.cache/atmos-toolchain fallback).
Applied to files:
pkg/utils/yaml_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:
pkg/utils/yaml_utils.go
📚 Learning: 2025-11-08T19:56:18.660Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1697
File: internal/exec/oci_utils.go:0-0
Timestamp: 2025-11-08T19:56:18.660Z
Learning: In the Atmos codebase, when a function receives an `*schema.AtmosConfiguration` parameter, it should read configuration values from `atmosConfig.Settings` fields rather than using direct `os.Getenv()` or `viper.GetString()` calls. The Atmos pattern is: viper.BindEnv in cmd/root.go binds environment variables → Viper unmarshals into atmosConfig.Settings via mapstructure → business logic reads from the Settings struct. This provides centralized config management, respects precedence, and enables testability. Example: `atmosConfig.Settings.AtmosGithubToken` instead of `os.Getenv("ATMOS_GITHUB_TOKEN")` in functions like `getGHCRAuth` in internal/exec/oci_utils.go.
Applied to files:
pkg/utils/yaml_utils.go
📚 Learning: 2024-10-20T13:12:46.499Z
Learnt from: haitham911
Repo: cloudposse/atmos PR: 736
File: pkg/config/const.go:6-6
Timestamp: 2024-10-20T13:12:46.499Z
Learning: In `cmd/cmd_utils.go`, it's acceptable to have hardcoded references to `atmos.yaml` in logs, and it's not necessary to update them to use the `CliConfigFileName` constant.
Applied to files:
pkg/utils/yaml_utils.go
📚 Learning: 2024-12-11T18:40:12.808Z
Learnt from: Listener430
Repo: cloudposse/atmos PR: 844
File: cmd/helmfile.go:37-37
Timestamp: 2024-12-11T18:40:12.808Z
Learning: In the atmos project, `cliConfig` is initialized within the `cmd` package in `root.go` and can be used in other command files.
Applied to files:
pkg/utils/yaml_utils.go
📚 Learning: 2025-12-16T18:20:55.614Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T18:20:55.614Z
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/utils/yaml_utils.go
📚 Learning: 2025-12-16T18:20:55.614Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T18:20:55.614Z
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/utils/yaml_utils.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:
pkg/utils/yaml_utils.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/utils/yaml_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:
pkg/utils/yaml_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 **/*_test.go : Every new feature must include comprehensive unit tests targeting >80% code coverage for all packages
Applied to files:
pkg/utils/yaml_utils_test.go
📚 Learning: 2025-12-16T18:20:55.614Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T18:20:55.614Z
Learning: Applies to **/*_test.go : Test behavior, not implementation; never test stub functions; avoid tautological tests; make code testable via DI; no coverage theater; remove always-skipped tests; use errors.Is() for error checking
Applied to files:
pkg/utils/yaml_utils_test.go
📚 Learning: 2025-11-24T17:35:37.209Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-11-24T17:35:37.209Z
Learning: Applies to **/*_test.go : Include integration tests for command flows and test CLI end-to-end when possible with test fixtures
Applied to files:
pkg/utils/yaml_utils_test.go
📚 Learning: 2025-11-24T17:35:37.209Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-11-24T17:35:37.209Z
Learning: Applies to **/*_test.go : Use table-driven tests for testing multiple scenarios in Go
Applied to files:
pkg/utils/yaml_utils_test.go
📚 Learning: 2024-10-23T20:13:23.054Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 731
File: pkg/utils/file_utils.go:198-202
Timestamp: 2024-10-23T20:13:23.054Z
Learning: In `pkg/utils/file_utils.go`, the current implementation of the `IsURL` function is considered sufficient; avoid suggesting more complex URL validation in future reviews.
Applied to files:
pkg/utils/yaml_utils_test.go
📚 Learning: 2025-12-16T18:20:55.614Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T18:20:55.614Z
Learning: Applies to **/*.go : Keep files small and focused (<600 lines); one cmd/impl per file; co-locate tests; never use //revive:disable:file-length-limit
Applied to files:
pkg/utils/yaml_utils_test.go
📚 Learning: 2025-12-16T18:20:55.614Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T18:20:55.614Z
Learning: Applies to **/*_test.go : Prefer unit tests with mocks over integration tests; use interfaces + dependency injection for testability; generate mocks with go.uber.org/mock/mockgen; use table-driven tests; target >80% coverage
Applied to files:
pkg/utils/yaml_utils_test.go
📚 Learning: 2024-11-19T23:00:45.899Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 795
File: internal/exec/stack_processor_utils.go:378-386
Timestamp: 2024-11-19T23:00:45.899Z
Learning: In the `ProcessYAMLConfigFile` function within `internal/exec/stack_processor_utils.go`, directory traversal in stack imports is acceptable and should not be restricted.
Applied to files:
pkg/utils/yaml_utils_test.go
📚 Learning: 2025-05-23T19:51:47.091Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 1255
File: cmd/describe_affected_test.go:15-15
Timestamp: 2025-05-23T19:51:47.091Z
Learning: The atmos codebase has a custom extension to *testing.T that provides a Chdir method, allowing test functions to call t.Chdir() to change working directories during tests. This is used consistently across test files in the codebase.
Applied to files:
pkg/utils/yaml_utils_test.go
📚 Learning: 2024-12-25T20:28:47.526Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 887
File: internal/exec/stack_processor_utils.go:380-380
Timestamp: 2024-12-25T20:28:47.526Z
Learning: Windows path handling often requires `filepath.Join` to ensure correct separators and comparisons. Insufficient tests can break cross-platform compatibility, so migrating from `path.Join` to `filepath.Join` needs thorough testing on Windows before merging.
Applied to files:
pkg/utils/yaml_utils_test.go
📚 Learning: 2025-12-16T18:20:55.614Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T18:20:55.614Z
Learning: Applies to **/*.go : Code must be Linux/macOS/Windows compatible; use SDKs over binaries; use filepath.Join() instead of hardcoded path separators
Applied to files:
pkg/utils/yaml_utils_test.go
📚 Learning: 2025-09-30T19:03:50.738Z
Learnt from: Cerebrovinny
Repo: cloudposse/atmos PR: 1560
File: pkg/utils/string_utils.go:43-64
Timestamp: 2025-09-30T19:03:50.738Z
Learning: In the Atmos codebase, YAML tags like !terraform.output rely on positional arguments, so the SplitStringByDelimiter function in pkg/utils/string_utils.go must preserve empty strings (even after trimming quotes) to maintain the correct number of positional arguments. Filtering out empty values after trimming would collapse the array and break these function calls.
Applied to files:
pkg/utils/yaml_utils_test.go
📚 Learning: 2025-04-24T01:40:13.576Z
Learnt from: haitham911
Repo: cloudposse/atmos PR: 1202
File: pkg/config/process_yaml.go:0-0
Timestamp: 2025-04-24T01:40:13.576Z
Learning: When processing YAML nodes with custom directives in Go using gopkg.in/yaml.v3, setting node.Tag = "" is sufficient to prevent re-processing of the node. It's not necessary to also clear node.Value after updating the configuration store (e.g., Viper), as the original value doesn't affect subsequent operations once the tag is removed.
Applied to files:
pkg/utils/yaml_utils_test.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/utils/yaml_utils_test.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/utils/yaml_utils_test.go
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Acceptance Tests (macos)
- GitHub Check: Acceptance Tests (windows)
- GitHub Check: Summary
🔇 Additional comments (3)
pkg/utils/yaml_utils.go (1)
15-15: LGTM!Import organization follows the guidelines correctly: stdlib packages, then 3rd-party, then Atmos packages.
pkg/utils/yaml_utils_test.go (2)
4-5: LGTM!Import organization is correct, and the new imports are appropriately used in the filesystem-related tests below.
994-1303: Excellent test coverage!The test suite comprehensively covers the new utility functions with edge cases, error conditions, and various data structures. Tests are well-organized and follow best practices.
|
💥 This pull request now has conflicts. Could you fix it @osterman? 🙏 |
|
@coderabbitai full review please |
|
@aknysh: I'll perform a comprehensive review of all changes in this PR. The review will cover:
Starting the full review now. 🧠 Learnings used✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
pkg/provisioner/source/provision_hook.go (1)
162-202: Past review concern addressed.The
determineSourceTargetDirectoryfunction now acceptscomponentTypeas a parameter (line 170) and uses it correctly in the workdir path construction (line 192). This ensures Helmfile and Packer components are vendored to.workdir/helmfile/...and.workdir/packer/...respectively.
🧹 Nitpick comments (1)
tests/fixtures/scenarios/source-provisioner-workdir/README.md (1)
7-12: Consider adding a Prerequisites section.The fixture documents testing steps and directory structure well, but lacks setup details—such as remote source URLs, authentication, or any environment configuration needed before running the tests. A brief prerequisites section would help developers quickly understand what's required.
Also applies to: 93-123
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (13)
internal/exec/helmfile.gointernal/exec/packer.gointernal/exec/stack_processor_merge.gointernal/exec/stack_processor_process_stacks_helpers_extraction.gointernal/exec/terraform.gopkg/provisioner/source/provision_hook.gopkg/provisioner/source/source.gotests/fixtures/scenarios/source-provisioner-workdir/.gitignoretests/fixtures/scenarios/source-provisioner-workdir/README.mdtests/fixtures/scenarios/source-provisioner-workdir/atmos.yamltests/fixtures/scenarios/source-provisioner-workdir/stacks/catalog/helmfile-source-with-workdir.yamltests/fixtures/scenarios/source-provisioner-workdir/stacks/catalog/packer-source-with-workdir.yamltests/fixtures/scenarios/source-provisioner-workdir/stacks/deploy/dev.yaml
🚧 Files skipped from review as they are similar to previous changes (4)
- internal/exec/stack_processor_merge.go
- pkg/provisioner/source/source.go
- tests/fixtures/scenarios/source-provisioner-workdir/.gitignore
- tests/fixtures/scenarios/source-provisioner-workdir/atmos.yaml
🧰 Additional context used
📓 Path-based instructions (2)
**/*.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: Context should be first parameter in functions that accept it
Use I/O layer (pkg/io/) for stream access and UI layer (pkg/ui/) for formatting - use data.Write/Writef/Writeln for stdout, ui.Write/Writef/Writeln for stderr - DO NOT use fmt.Fprintf(os.Stdout/Stderr) or fmt.Println
All comments must end with periods (enforced by godot linter)
Use three-group import organization separated by blank lines, sorted alphabetically: Go stdlib, 3rd-party (NOT cloudposse/atmos), Atmos packages - maintain aliases: cfg, log, u, errUtils
Add defer perf.Track(atmosConfig, "pkg.FuncName")() + blank line to all public functions, 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, fmt.Errorf with %w for adding string context, error builder for complex errors, errors.Is() for error checking
Use go.uber.org/...
Files:
internal/exec/stack_processor_process_stacks_helpers_extraction.gointernal/exec/helmfile.gointernal/exec/terraform.gointernal/exec/packer.gopkg/provisioner/source/provision_hook.go
internal/exec/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
New configs support Go templating with FuncMap() from internal/exec/template_funcs.go
Files:
internal/exec/stack_processor_process_stacks_helpers_extraction.gointernal/exec/helmfile.gointernal/exec/terraform.gointernal/exec/packer.go
🧠 Learnings (33)
📓 Common learnings
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: 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: 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.
📚 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:
internal/exec/stack_processor_process_stacks_helpers_extraction.gointernal/exec/helmfile.gotests/fixtures/scenarios/source-provisioner-workdir/stacks/catalog/helmfile-source-with-workdir.yaml
📚 Learning: 2024-12-07T16:19:01.683Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 825
File: internal/exec/terraform.go:30-30
Timestamp: 2024-12-07T16:19:01.683Z
Learning: In `internal/exec/terraform.go`, skipping stack validation when help flags are present is not necessary.
Applied to files:
internal/exec/stack_processor_process_stacks_helpers_extraction.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/stack_processor_process_stacks_helpers_extraction.gointernal/exec/packer.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/stack_processor_process_stacks_helpers_extraction.gointernal/exec/helmfile.gointernal/exec/terraform.gointernal/exec/packer.gopkg/provisioner/source/provision_hook.go
📚 Learning: 2024-10-31T01:22:09.586Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 737
File: examples/demo-vendoring/vendor.d/vendor1.yaml:10-11
Timestamp: 2024-10-31T01:22:09.586Z
Learning: In `examples/demo-vendoring/vendor.d/vendor1.yaml`, when specifying the source for the `ipinfo` component, it's acceptable to use `main` as the version reference if pinning to a specific version is not practical.
Applied to files:
tests/fixtures/scenarios/source-provisioner-workdir/stacks/catalog/packer-source-with-workdir.yamltests/fixtures/scenarios/source-provisioner-workdir/stacks/catalog/helmfile-source-with-workdir.yaml
📚 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:
internal/exec/helmfile.gotests/fixtures/scenarios/source-provisioner-workdir/stacks/catalog/helmfile-source-with-workdir.yaml
📚 Learning: 2025-12-26T06:31:02.259Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-26T06:31:02.259Z
Learning: Applies to **/*.go : Code must be Linux/macOS/Windows compatible - use SDKs over binaries, use filepath.Join() instead of hardcoded separators
Applied to files:
internal/exec/helmfile.gointernal/exec/packer.go
📚 Learning: 2025-12-26T06:31:02.259Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-26T06:31:02.259Z
Learning: Applies to internal/exec/**/*.go : New configs support Go templating with FuncMap() from internal/exec/template_funcs.go
Applied to files:
internal/exec/helmfile.gointernal/exec/packer.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:
internal/exec/helmfile.gointernal/exec/packer.go
📚 Learning: 2025-12-26T06:31:02.259Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-26T06:31:02.259Z
Learning: Applies to **/*.go : Context should be first parameter in functions that accept it
Applied to files:
internal/exec/helmfile.gointernal/exec/packer.go
📚 Learning: 2025-12-26T06:31:02.259Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-26T06:31:02.259Z
Learning: Applies to **/*.go : Use three-group import organization separated by blank lines, sorted alphabetically: Go stdlib, 3rd-party (NOT cloudposse/atmos), Atmos packages - maintain aliases: cfg, log, u, errUtils
Applied to files:
internal/exec/helmfile.gointernal/exec/terraform.gointernal/exec/packer.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/helmfile.gointernal/exec/terraform.gointernal/exec/packer.go
📚 Learning: 2025-12-26T06:31:02.259Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-26T06:31:02.259Z
Learning: Applies to **/*.go : Add defer perf.Track(atmosConfig, "pkg.FuncName")() + blank line to all public functions, use nil if no atmosConfig param
Applied to files:
internal/exec/helmfile.gointernal/exec/packer.go
📚 Learning: 2025-09-10T21:17:55.273Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 1466
File: toolchain/http_client_test.go:3-10
Timestamp: 2025-09-10T21:17:55.273Z
Learning: In the cloudposse/atmos repository, imports should never be changed as per samtholiya's coding guidelines.
Applied to files:
internal/exec/helmfile.gointernal/exec/terraform.gotests/fixtures/scenarios/source-provisioner-workdir/stacks/deploy/dev.yamlinternal/exec/packer.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/helmfile.gointernal/exec/packer.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:
internal/exec/helmfile.gointernal/exec/packer.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/helmfile.gointernal/exec/packer.go
📚 Learning: 2024-11-12T03:16:02.910Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 775
File: internal/exec/template_funcs_component.go:157-159
Timestamp: 2024-11-12T03:16:02.910Z
Learning: In the Go code for `componentFunc` in `internal/exec/template_funcs_component.go`, the function `cleanTerraformWorkspace` does not return errors, and it's acceptable if the file does not exist. Therefore, error handling for `cleanTerraformWorkspace` is not needed.
Applied to files:
internal/exec/helmfile.gointernal/exec/terraform.gointernal/exec/packer.gopkg/provisioner/source/provision_hook.go
📚 Learning: 2025-12-17T20:55:47.884Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1874
File: internal/exec/describe_affected_utils_test.go:436-468
Timestamp: 2025-12-17T20:55:47.884Z
Learning: In the Atmos codebase, there are two different paths for the `locked` flag: (1) filtering logic in `internal/exec/component_utils.go` (`isComponentLocked()`) reads from `componentSection["metadata"]["locked"]` to determine which components to include/exclude, and (2) extraction/rendering logic in `pkg/list/extract/affected.go` reads from `settings.metadata.locked` to display the locked status in output. Tests for filtering behavior should use `metadata.locked`.
Applied to files:
internal/exec/helmfile.gointernal/exec/packer.gopkg/provisioner/source/provision_hook.go
📚 Learning: 2025-11-24T17:35:37.209Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-11-24T17:35:37.209Z
Learning: Applies to **/*_test.go : Include integration tests for command flows and test CLI end-to-end when possible with test fixtures
Applied to files:
tests/fixtures/scenarios/source-provisioner-workdir/README.md
📚 Learning: 2024-11-02T15:35:09.958Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 759
File: internal/exec/terraform.go:366-368
Timestamp: 2024-11-02T15:35:09.958Z
Learning: In `internal/exec/terraform.go`, the workspace cleaning code under both the general execution path and within the `case "init":` block is intentionally duplicated because the code execution paths are different. The `.terraform/environment` file should be deleted before executing `terraform init` in both scenarios to ensure a clean state.
Applied to files:
internal/exec/terraform.gopkg/provisioner/source/provision_hook.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:
internal/exec/terraform.gointernal/exec/packer.gopkg/provisioner/source/provision_hook.go
📚 Learning: 2024-11-25T17:17:15.703Z
Learnt from: RoseSecurity
Repo: cloudposse/atmos PR: 797
File: pkg/list/atmos.yaml:213-214
Timestamp: 2024-11-25T17:17:15.703Z
Learning: The file `pkg/list/atmos.yaml` is primarily intended for testing purposes.
Applied to files:
tests/fixtures/scenarios/source-provisioner-workdir/stacks/deploy/dev.yaml
📚 Learning: 2024-12-01T00:33:20.298Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 810
File: examples/tests/stacks/catalog/terraform/template-functions-test2/defaults.yaml:28-32
Timestamp: 2024-12-01T00:33:20.298Z
Learning: In `examples/tests/stacks/catalog/terraform/template-functions-test2/defaults.yaml`, `!exec atmos terraform output` is used in examples to demonstrate its usage, even though `!terraform.output` is the recommended approach according to the documentation.
Applied to files:
tests/fixtures/scenarios/source-provisioner-workdir/stacks/deploy/dev.yaml
📚 Learning: 2025-01-25T03:51:57.689Z
Learnt from: Listener430
Repo: cloudposse/atmos PR: 934
File: tests/fixtures/scenarios/docs-generate/README.md.gotmpl:99-118
Timestamp: 2025-01-25T03:51:57.689Z
Learning: For the cloudposse/atmos repository, changes to template contents should be handled in dedicated PRs and are typically considered out of scope for PRs focused on other objectives.
Applied to files:
tests/fixtures/scenarios/source-provisioner-workdir/stacks/deploy/dev.yaml
📚 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:
pkg/provisioner/source/provision_hook.go
📚 Learning: 2024-10-27T04:41:49.199Z
Learnt from: haitham911
Repo: cloudposse/atmos PR: 727
File: internal/exec/terraform_clean.go:215-223
Timestamp: 2024-10-27T04:41:49.199Z
Learning: In `internal/exec/terraform_clean.go`, the function `determineCleanPath` is necessary and should not be removed.
Applied to files:
pkg/provisioner/source/provision_hook.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:
pkg/provisioner/source/provision_hook.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/provisioner/source/provision_hook.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/provisioner/source/provision_hook.go
📚 Learning: 2024-10-27T04:28:40.966Z
Learnt from: haitham911
Repo: cloudposse/atmos PR: 727
File: internal/exec/terraform_clean.go:155-175
Timestamp: 2024-10-27T04:28:40.966Z
Learning: In the `CollectDirectoryObjects` function in `internal/exec/terraform_clean.go`, recursive search through all subdirectories is not needed.
Applied to files:
pkg/provisioner/source/provision_hook.go
📚 Learning: 2025-12-24T04:29:23.938Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1891
File: pkg/ci/terraform/templates/apply.md:17-17
Timestamp: 2025-12-24T04:29:23.938Z
Learning: In the cloudposse/atmos repository, Terraform CI templates (pkg/ci/terraform/templates/*.md) are rendered using TerraformTemplateContext (defined in pkg/ci/terraform/context.go), not the base ci.TemplateContext. TerraformTemplateContext provides top-level fields: .Resources (ci.ResourceCounts), .HasChanges() method, and .HasDestroy field, which are correctly accessed directly in templates without a .Result prefix.
Applied to files:
pkg/provisioner/source/provision_hook.go
🧬 Code graph analysis (2)
internal/exec/stack_processor_process_stacks_helpers_extraction.go (2)
pkg/config/const.go (4)
TerraformComponentType(57-57)HelmfileComponentType(58-58)PackerComponentType(59-59)SourceSectionName(78-78)errors/errors.go (1)
ErrInvalidTerraformSource(372-372)
pkg/provisioner/source/provision_hook.go (6)
pkg/perf/perf.go (1)
Track(121-138)pkg/schema/vendor_component.go (1)
VendorComponentSource(5-12)pkg/provisioner/source/extract.go (1)
ExtractSource(17-29)errors/errors.go (1)
ErrSourceProvision(672-672)pkg/provisioner/source/vendor.go (1)
VendorSource(31-95)pkg/provisioner/source/source.go (1)
DetermineTargetDirectory(122-147)
⏰ 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 (12)
tests/fixtures/scenarios/source-provisioner-workdir/README.md (1)
1-123: Documentation is accurate—no changes needed.The
source pullcommand referenced in the README (lines 44, 57, 67) is properly implemented across terraform, helmfile, and packer (confirmed incmd/*/source/source.go). While the PR objectives listed specific operations,pullis the implemented command for acquiring vendored sources, and it works correctly for all three component types as documented.internal/exec/terraform.go (1)
182-212: JIT provisioning integration looks solid.The logic correctly:
- Checks for source configuration before attempting provisioning
- Passes the correct component type (
cfg.TerraformComponentType)- Handles both workdir path override and standard re-check scenarios
- Preserves the original error behavior when provisioning doesn't resolve the path
One minor observation: using
context.Background()here is acceptable for now since there's no parent context available, but consider whether a timeout context would be more appropriate for production resilience.internal/exec/helmfile.go (1)
85-115: Consistent with Terraform implementation.The JIT provisioning logic mirrors the Terraform implementation correctly, using
cfg.HelmfileComponentTypefor proper path resolution. Good.internal/exec/packer.go (1)
80-110: Packer provisioning follows the established pattern.Implementation aligns with Terraform and Helmfile. Uses
cfg.PackerComponentTypecorrectly.pkg/provisioner/source/provision_hook.go (5)
62-102: Clean orchestration logic.The function properly coordinates extraction, target determination, and vendoring. Mutating
componentConfigto setWorkdirPathKeyis intentional for passing the workdir path back to callers.
220-240: Defensive directory checking.The function handles edge cases well: non-existent, inaccessible, non-directory, and empty directories all trigger provisioning. This is appropriately defensive.
242-257: Component name extraction is straightforward.The fallback from
componenttometadata.componenthandles common configuration patterns. The caller appropriately handles the empty-string case.
126-151: Proper UI and error handling.Uses
ui.Infoandui.Successfor status messages per coding guidelines. Error wrapping includes helpful context (component, source URI, target path) and hints.
29-37: Hook registration is terraform-only by design.The hook registers only for
before.terraform.init, which is intentional. Helmfile and Packer callAutoProvisionSourcedirectly from their execution paths rather than through the hook system. This is a reasonable design choice.internal/exec/stack_processor_process_stacks_helpers_extraction.go (1)
91-104: Good extension to multi-component-type provisioning.The provision section now correctly supports Terraform, Helmfile, and Packer components, aligning with the PR's goal of enabling JIT provisioning across all three types.
tests/fixtures/scenarios/source-provisioner-workdir/stacks/deploy/dev.yaml (1)
1-39: Well-structured test fixture.The manifest cleanly imports catalogs by component type and consistently overrides environment vars for dev. No issues.
tests/fixtures/scenarios/source-provisioner-workdir/stacks/catalog/packer-source-with-workdir.yaml (1)
11-18: The external GitHub repository is currently accessible and contains the expected Packer configuration files. The fixture is correctly configured.The underlying concern about external dependencies introducing brittleness remains architecturally valid—if the repository were to be moved, deleted, or significantly changed, tests relying on it would break. Consider whether this external dependency aligns with your test stability strategy, or evaluate mitigations like pinning to a specific release tag instead of
main.
...xtures/scenarios/source-provisioner-workdir/stacks/catalog/helmfile-source-with-workdir.yaml
Show resolved
Hide resolved
The blog post previously only mentioned Terraform for the source provisioner feature. Updated to include all three supported component types: - Updated intro to mention Terraform, Helmfile, and Packer - Added helmfile and packer examples in the YAML configuration - Added helmfile and packer CLI commands section - Updated workdir section to mention all component types - Updated directory structure to show all three component types - Updated documentation links to include helmfile and packer 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
|
@aknysh: I'll perform a comprehensive review of all changes in PR #1877 implementing the source provisioner for JIT component vendoring. The review will cover: Core Provisioning Logic
CLI Commands
Schema & Stack Processing
Error Handling & Retry
Integration & Hooks
Test Coverage
Documentation
Starting the full review now. 🧠 Learnings used✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (4)
website/docs/design-patterns/version-management/strict-version-pinning.mdx (1)
36-38: Typo still present despite previous fix.The past review comment flagged "Simper" → "Simpler" and was marked as addressed in commits d5b0d86 to 2bad1a2, but the typo still appears in the current code.
🔎 Apply this diff
-:::tip Simper Alternative: Source-Based Version Pinning +:::tip Simpler Alternative: Source-Based Version Pinningwebsite/docs/cli/commands/terraform/source/source.mdx (1)
431-431: Link may need correction.Per a past review comment,
/cli/commands/vendor/pullshould be/cli/commands/vendor/vendor-pullto match the actual filevendor-pull.mdx.pkg/provisioner/source/cmd/describe.go (1)
17-45: Missing perf.Track on DescribeCommand.Past review suggested adding perf.Track. This wasn't addressed yet.
🔎 Proposed fix
func DescribeCommand(cfg *Config) *cobra.Command { + defer perf.Track(nil, "source.cmd.DescribeCommand")() + parser := flags.NewStandardParser(pkg/downloader/get_git_retry_test.go (1)
273-341: MissingintToStringhelper function.Lines 294 and 310 reference
intToString(failCount), but this function isn't defined. Usestrconv.Itoadirectly or add the helper.🔎 Proposed fix
Add after line 341:
+// intToString converts an int to string. +func intToString(n int) string { + return strconv.Itoa(n) +}Or replace usages with
strconv.Itoa(failCount)directly.
🧹 Nitpick comments (13)
tests/fixtures/scenarios/source-provisioner/README.md (1)
1-167: Well-structured and comprehensive fixture documentation.This README effectively documents the source provisioner test fixture with clear sections, runnable examples, and all three configuration forms. The manual testing steps cover the main workflows and the directory structure is easy to follow.
Consider adding clarification on:
- What's in the
.gitignorefile (line 109)- Other authentication methods beyond
GITHUB_TOKEN(lines 84–92)These are nice-to-have enhancements rather than blockers.
website/docs/cli/configuration/vendor.mdx (2)
34-40: Consider mentioning that additional retry fields are available.The example shows 4 core retry fields, but the documentation below describes 7 fields total (adding
multiplier,random_jitter, andmax_elapsed_time). Users who only reference the example might miss these additional configuration options.🔎 Suggested addition to the example
# Global retry configuration for vendor operations (optional) + # See Configuration Reference below for additional retry options retry: max_attempts: 3 initial_delay: 2s max_delay: 30s backoff_strategy: exponential
81-83: Add source-level retry example to the vendor manifest.The documentation correctly states that global retry settings can be "overridden at the source level," and the code supports this. However, the vendor manifest example doesn't show how. Add a source-level retry configuration to one of the source entries so users understand how to override global settings for individual sources.
internal/exec/path_utils.go (1)
88-120: Consistent pattern across component types.The Helmfile workdir resolution mirrors the Terraform implementation exactly. The duplication is acceptable for clarity, though a shared helper could reduce maintenance burden if more component types are added.
website/docs/cli/commands/terraform/source/delete.mdx (1)
95-97: Minor: typographical style.Static analysis flagged the use of hyphens vs em dashes. This is stylistic and not blocking.
Optional: use em dashes for list items
-1. **Requires --force** - Prevents accidental deletion -2. **Only deletes source-managed components** - Only works on components with `source` configured -3. **Non-destructive on missing directories** - Shows a warning if the directory doesn't exist instead of failing +1. **Requires --force** — Prevents accidental deletion +2. **Only deletes source-managed components** — Works only on components with `source` configured +3. **Non-destructive on missing directories** — Shows a warning if the directory doesn't exist instead of failingpkg/provisioner/source/cmd/list.go (1)
15-42: Consider adding perf.Track to ListCommand.Per coding guidelines, public functions should include
defer perf.Track(). The pattern is used in other command constructors.🔎 Proposed fix
func ListCommand(cfg *Config) *cobra.Command { + defer perf.Track(nil, "source.cmd.ListCommand")() + parser := flags.NewStandardParser( flags.WithStackFlag(), )pkg/provisioner/source/cmd/pull.go (1)
16-52: Consider adding perf.Track to PullCommand.Same as ListCommand - public function should have perf tracking per guidelines.
🔎 Proposed fix
func PullCommand(cfg *Config) *cobra.Command { + defer perf.Track(nil, "source.cmd.PullCommand")() + parser := flags.NewStandardParser(pkg/provisioner/source/extract.go (1)
95-104: Consider warning on non-string items.
toStringSlicesilently skips non-string items. For debugging, a warning might help users identify misconfigured paths.🔎 Optional enhancement
func toStringSlice(items []any) []string { result := make([]string, 0, len(items)) for _, item := range items { if s, ok := item.(string); ok { result = append(result, s) + } else { + _ = ui.Warningf("ignoring non-string path item: %T", item) } } return result }tests/cli_source_provisioner_test.go (1)
43-54: Consider usingassert.Containsfor cleaner error assertions.The OR condition works but
assert.Containswith a single expected substring is more direct when possible.- assert.True(t, strings.Contains(err.Error(), "source") || strings.Contains(err.Error(), "metadata"), - "Expected error about missing source") + // Either "source" or "metadata" in error is acceptable. + errStr := err.Error() + assert.True(t, strings.Contains(errStr, "source") || strings.Contains(errStr, "metadata"), + "Expected error about missing source, got: %s", errStr)pkg/provisioner/source/cmd/helpers_test.go (2)
52-55: Redundant SetArgs call.
SetArgsis unnecessary here since you're callingParseFlagsdirectly. Remove the unused line.- // Set the flags via command line parsing. - cmd.SetArgs([]string{"--stack", "dev-us-east-1"}) err := cmd.ParseFlags([]string{"--stack", "dev-us-east-1"})
450-451: Remove the unused import workaround.This
var _ = cfg.InitCliConfigexists only to satisfy the compiler. Ifcfgis truly unused, remove the import instead. If it's needed for side effects elsewhere, document why.-// Ensure package imports are used. -var _ = cfg.InitCliConfigpkg/provisioner/source/cmd/helpers.go (1)
97-101: Consider adding context to the config init error.The
errors.Joinpattern works, but component/stack context would help troubleshooting.atmosConfig, err := initCliConfigFunc(configInfo, false) if err != nil { - return nil, nil, errors.Join(errUtils.ErrFailedToInitConfig, err) + return nil, nil, fmt.Errorf("component=%s stack=%s: %w", component, stack, errors.Join(errUtils.ErrFailedToInitConfig, err)) }pkg/provisioner/source/provision_hook.go (1)
242-257: Fallback component name extraction.The two-level lookup (direct field, then metadata) matches how components can be configured. Consider documenting which takes precedence in a brief comment.
// extractComponentName extracts the component name from config. +// Priority: componentConfig["component"] > metadata["component"]. func extractComponentName(componentConfig map[string]any) string {
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (6)
pkg/utils/yaml_test.go (1)
31-61: LGTM! Solid test isolation with mocked GitHub interactions.The test setup properly isolates network dependencies by:
- Bypassing GitHub rate limiting (preventing 24+ minute waits on CI)
- Injecting a mock HTTP server for deterministic responses
- Using t.Cleanup for proper teardown
This ensures tests run fast and reliably without hitting real GitHub APIs.
Note: Comment formatting issue flagged in previous review (missing periods on lines 32, 40, 44, 57-58) still present. Apply the diff from the past review comment to address.
internal/exec/stack_processor_process_stacks_helpers_extraction.go (1)
157-170: Source extraction logic is solid.The implementation follows the established pattern. The error sentinel naming concern (
ErrInvalidTerraformSourceused for non-Terraform components) was already flagged in a prior review. Based on learnings, this cleanup can be addressed in a separate refactor PR.website/docs/cli/commands/terraform/source/source.mdx (2)
267-267: Verify authentication documentation link.Line 267 references
/stacks/authfor authentication configuration. Based on previous review feedback, this path should be verified. The correct path might be/cli/commands/author a dedicated authentication configuration guide if one exists.#!/bin/bash # Verify the authentication documentation paths exist echo "=== Checking for authentication documentation ===" fd -i "auth" website/docs -e mdx -e md | grep -E "(stacks|cli/commands)" | head -20
431-431: Fix vendor pull documentation link.Line 431 references
/cli/commands/vendor/pullbut based on previous review, the actual filename isvendor-pull.mdx. Update this to/cli/commands/vendor/vendor-pull.🔎 Proposed fix
-- [`atmos vendor pull`](/cli/commands/vendor/pull) — Traditional component vendoring +- [`atmos vendor pull`](/cli/commands/vendor/vendor-pull) — Traditional component vendoringpkg/downloader/get_git_retry_test.go (1)
273-330: MissingintToStringhelper function.Lines 294 and 310 call
intToString(failCount), but this function isn't defined in the file. The tests won't compile.🔎 Add the missing helper function
+// intToString converts an int to string for use in scripts. +func intToString(n int) string { + return strconv.Itoa(n) +}Add this helper after line 341 (after
readCounter).pkg/github/ratelimit.go (1)
82-84: Comment missing period.The comment on line 83 doesn't end with a period.
🔎 Proposed fix
// RateLimitWaiter is the function used to wait for rate limits. -// Tests can override this to skip the actual wait +// Tests can override this to skip the actual wait. var RateLimitWaiter = waitForRateLimitImpl
🧹 Nitpick comments (8)
tests/fixtures/scenarios/source-provisioner/components/terraform/mock/main.tf (1)
1-2: Consider clarifying the comment.The phrase "without source" might be a bit ambiguous here—could mean either "no source attribute" or "no real resources." Based on context, it looks like you meant the latter. Something like "Mock component for testing provisioning without real resources" might be clearer.
website/docs/cli/commands/helmfile/source/delete.mdx (1)
1-105: LGTM!The documentation follows all required guidelines for CLI command pages: complete frontmatter, Intro component, Screengrab, Usage, Arguments/Flags in definition lists, and Examples. The safety features are well-documented, and the See Also section provides helpful cross-references.
The static analysis tool suggests minor style improvements (using em dashes instead of hyphens in lines 95-97, removing duplicate "only" in line 96), but these are purely stylistic nitpicks that don't affect clarity. Feel free to address or ignore based on your documentation style preferences.
website/static/schemas/atmos/atmos-manifest/1.0/atmos-manifest.json (2)
1332-1385: Consider adding duration format examples to field descriptions.The
initial_delay,max_delay, andmax_elapsed_timefields expect duration strings (e.g., '1s', '500ms') but the schema cannot validate the format. Consider enhancing the descriptions with examples to guide users, such as:"description": "Initial delay between retries. Examples: '1s', '500ms', '2m'. Valid units: ns, us, ms, s, m, h"This would reduce configuration errors and improve developer experience.
1280-1331: Document the version precedence behavior in schema descriptions.The codebase already handles dual version specification—if both URI query string (
?ref=) and the separateversionfield are specified, the URI parameter takes precedence and the separate field is silently ignored. Consider updating the schema descriptions to clarify this behavior, especially in the object form'suriandversionfield descriptions, to prevent user confusion when both are accidentally specified.internal/exec/stack_processor_merge.go (1)
308-308: Inconsistent error wrapping format.Line 308 uses
%vfor the inner error, but line 313 uses%w. For consistent error chain behavior witherrors.Is/errors.As, prefer%w.Suggested fix
- return nil, fmt.Errorf("%w: merge auth config: %v", errUtils.ErrInvalidAuthConfig, err) + return nil, fmt.Errorf("%w: merge auth config: %w", errUtils.ErrInvalidAuthConfig, err)internal/exec/terraform.go (1)
185-188: Consider adding timeout for source provisioning.The
AutoProvisionSourcecall usescontext.Background()without a timeout, while other provisioner calls in this file (lines 425, 533) usecontext.WithTimeout(5*time.Minute). This inconsistency could cause the command to hang if source download stalls.Suggested fix
- ctx := context.Background() - if err := provSource.AutoProvisionSource(ctx, &atmosConfig, cfg.TerraformComponentType, info.ComponentSection, info.AuthContext); err != nil { + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Minute) + defer cancel() + if err := provSource.AutoProvisionSource(ctx, &atmosConfig, cfg.TerraformComponentType, info.ComponentSection, info.AuthContext); err != nil {tests/cli_source_provisioner_test.go (1)
56-61: Skipped test documents Viper state leakage.Good documentation of the known issue. Consider tracking this as a follow-up to fix Viper state isolation between tests.
Do you want me to open an issue to track fixing the Viper state leakage between tests?
pkg/provisioner/source/cmd/helpers_test.go (1)
450-451: Consider removing the blank import usage.Line 451 uses
var _ = cfg.InitCliConfigto satisfy the import. Ifcfgisn't used elsewhere in tests, consider removing the import instead of this workaround.
tests/fixtures/scenarios/source-provisioner/stacks/catalog/vpc-source-retry.yaml
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
pkg/provisioner/source/provision_hook_test.go (1)
403-432: LGTM! Error wrapping validated.The test confirms that
wrapProvisionErroralways returns an error (even with nil input) and properly wraps it asErrSourceProvision.Optional: Enhance error message validation
Consider adding assertions to verify that the error message includes the explanation and component name:
t.Run(tt.name, func(t *testing.T) { result := wrapProvisionError(tt.err, tt.explanation, tt.component) require.Error(t, result) // Verify error is of expected type. assert.ErrorIs(t, result, errUtils.ErrSourceProvision) + // Verify error message contains context. + assert.Contains(t, result.Error(), tt.explanation) + assert.Contains(t, result.Error(), tt.component) })internal/exec/packer.go (1)
85-85: Consider making the provisioning timeout configurable.The 5-minute timeout is hardcoded. For large sources or slow networks, this might be insufficient. Consider reading from
atmosConfig.Settingsor allowing configuration viaatmos.yaml.
Tests were failing on Windows because expected paths used Unix-style forward slashes while actual paths used Windows-style backslashes. Updated test assertions to use filepath.FromSlash() which converts forward slashes to the native path separator on each platform. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
The CodeRabbit suggestion to assert on error message content doesn't apply because ErrorBuilder stores explanation and context as enrichments that are not included in the .Error() string representation. Added a comment explaining this behavior. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
- Use filepath.Join for both test inputs and expected values - Workdir paths stored in config are returned verbatim, so test input must match expected output exactly - Constructed paths use filepath.Join to get native separators - Removes filepath.FromSlash calls that caused Windows mismatches 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
📜 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 (2)
internal/exec/path_utils_test.gopkg/provisioner/source/provision_hook_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/provisioner/source/provision_hook_test.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: Context should be first parameter in functions that accept it
Use I/O layer (pkg/io/) for stream access and UI layer (pkg/ui/) for formatting - use data.Write/Writef/Writeln for stdout, ui.Write/Writef/Writeln for stderr - DO NOT use fmt.Fprintf(os.Stdout/Stderr) or fmt.Println
All comments must end with periods (enforced by godot linter)
Use three-group import organization separated by blank lines, sorted alphabetically: Go stdlib, 3rd-party (NOT cloudposse/atmos), Atmos packages - maintain aliases: cfg, log, u, errUtils
Add defer perf.Track(atmosConfig, "pkg.FuncName")() + blank line to all public functions, 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, fmt.Errorf with %w for adding string context, error builder for complex errors, errors.Is() for error checking
Use go.uber.org/...
Files:
internal/exec/path_utils_test.go
**/*_test.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
**/*_test.go: Every new feature must include comprehensive unit tests targeting >80% code coverage for all packages
Use table-driven tests for testing multiple scenarios in Go
Include integration tests for command flows and test CLI end-to-end when possible with test fixturesUse table-driven tests for comprehensive coverage and target >80% coverage
Files:
internal/exec/path_utils_test.go
internal/exec/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
New configs support Go templating with FuncMap() from internal/exec/template_funcs.go
Files:
internal/exec/path_utils_test.go
🧠 Learnings (19)
📓 Common learnings
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: 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.
📚 Learning: 2024-12-25T20:28:47.526Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 887
File: internal/exec/stack_processor_utils.go:380-380
Timestamp: 2024-12-25T20:28:47.526Z
Learning: Windows path handling often requires `filepath.Join` to ensure correct separators and comparisons. Insufficient tests can break cross-platform compatibility, so migrating from `path.Join` to `filepath.Join` needs thorough testing on Windows before merging.
Applied to files:
internal/exec/path_utils_test.go
📚 Learning: 2025-12-26T06:31:02.259Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-26T06:31:02.259Z
Learning: Applies to **/*_test.go : Use table-driven tests for comprehensive coverage and target >80% coverage
Applied to files:
internal/exec/path_utils_test.go
📚 Learning: 2025-11-24T17:35:37.209Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-11-24T17:35:37.209Z
Learning: Applies to **/*_test.go : Use table-driven tests for testing multiple scenarios in Go
Applied to files:
internal/exec/path_utils_test.go
📚 Learning: 2025-05-23T19:51:47.091Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 1255
File: cmd/describe_affected_test.go:15-15
Timestamp: 2025-05-23T19:51:47.091Z
Learning: The atmos codebase has a custom extension to *testing.T that provides a Chdir method, allowing test functions to call t.Chdir() to change working directories during tests. This is used consistently across test files in the codebase.
Applied to files:
internal/exec/path_utils_test.go
📚 Learning: 2024-10-28T01:51:30.811Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 727
File: internal/exec/terraform_clean.go:329-332
Timestamp: 2024-10-28T01:51:30.811Z
Learning: In the Atmos Go code, when deleting directories or handling file paths (e.g., in `terraform_clean.go`), always resolve the absolute path using `filepath.Abs` and use the logger `u.LogWarning` for logging messages instead of using `fmt.Printf`.
Applied to files:
internal/exec/path_utils_test.go
📚 Learning: 2025-12-13T06:10:25.156Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1686
File: internal/exec/workflow_utils.go:0-0
Timestamp: 2025-12-13T06:10:25.156Z
Learning: Atmos workflows: In internal/exec/workflow_utils.go ExecuteWorkflow, non-identity steps intentionally use baseWorkflowEnv, which is constructed from the parent environment with PATH modifications for the toolchain. Avoid appending os.Environ() again; prefer documenting this behavior and testing that standard environment variables are preserved.
Applied to files:
internal/exec/path_utils_test.go
📚 Learning: 2025-11-24T17:35:37.209Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-11-24T17:35:37.209Z
Learning: Applies to **/*_test.go : Include integration tests for command flows and test CLI end-to-end when possible with test fixtures
Applied to files:
internal/exec/path_utils_test.go
📚 Learning: 2024-12-25T20:28:19.618Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 887
File: internal/exec/workflow_utils.go:167-169
Timestamp: 2024-12-25T20:28:19.618Z
Learning: The user plans to revert the change from `path.Join` to `filepath.Join` in this PR due to testing gaps and will open a new PR to safely handle the migration without breaking `main`.
Applied to files:
internal/exec/path_utils_test.go
📚 Learning: 2025-05-23T19:51:47.091Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 1255
File: cmd/describe_affected_test.go:15-15
Timestamp: 2025-05-23T19:51:47.091Z
Learning: In the atmos codebase, t.Chdir() is a valid method call on *testing.T objects and works correctly for changing directories in tests. This is implemented through custom testing framework extensions and is used consistently throughout the test suite.
Applied to files:
internal/exec/path_utils_test.go
📚 Learning: 2025-12-26T06:31:02.259Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-26T06:31:02.259Z
Learning: Applies to **/*.go : Code must be Linux/macOS/Windows compatible - use SDKs over binaries, use filepath.Join() instead of hardcoded separators
Applied to files:
internal/exec/path_utils_test.go
📚 Learning: 2025-05-23T19:51:47.091Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 1255
File: cmd/describe_affected_test.go:15-15
Timestamp: 2025-05-23T19:51:47.091Z
Learning: In the atmos codebase, t.Chdir() is a valid method call on *testing.T objects and works correctly for changing directories in tests.
Applied to files:
internal/exec/path_utils_test.go
📚 Learning: 2025-05-23T19:51:47.091Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 1255
File: cmd/describe_affected_test.go:15-15
Timestamp: 2025-05-23T19:51:47.091Z
Learning: In the atmos codebase, t.Chdir() is a valid method that can be called on *testing.T objects. This functionality is implemented through custom testing framework extensions and is used consistently throughout the test suite for changing working directories during tests.
Applied to files:
internal/exec/path_utils_test.go
📚 Learning: 2025-11-11T03:47:45.878Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1686
File: toolchain/add_test.go:67-77
Timestamp: 2025-11-11T03:47:45.878Z
Learning: In the cloudposse/atmos codebase, tests should prefer t.Setenv for environment variable setup/teardown instead of os.Setenv/Unsetenv to ensure test-scoped isolation.
Applied to files:
internal/exec/path_utils_test.go
📚 Learning: 2024-11-12T03:16:02.910Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 775
File: internal/exec/template_funcs_component.go:157-159
Timestamp: 2024-11-12T03:16:02.910Z
Learning: In the Go code for `componentFunc` in `internal/exec/template_funcs_component.go`, the function `cleanTerraformWorkspace` does not return errors, and it's acceptable if the file does not exist. Therefore, error handling for `cleanTerraformWorkspace` is not needed.
Applied to files:
internal/exec/path_utils_test.go
📚 Learning: 2025-11-11T03:47:59.576Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1686
File: toolchain/which_test.go:166-223
Timestamp: 2025-11-11T03:47:59.576Z
Learning: In the cloudposse/atmos repo, tests that manipulate environment variables should use testing.T.Setenv for automatic setup/teardown instead of os.Setenv/Unsetenv.
Applied to files:
internal/exec/path_utils_test.go
📚 Learning: 2025-12-10T18:32:51.237Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1808
File: cmd/terraform/backend/backend_delete_test.go:9-23
Timestamp: 2025-12-10T18:32:51.237Z
Learning: In cmd subpackages (e.g., cmd/terraform/backend/), tests cannot use cmd.NewTestKit(t) due to Go's test visibility rules (NewTestKit is in a parent package test file). These tests only need TestKit if they execute commands through RootCmd or modify RootCmd state. Structural tests that only verify command structure/flags without touching RootCmd don't require TestKit cleanup.
Applied to files:
internal/exec/path_utils_test.go
📚 Learning: 2025-11-10T23:23:39.771Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1686
File: toolchain/registry/aqua/aqua_test.go:417-442
Timestamp: 2025-11-10T23:23:39.771Z
Learning: In Atmos toolchain AquaRegistry, tests should not hit real GitHub. Use the options pattern via WithGitHubBaseURL to inject an httptest server URL and make GetLatestVersion/GetAvailableVersions deterministic.
Applied to files:
internal/exec/path_utils_test.go
📚 Learning: 2025-12-21T04:10:29.030Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1891
File: internal/exec/describe_affected.go:468-468
Timestamp: 2025-12-21T04:10:29.030Z
Learning: In Go, package-level declarations (constants, variables, types, and functions) are visible to all files in the same package without imports. During reviews in cloudposse/atmos (and similar Go codebases), before suggesting to declare a new identifier, first check if it already exists in another file of the same package. If it exists, you can avoid adding a new declaration; if not, proceed with a proper package-level declaration.
Applied to files:
internal/exec/path_utils_test.go
🧬 Code graph analysis (1)
internal/exec/path_utils_test.go (2)
pkg/schema/schema.go (5)
AtmosConfiguration(54-99)ConfigAndStacksInfo(692-788)Terraform(388-400)Helmfile(414-421)Packer(423-426)pkg/provisioner/workdir/types.go (1)
WorkdirPathKey(73-73)
🪛 GitHub Actions: Tests
internal/exec/path_utils_test.go
[error] 81-81: TestConstructTerraformComponentWorkingDir_WithWorkdirPath: expected "\workdir\terraform\dev-vpc" but got "/workdir/terraform/dev-vpc".
[error] 105-105: TestConstructHelmfileComponentWorkingDir_WithWorkdirPath: expected "\workdir\helmfile\dev-nginx" but got "/workdir/helmfile/dev-nginx".
[error] 136-136: TestConstructPackerComponentWorkingDir_WithWorkdirPath: expected "\workdir\packer\dev-ami" but got "/workdir/packer/dev-ami".
⏰ 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 (2)
internal/exec/path_utils_test.go (2)
3-11: Import organization looks good.The three-group structure (stdlib, 3rd-party, atmos) with proper aliasing follows the coding guidelines.
148-282: Existing Packer tests handle cross-platform paths correctly.These table-driven tests properly use
filepath.FromSlash()to normalize expected paths, ensuring Windows compatibility. The structure is solid.
There was a problem hiding this comment.
Actionable comments posted: 3
📜 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 (2)
internal/exec/path_utils_test.gopkg/provisioner/source/provision_hook_test.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: Context should be first parameter in functions that accept it
Use I/O layer (pkg/io/) for stream access and UI layer (pkg/ui/) for formatting - use data.Write/Writef/Writeln for stdout, ui.Write/Writef/Writeln for stderr - DO NOT use fmt.Fprintf(os.Stdout/Stderr) or fmt.Println
All comments must end with periods (enforced by godot linter)
Use three-group import organization separated by blank lines, sorted alphabetically: Go stdlib, 3rd-party (NOT cloudposse/atmos), Atmos packages - maintain aliases: cfg, log, u, errUtils
Add defer perf.Track(atmosConfig, "pkg.FuncName")() + blank line to all public functions, 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, fmt.Errorf with %w for adding string context, error builder for complex errors, errors.Is() for error checking
Use go.uber.org/...
Files:
pkg/provisioner/source/provision_hook_test.gointernal/exec/path_utils_test.go
**/*_test.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
**/*_test.go: Every new feature must include comprehensive unit tests targeting >80% code coverage for all packages
Use table-driven tests for testing multiple scenarios in Go
Include integration tests for command flows and test CLI end-to-end when possible with test fixturesUse table-driven tests for comprehensive coverage and target >80% coverage
Files:
pkg/provisioner/source/provision_hook_test.gointernal/exec/path_utils_test.go
internal/exec/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
New configs support Go templating with FuncMap() from internal/exec/template_funcs.go
Files:
internal/exec/path_utils_test.go
🧠 Learnings (23)
📓 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: 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: 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.
📚 Learning: 2025-11-24T17:35:37.209Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-11-24T17:35:37.209Z
Learning: Applies to **/*_test.go : Every new feature must include comprehensive unit tests targeting >80% code coverage for all packages
Applied to files:
pkg/provisioner/source/provision_hook_test.gointernal/exec/path_utils_test.go
📚 Learning: 2025-11-24T17:35:37.209Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-11-24T17:35:37.209Z
Learning: Applies to **/*_test.go : Include integration tests for command flows and test CLI end-to-end when possible with test fixtures
Applied to files:
pkg/provisioner/source/provision_hook_test.gointernal/exec/path_utils_test.go
📚 Learning: 2025-12-26T06:31:02.259Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-26T06:31:02.259Z
Learning: Applies to **/*_test.go : Use table-driven tests for comprehensive coverage and target >80% coverage
Applied to files:
pkg/provisioner/source/provision_hook_test.gointernal/exec/path_utils_test.go
📚 Learning: 2025-11-24T17:35:37.209Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-11-24T17:35:37.209Z
Learning: Applies to **/*_test.go : Use table-driven tests for testing multiple scenarios in Go
Applied to files:
pkg/provisioner/source/provision_hook_test.gointernal/exec/path_utils_test.go
📚 Learning: 2025-12-26T06:31:02.259Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-26T06:31:02.259Z
Learning: Applies to **/*.go : Maintain 80% minimum test coverage (CodeCov enforced) - all features need tests
Applied to files:
pkg/provisioner/source/provision_hook_test.go
📚 Learning: 2025-12-10T18:32:51.237Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1808
File: cmd/terraform/backend/backend_delete_test.go:9-23
Timestamp: 2025-12-10T18:32:51.237Z
Learning: In cmd subpackages (e.g., cmd/terraform/backend/), tests cannot use cmd.NewTestKit(t) due to Go's test visibility rules (NewTestKit is in a parent package test file). These tests only need TestKit if they execute commands through RootCmd or modify RootCmd state. Structural tests that only verify command structure/flags without touching RootCmd don't require TestKit cleanup.
Applied to files:
pkg/provisioner/source/provision_hook_test.gointernal/exec/path_utils_test.go
📚 Learning: 2025-11-11T03:47:45.878Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1686
File: toolchain/add_test.go:67-77
Timestamp: 2025-11-11T03:47:45.878Z
Learning: In the cloudposse/atmos codebase, tests should prefer t.Setenv for environment variable setup/teardown instead of os.Setenv/Unsetenv to ensure test-scoped isolation.
Applied to files:
pkg/provisioner/source/provision_hook_test.gointernal/exec/path_utils_test.go
📚 Learning: 2025-05-23T19:51:47.091Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 1255
File: cmd/describe_affected_test.go:15-15
Timestamp: 2025-05-23T19:51:47.091Z
Learning: The atmos codebase has a custom extension to *testing.T that provides a Chdir method, allowing test functions to call t.Chdir() to change working directories during tests. This is used consistently across test files in the codebase.
Applied to files:
pkg/provisioner/source/provision_hook_test.gointernal/exec/path_utils_test.go
📚 Learning: 2025-08-15T14:43:41.030Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 1352
File: pkg/store/artifactory_store_test.go:108-113
Timestamp: 2025-08-15T14:43:41.030Z
Learning: In test files for the atmos project, it's acceptable to ignore errors from os.Setenv/Unsetenv operations during test environment setup and teardown, as these are controlled test scenarios.
Applied to files:
pkg/provisioner/source/provision_hook_test.go
📚 Learning: 2025-11-11T03:47:59.576Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1686
File: toolchain/which_test.go:166-223
Timestamp: 2025-11-11T03:47:59.576Z
Learning: In the cloudposse/atmos repo, tests that manipulate environment variables should use testing.T.Setenv for automatic setup/teardown instead of os.Setenv/Unsetenv.
Applied to files:
pkg/provisioner/source/provision_hook_test.gointernal/exec/path_utils_test.go
📚 Learning: 2024-11-12T03:16:02.910Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 775
File: internal/exec/template_funcs_component.go:157-159
Timestamp: 2024-11-12T03:16:02.910Z
Learning: In the Go code for `componentFunc` in `internal/exec/template_funcs_component.go`, the function `cleanTerraformWorkspace` does not return errors, and it's acceptable if the file does not exist. Therefore, error handling for `cleanTerraformWorkspace` is not needed.
Applied to files:
pkg/provisioner/source/provision_hook_test.gointernal/exec/path_utils_test.go
📚 Learning: 2025-05-23T19:51:47.091Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 1255
File: cmd/describe_affected_test.go:15-15
Timestamp: 2025-05-23T19:51:47.091Z
Learning: In the atmos codebase, t.Chdir() is a valid method call on *testing.T objects and works correctly for changing directories in tests. This is implemented through custom testing framework extensions and is used consistently throughout the test suite.
Applied to files:
pkg/provisioner/source/provision_hook_test.gointernal/exec/path_utils_test.go
📚 Learning: 2024-10-23T21:36:40.262Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 740
File: cmd/cmd_utils.go:340-359
Timestamp: 2024-10-23T21:36:40.262Z
Learning: In the Go codebase for Atmos, when reviewing functions like `checkAtmosConfig` in `cmd/cmd_utils.go`, avoid suggesting refactoring to return errors instead of calling `os.Exit` if such changes would significantly increase the scope due to the need to update multiple call sites.
Applied to files:
pkg/provisioner/source/provision_hook_test.go
📚 Learning: 2025-12-21T04:10:29.030Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1891
File: internal/exec/describe_affected.go:468-468
Timestamp: 2025-12-21T04:10:29.030Z
Learning: In Go, package-level declarations (constants, variables, types, and functions) are visible to all files in the same package without imports. During reviews in cloudposse/atmos (and similar Go codebases), before suggesting to declare a new identifier, first check if it already exists in another file of the same package. If it exists, you can avoid adding a new declaration; if not, proceed with a proper package-level declaration.
Applied to files:
pkg/provisioner/source/provision_hook_test.gointernal/exec/path_utils_test.go
📚 Learning: 2024-12-25T20:28:47.526Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 887
File: internal/exec/stack_processor_utils.go:380-380
Timestamp: 2024-12-25T20:28:47.526Z
Learning: Windows path handling often requires `filepath.Join` to ensure correct separators and comparisons. Insufficient tests can break cross-platform compatibility, so migrating from `path.Join` to `filepath.Join` needs thorough testing on Windows before merging.
Applied to files:
internal/exec/path_utils_test.go
📚 Learning: 2024-10-28T01:51:30.811Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 727
File: internal/exec/terraform_clean.go:329-332
Timestamp: 2024-10-28T01:51:30.811Z
Learning: In the Atmos Go code, when deleting directories or handling file paths (e.g., in `terraform_clean.go`), always resolve the absolute path using `filepath.Abs` and use the logger `u.LogWarning` for logging messages instead of using `fmt.Printf`.
Applied to files:
internal/exec/path_utils_test.go
📚 Learning: 2025-12-13T06:10:25.156Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1686
File: internal/exec/workflow_utils.go:0-0
Timestamp: 2025-12-13T06:10:25.156Z
Learning: Atmos workflows: In internal/exec/workflow_utils.go ExecuteWorkflow, non-identity steps intentionally use baseWorkflowEnv, which is constructed from the parent environment with PATH modifications for the toolchain. Avoid appending os.Environ() again; prefer documenting this behavior and testing that standard environment variables are preserved.
Applied to files:
internal/exec/path_utils_test.go
📚 Learning: 2024-12-25T20:28:19.618Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 887
File: internal/exec/workflow_utils.go:167-169
Timestamp: 2024-12-25T20:28:19.618Z
Learning: The user plans to revert the change from `path.Join` to `filepath.Join` in this PR due to testing gaps and will open a new PR to safely handle the migration without breaking `main`.
Applied to files:
internal/exec/path_utils_test.go
📚 Learning: 2025-05-23T19:51:47.091Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 1255
File: cmd/describe_affected_test.go:15-15
Timestamp: 2025-05-23T19:51:47.091Z
Learning: In the atmos codebase, t.Chdir() is a valid method that can be called on *testing.T objects. This functionality is implemented through custom testing framework extensions and is used consistently throughout the test suite for changing working directories during tests.
Applied to files:
internal/exec/path_utils_test.go
📚 Learning: 2025-05-23T19:51:47.091Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 1255
File: cmd/describe_affected_test.go:15-15
Timestamp: 2025-05-23T19:51:47.091Z
Learning: In the atmos codebase, t.Chdir() is a valid method call on *testing.T objects and works correctly for changing directories in tests.
Applied to files:
internal/exec/path_utils_test.go
📚 Learning: 2025-12-26T06:31:02.259Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-26T06:31:02.259Z
Learning: Applies to **/*.go : Code must be Linux/macOS/Windows compatible - use SDKs over binaries, use filepath.Join() instead of hardcoded separators
Applied to files:
internal/exec/path_utils_test.go
📚 Learning: 2025-11-10T23:23:39.771Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1686
File: toolchain/registry/aqua/aqua_test.go:417-442
Timestamp: 2025-11-10T23:23:39.771Z
Learning: In Atmos toolchain AquaRegistry, tests should not hit real GitHub. Use the options pattern via WithGitHubBaseURL to inject an httptest server URL and make GetLatestVersion/GetAvailableVersions deterministic.
Applied to files:
internal/exec/path_utils_test.go
🧬 Code graph analysis (1)
internal/exec/path_utils_test.go (2)
pkg/schema/schema.go (5)
AtmosConfiguration(54-99)ConfigAndStacksInfo(692-788)Terraform(388-400)Helmfile(414-421)Packer(423-426)pkg/provisioner/workdir/types.go (1)
WorkdirPathKey(73-73)
🪛 GitHub Actions: Tests
internal/exec/path_utils_test.go
[error] 81-81: TestConstructTerraformComponentWorkingDir_WithWorkdirPath: expected '\workdir\terraform\dev-vpc' but got '/workdir/terraform/dev-vpc'.
[error] 105-105: TestConstructHelmfileComponentWorkingDir_WithWorkdirPath: expected '\workdir\helmfile\dev-nginx' but got '/workdir/helmfile/dev-nginx'.
[error] 136-136: TestConstructPackerComponentWorkingDir_WithWorkdirPath: expected '\workdir\packer\dev-ami' but got '/workdir/packer/dev-ami'.
⏰ 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 (8)
pkg/provisioner/source/provision_hook_test.go (6)
1-12: LGTM! Clean imports and organization.The package and imports follow all conventions: proper three-group organization, correct aliasing (
errUtils), and appropriate dependencies for comprehensive unit testing.
14-94: Excellent coverage of component name extraction.This table-driven test thoroughly exercises
extractComponentNamewith comprehensive edge cases including priority rules, nil handling, type mismatches, and missing fields. Well-structured.
96-187: Comprehensive workdir enablement logic testing.This test thoroughly validates the nested map navigation for
provision.workdir.enabledwith excellent coverage of missing sections, type mismatches, and edge cases. Clean and complete.
189-323: Strong directory resolution testing with cross-platform support.The comment provides helpful context about related tests. The test cases comprehensively cover workdir logic, component types, error conditions, and edge cases. Excellent use of
filepath.FromSlashat line 318 for cross-platform test compatibility.
325-402: Thorough extraction logic with proper error handling.This test validates source and component extraction with good coverage of valid cases, missing data, type mismatches, and error conditions. The conditional assertions at lines 393-398 correctly handle the different expectations.
404-435: Error wrapping tested appropriately.This test validates that
wrapProvisionErroralways returns a properly typed error (even with nil input), correctly usingassert.ErrorIsto verify the sentinel error type. The comment at lines 431-432 provides helpful context about ErrorBuilder enrichments.internal/exec/path_utils_test.go (2)
9-9: Import alias looks good.Clean alias for the workdir provisioner package.
202-202: Cross-platform path assertions improved.Wrapping expected paths with
filepath.FromSlash()ensures tests work correctly on Windows and other platforms. This is the right pattern for cross-platform path testing.Also applies to: 218-218, 225-225, 253-253, 272-272, 279-279
Added filepath.FromSlash() to constructTerraformComponentWorkingDir, constructHelmfileComponentWorkingDir, and constructPackerComponentWorkingDir to normalize path separators when returning workdir paths from config. This ensures paths with forward slashes (common in YAML config files) are converted to native path separators on Windows. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
internal/exec/path_utils_test.go (2)
153-185: Consider table-driven format for consistency.The inline test approach works fine, but the coding guidelines prefer table-driven tests for multiple scenarios. Since you have two test cases here and
TestConstructPackerComponentVarfileName(lines 120-151) uses a table-driven structure, converting this test would improve consistency.Optional table-driven refactor
func TestConstructPackerComponentWorkingDir(t *testing.T) { - // Test basic working dir with folder prefix. - atmosConfig1 := schema.AtmosConfiguration{ - BasePath: "base", - Components: schema.Components{ - Packer: schema.Packer{ - BasePath: "packer", + tests := []struct { + name string + atmosConfig schema.AtmosConfiguration + info schema.ConfigAndStacksInfo + want string + }{ + { + name: "with folder prefix", + atmosConfig: schema.AtmosConfiguration{ + BasePath: "base", + Components: schema.Components{ + Packer: schema.Packer{ + BasePath: "packer", + }, + }, + }, + info: schema.ConfigAndStacksInfo{ + ComponentFolderPrefix: "infra", + FinalComponent: "app", + }, + want: filepath.Join("base", "packer", "infra", "app"), + }, + { + name: "empty folder prefix", + atmosConfig: schema.AtmosConfiguration{ + BasePath: "root", + Components: schema.Components{ + Packer: schema.Packer{ + BasePath: "packer-templates", + }, + }, + }, + info: schema.ConfigAndStacksInfo{ + ComponentFolderPrefix: "", + FinalComponent: "base", }, + want: filepath.Join("root", "packer-templates", "base"), }, } - info1 := schema.ConfigAndStacksInfo{ - ComponentFolderPrefix: "infra", - FinalComponent: "app", - } - got1 := constructPackerComponentWorkingDir(&atmosConfig1, &info1) - assert.Equal(t, filepath.Join("base", "packer", "infra", "app"), got1) - - // Test empty folder prefix. - atmosConfig2 := schema.AtmosConfiguration{ - BasePath: "root", - Components: schema.Components{ - Packer: schema.Packer{ - BasePath: "packer-templates", - }, - }, - } - info2 := schema.ConfigAndStacksInfo{ - ComponentFolderPrefix: "", - FinalComponent: "base", - } - got2 := constructPackerComponentWorkingDir(&atmosConfig2, &info2) - assert.Equal(t, filepath.Join("root", "packer-templates", "base"), got2) + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := constructPackerComponentWorkingDir(&tt.atmosConfig, &tt.info) + assert.Equal(t, tt.want, got) + }) + } }As per coding guidelines (table-driven tests for multiple scenarios).
187-224: Same suggestion: table-driven would improve consistency.Like the previous test, this could benefit from a table-driven structure to match the coding guidelines and maintain consistency with
TestConstructPackerComponentVarfileName.Optional table-driven refactor
func TestConstructPackerComponentVarfilePath(t *testing.T) { - // Test complete path construction. - atmosConfig1 := schema.AtmosConfiguration{ - BasePath: "base", - Components: schema.Components{ - Packer: schema.Packer{ - BasePath: "packer", + tests := []struct { + name string + atmosConfig schema.AtmosConfiguration + info schema.ConfigAndStacksInfo + want string + }{ + { + name: "complete path construction", + atmosConfig: schema.AtmosConfiguration{ + BasePath: "base", + Components: schema.Components{ + Packer: schema.Packer{ + BasePath: "packer", + }, + }, + }, + info: schema.ConfigAndStacksInfo{ + ContextPrefix: "dev", + ComponentFolderPrefix: "infra", + Component: "app", + FinalComponent: "app", + }, + want: filepath.Join("base", "packer", "infra", "app", "dev-app.packer.vars.json"), + }, + { + name: "with replaced folder prefix", + atmosConfig: schema.AtmosConfiguration{ + BasePath: "root", + Components: schema.Components{ + Packer: schema.Packer{ + BasePath: "packer-templates", + }, + }, + }, + info: schema.ConfigAndStacksInfo{ + ContextPrefix: "prod", + ComponentFolderPrefix: "platform", + ComponentFolderPrefixReplaced: "plat", + Component: "base", + FinalComponent: "base", }, + want: filepath.Join("root", "packer-templates", "platform", "base", "prod-plat-base.packer.vars.json"), }, } - info1 := schema.ConfigAndStacksInfo{ - ContextPrefix: "dev", - ComponentFolderPrefix: "infra", - Component: "app", - FinalComponent: "app", - } - got1 := constructPackerComponentVarfilePath(&atmosConfig1, &info1) - assert.Equal(t, filepath.Join("base", "packer", "infra", "app", "dev-app.packer.vars.json"), got1) - - // Test path with replaced folder prefix. - atmosConfig2 := schema.AtmosConfiguration{ - BasePath: "root", - Components: schema.Components{ - Packer: schema.Packer{ - BasePath: "packer-templates", - }, - }, - } - info2 := schema.ConfigAndStacksInfo{ - ContextPrefix: "prod", - ComponentFolderPrefix: "platform", - ComponentFolderPrefixReplaced: "plat", - Component: "base", - FinalComponent: "base", - } - got2 := constructPackerComponentVarfilePath(&atmosConfig2, &info2) - assert.Equal(t, filepath.Join("root", "packer-templates", "platform", "base", "prod-plat-base.packer.vars.json"), got2) + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := constructPackerComponentVarfilePath(&tt.atmosConfig, &tt.info) + assert.Equal(t, tt.want, got) + }) + } }As per coding guidelines (table-driven tests for multiple scenarios).
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
internal/exec/path_utils.gointernal/exec/path_utils_test.gopkg/provisioner/source/provision_hook_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/exec/path_utils.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: Context should be first parameter in functions that accept it
Use I/O layer (pkg/io/) for stream access and UI layer (pkg/ui/) for formatting - use data.Write/Writef/Writeln for stdout, ui.Write/Writef/Writeln for stderr - DO NOT use fmt.Fprintf(os.Stdout/Stderr) or fmt.Println
All comments must end with periods (enforced by godot linter)
Use three-group import organization separated by blank lines, sorted alphabetically: Go stdlib, 3rd-party (NOT cloudposse/atmos), Atmos packages - maintain aliases: cfg, log, u, errUtils
Add defer perf.Track(atmosConfig, "pkg.FuncName")() + blank line to all public functions, 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, fmt.Errorf with %w for adding string context, error builder for complex errors, errors.Is() for error checking
Use go.uber.org/...
Files:
pkg/provisioner/source/provision_hook_test.gointernal/exec/path_utils_test.go
**/*_test.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
**/*_test.go: Every new feature must include comprehensive unit tests targeting >80% code coverage for all packages
Use table-driven tests for testing multiple scenarios in Go
Include integration tests for command flows and test CLI end-to-end when possible with test fixturesUse table-driven tests for comprehensive coverage and target >80% coverage
Files:
pkg/provisioner/source/provision_hook_test.gointernal/exec/path_utils_test.go
internal/exec/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
New configs support Go templating with FuncMap() from internal/exec/template_funcs.go
Files:
internal/exec/path_utils_test.go
🧠 Learnings (34)
📓 Common learnings
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: 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: 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: 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: 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.
📚 Learning: 2025-11-24T17:35:37.209Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-11-24T17:35:37.209Z
Learning: Applies to **/*_test.go : Every new feature must include comprehensive unit tests targeting >80% code coverage for all packages
Applied to files:
pkg/provisioner/source/provision_hook_test.gointernal/exec/path_utils_test.go
📚 Learning: 2025-11-24T17:35:37.209Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-11-24T17:35:37.209Z
Learning: Applies to **/*_test.go : Include integration tests for command flows and test CLI end-to-end when possible with test fixtures
Applied to files:
pkg/provisioner/source/provision_hook_test.gointernal/exec/path_utils_test.go
📚 Learning: 2025-12-26T06:31:02.259Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-26T06:31:02.259Z
Learning: Applies to **/*_test.go : Use table-driven tests for comprehensive coverage and target >80% coverage
Applied to files:
pkg/provisioner/source/provision_hook_test.gointernal/exec/path_utils_test.go
📚 Learning: 2025-12-26T06:31:02.259Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-26T06:31:02.259Z
Learning: Applies to **/*.go : Maintain 80% minimum test coverage (CodeCov enforced) - all features need tests
Applied to files:
pkg/provisioner/source/provision_hook_test.go
📚 Learning: 2025-11-11T03:47:45.878Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1686
File: toolchain/add_test.go:67-77
Timestamp: 2025-11-11T03:47:45.878Z
Learning: In the cloudposse/atmos codebase, tests should prefer t.Setenv for environment variable setup/teardown instead of os.Setenv/Unsetenv to ensure test-scoped isolation.
Applied to files:
pkg/provisioner/source/provision_hook_test.gointernal/exec/path_utils_test.go
📚 Learning: 2025-05-23T19:51:47.091Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 1255
File: cmd/describe_affected_test.go:15-15
Timestamp: 2025-05-23T19:51:47.091Z
Learning: The atmos codebase has a custom extension to *testing.T that provides a Chdir method, allowing test functions to call t.Chdir() to change working directories during tests. This is used consistently across test files in the codebase.
Applied to files:
pkg/provisioner/source/provision_hook_test.gointernal/exec/path_utils_test.go
📚 Learning: 2025-08-15T14:43:41.030Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 1352
File: pkg/store/artifactory_store_test.go:108-113
Timestamp: 2025-08-15T14:43:41.030Z
Learning: In test files for the atmos project, it's acceptable to ignore errors from os.Setenv/Unsetenv operations during test environment setup and teardown, as these are controlled test scenarios.
Applied to files:
pkg/provisioner/source/provision_hook_test.go
📚 Learning: 2025-11-11T03:47:59.576Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1686
File: toolchain/which_test.go:166-223
Timestamp: 2025-11-11T03:47:59.576Z
Learning: In the cloudposse/atmos repo, tests that manipulate environment variables should use testing.T.Setenv for automatic setup/teardown instead of os.Setenv/Unsetenv.
Applied to files:
pkg/provisioner/source/provision_hook_test.gointernal/exec/path_utils_test.go
📚 Learning: 2024-11-12T03:16:02.910Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 775
File: internal/exec/template_funcs_component.go:157-159
Timestamp: 2024-11-12T03:16:02.910Z
Learning: In the Go code for `componentFunc` in `internal/exec/template_funcs_component.go`, the function `cleanTerraformWorkspace` does not return errors, and it's acceptable if the file does not exist. Therefore, error handling for `cleanTerraformWorkspace` is not needed.
Applied to files:
pkg/provisioner/source/provision_hook_test.gointernal/exec/path_utils_test.go
📚 Learning: 2025-05-23T19:51:47.091Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 1255
File: cmd/describe_affected_test.go:15-15
Timestamp: 2025-05-23T19:51:47.091Z
Learning: In the atmos codebase, t.Chdir() is a valid method call on *testing.T objects and works correctly for changing directories in tests. This is implemented through custom testing framework extensions and is used consistently throughout the test suite.
Applied to files:
pkg/provisioner/source/provision_hook_test.gointernal/exec/path_utils_test.go
📚 Learning: 2025-12-10T18:32:51.237Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1808
File: cmd/terraform/backend/backend_delete_test.go:9-23
Timestamp: 2025-12-10T18:32:51.237Z
Learning: In cmd subpackages (e.g., cmd/terraform/backend/), tests cannot use cmd.NewTestKit(t) due to Go's test visibility rules (NewTestKit is in a parent package test file). These tests only need TestKit if they execute commands through RootCmd or modify RootCmd state. Structural tests that only verify command structure/flags without touching RootCmd don't require TestKit cleanup.
Applied to files:
pkg/provisioner/source/provision_hook_test.gointernal/exec/path_utils_test.go
📚 Learning: 2024-10-23T21:36:40.262Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 740
File: cmd/cmd_utils.go:340-359
Timestamp: 2024-10-23T21:36:40.262Z
Learning: In the Go codebase for Atmos, when reviewing functions like `checkAtmosConfig` in `cmd/cmd_utils.go`, avoid suggesting refactoring to return errors instead of calling `os.Exit` if such changes would significantly increase the scope due to the need to update multiple call sites.
Applied to files:
pkg/provisioner/source/provision_hook_test.go
📚 Learning: 2025-12-21T04:10:29.030Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1891
File: internal/exec/describe_affected.go:468-468
Timestamp: 2025-12-21T04:10:29.030Z
Learning: In Go, package-level declarations (constants, variables, types, and functions) are visible to all files in the same package without imports. During reviews in cloudposse/atmos (and similar Go codebases), before suggesting to declare a new identifier, first check if it already exists in another file of the same package. If it exists, you can avoid adding a new declaration; if not, proceed with a proper package-level declaration.
Applied to files:
pkg/provisioner/source/provision_hook_test.gointernal/exec/path_utils_test.go
📚 Learning: 2024-12-25T20:28:47.526Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 887
File: internal/exec/stack_processor_utils.go:380-380
Timestamp: 2024-12-25T20:28:47.526Z
Learning: Windows path handling often requires `filepath.Join` to ensure correct separators and comparisons. Insufficient tests can break cross-platform compatibility, so migrating from `path.Join` to `filepath.Join` needs thorough testing on Windows before merging.
Applied to files:
internal/exec/path_utils_test.go
📚 Learning: 2024-10-28T01:51:30.811Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 727
File: internal/exec/terraform_clean.go:329-332
Timestamp: 2024-10-28T01:51:30.811Z
Learning: In the Atmos Go code, when deleting directories or handling file paths (e.g., in `terraform_clean.go`), always resolve the absolute path using `filepath.Abs` and use the logger `u.LogWarning` for logging messages instead of using `fmt.Printf`.
Applied to files:
internal/exec/path_utils_test.go
📚 Learning: 2025-12-13T06:10:25.156Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1686
File: internal/exec/workflow_utils.go:0-0
Timestamp: 2025-12-13T06:10:25.156Z
Learning: Atmos workflows: In internal/exec/workflow_utils.go ExecuteWorkflow, non-identity steps intentionally use baseWorkflowEnv, which is constructed from the parent environment with PATH modifications for the toolchain. Avoid appending os.Environ() again; prefer documenting this behavior and testing that standard environment variables are preserved.
Applied to files:
internal/exec/path_utils_test.go
📚 Learning: 2025-05-23T19:51:47.091Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 1255
File: cmd/describe_affected_test.go:15-15
Timestamp: 2025-05-23T19:51:47.091Z
Learning: In the atmos codebase, t.Chdir() is a valid method that can be called on *testing.T objects. This functionality is implemented through custom testing framework extensions and is used consistently throughout the test suite for changing working directories during tests.
Applied to files:
internal/exec/path_utils_test.go
📚 Learning: 2025-12-26T06:31:02.259Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-26T06:31:02.259Z
Learning: Applies to **/*.go : Code must be Linux/macOS/Windows compatible - use SDKs over binaries, use filepath.Join() instead of hardcoded separators
Applied to files:
internal/exec/path_utils_test.go
📚 Learning: 2024-11-02T15:35:09.958Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 759
File: internal/exec/terraform.go:366-368
Timestamp: 2024-11-02T15:35:09.958Z
Learning: In `internal/exec/terraform.go`, the workspace cleaning code under both the general execution path and within the `case "init":` block is intentionally duplicated because the code execution paths are different. The `.terraform/environment` file should be deleted before executing `terraform init` in both scenarios to ensure a clean state.
Applied to files:
internal/exec/path_utils_test.go
📚 Learning: 2024-12-25T20:28:19.618Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 887
File: internal/exec/workflow_utils.go:167-169
Timestamp: 2024-12-25T20:28:19.618Z
Learning: The user plans to revert the change from `path.Join` to `filepath.Join` in this PR due to testing gaps and will open a new PR to safely handle the migration without breaking `main`.
Applied to files:
internal/exec/path_utils_test.go
📚 Learning: 2025-03-18T12:26:25.329Z
Learnt from: Listener430
Repo: cloudposse/atmos PR: 1149
File: tests/snapshots/TestCLICommands_atmos_vendor_pull_ssh.stderr.golden:7-7
Timestamp: 2025-03-18T12:26:25.329Z
Learning: In the Atmos project, typos or inconsistencies in test snapshot files (such as "terrafrom" instead of "terraform") may be intentional as they capture the exact output of commands and should not be flagged as issues requiring correction.
Applied to files:
internal/exec/path_utils_test.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:
internal/exec/path_utils_test.go
📚 Learning: 2025-11-24T17:35:37.209Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-11-24T17:35:37.209Z
Learning: Applies to **/*_test.go : Use table-driven tests for testing multiple scenarios in Go
Applied to files:
internal/exec/path_utils_test.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:
internal/exec/path_utils_test.go
📚 Learning: 2025-12-17T20:55:47.884Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1874
File: internal/exec/describe_affected_utils_test.go:436-468
Timestamp: 2025-12-17T20:55:47.884Z
Learning: In the Atmos codebase, there are two different paths for the `locked` flag: (1) filtering logic in `internal/exec/component_utils.go` (`isComponentLocked()`) reads from `componentSection["metadata"]["locked"]` to determine which components to include/exclude, and (2) extraction/rendering logic in `pkg/list/extract/affected.go` reads from `settings.metadata.locked` to display the locked status in output. Tests for filtering behavior should use `metadata.locked`.
Applied to files:
internal/exec/path_utils_test.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:
internal/exec/path_utils_test.go
📚 Learning: 2025-04-26T15:54:10.506Z
Learnt from: haitham911
Repo: cloudposse/atmos PR: 1195
File: internal/exec/terraform_clean.go:99-99
Timestamp: 2025-04-26T15:54:10.506Z
Learning: The error variable `ErrRelPath` is defined in `internal/exec/terraform_clean_util.go` and is used across files in the `exec` package, including in `terraform_clean.go`. This is part of an approach to standardize error handling in the codebase.
Applied to files:
internal/exec/path_utils_test.go
📚 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:
internal/exec/path_utils_test.go
📚 Learning: 2024-10-23T20:13:23.054Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 731
File: pkg/utils/file_utils.go:198-202
Timestamp: 2024-10-23T20:13:23.054Z
Learning: In `pkg/utils/file_utils.go`, the current implementation of the `IsURL` function is considered sufficient; avoid suggesting more complex URL validation in future reviews.
Applied to files:
internal/exec/path_utils_test.go
📚 Learning: 2024-10-27T04:41:49.199Z
Learnt from: haitham911
Repo: cloudposse/atmos PR: 727
File: internal/exec/terraform_clean.go:215-223
Timestamp: 2024-10-27T04:41:49.199Z
Learning: In `internal/exec/terraform_clean.go`, the function `determineCleanPath` is necessary and should not be removed.
Applied to files:
internal/exec/path_utils_test.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:
internal/exec/path_utils_test.go
📚 Learning: 2025-05-23T19:51:47.091Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 1255
File: cmd/describe_affected_test.go:15-15
Timestamp: 2025-05-23T19:51:47.091Z
Learning: In the atmos codebase, t.Chdir() is a valid method call on *testing.T objects and works correctly for changing directories in tests.
Applied to files:
internal/exec/path_utils_test.go
📚 Learning: 2025-09-08T01:25:44.958Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1466
File: website/docs/cli/commands/toolchain/usage.mdx:117-121
Timestamp: 2025-09-08T01:25:44.958Z
Learning: Final XDG Base Directory Specification implementation for atmos toolchain is complete and verified: toolchain/xdg_cache.go provides GetXDGCacheDir() and GetXDGTempCacheDir() functions, all hardcoded ~/.cache/tools-cache paths have been replaced with XDG-compliant paths using ${XDG_CACHE_HOME}/atmos-toolchain (or ~/.cache/atmos-toolchain fallback), and tests have been updated to expect the new path structure.
Applied to files:
internal/exec/path_utils_test.go
🧬 Code graph analysis (2)
pkg/provisioner/source/provision_hook_test.go (2)
pkg/schema/schema.go (3)
AtmosConfiguration(54-99)Terraform(388-400)Helmfile(414-421)errors/errors.go (1)
ErrSourceProvision(673-673)
internal/exec/path_utils_test.go (2)
pkg/schema/schema.go (5)
AtmosConfiguration(54-99)Terraform(388-400)ConfigAndStacksInfo(692-788)Helmfile(414-421)Packer(423-426)pkg/provisioner/workdir/types.go (1)
WorkdirPathKey(73-73)
⏰ 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 (10)
pkg/provisioner/source/provision_hook_test.go (7)
3-12: Import organization looks great.Properly organized in three groups (stdlib, 3rd-party, atmos packages) with correct alias for the errors package.
14-94: Comprehensive table-driven test with excellent edge-case coverage.The 10 test cases thoroughly cover all scenarios including type mismatches, nil/empty configs, and field priority.
96-187: Well-structured test for nested configuration parsing.Good coverage of type-safety scenarios where intermediate config levels might not be maps.
189-190: Good cross-reference comment.Helpful note explaining where the related function is tested.
192-323: Excellent cross-platform test with proper path handling.The test correctly uses
filepath.FromSlashon line 318 to normalize path separators for cross-platform compatibility. Good coverage of standard paths, workdir scenarios, and error cases.
325-402: Clean test for source extraction logic.Good balance of happy path and error scenarios, including type validation.
404-435: Solid error-wrapping test with good defensive programming.The test correctly verifies
ErrorIsbehavior with the sentinel error. The nil error test case (line 418-422) is good defensive programming—the wrapper always appliesErrSourceProvisioneven if the underlying error is nil. The comment on lines 431-432 helpfully documents ErrorBuilder behavior.internal/exec/path_utils_test.go (3)
13-54: Solid workdir precedence test coverage.This test thoroughly validates the three key scenarios: workdir path taking precedence, empty string fallback, and missing key fallback. The use of
filepath.Jointhroughout ensures cross-platform compatibility, and the test structure is clear and maintainable.
56-86: Clean Helmfile workdir test.The test validates workdir precedence and standard path fallback using the same solid approach as the Terraform test. Cross-platform path handling is correct.
88-118: Consistent Packer workdir test.The test structure matches the Helmfile approach and correctly validates workdir precedence for Packer components. Cross-platform compatibility is maintained.
|
These changes were released in v1.203.0-rc.4. |
Summary
Implements a comprehensive source provisioner system for just-in-time (JIT) vendoring of component sources. This enables components to declare their source location inline using
sourceconfiguration, supporting dynamic vendoring workflows for Terraform, Helmfile, and Packer components.Closes #598
What Changed
Core Source Provisioner (
pkg/provisioner/source/)CLI Commands
atmos terraform source pull- Vendor component source on demandatmos terraform source describe- Display source configuration detailsatmos terraform source delete- Remove vendored source (requires--force)atmos terraform source list- List components with source configurationJIT Auto-Provisioning
terraform plan/apply/deploy,helmfile sync/diff,packer build--forceflag to override existing sourcesWorkdir Provisioner (
pkg/provisioner/workdir/).workdir/<type>/<stack>-<component>)construct*ComponentWorkingDirfunctionsSchema Updates
source- URI, version, included_paths, excluded_paths, retry configurationprovision.workdir- Enable per-stack working directoriessource_retry- max_retries, delay, max_delay for download resilienceError Handling
ErrSourceProvision,ErrSourceInvalidSpec,ErrSourceCopyFailed,ErrSourceMissing,ErrSourceTargetExists,ErrForceRequired,ErrCreateTempDir,ErrSourceWorkdirConflict,ErrFailedToInitConfig,ErrDescribeComponentTesting
tests/fixtures/scenarios/source-provisioner/andsource-provisioner-workdir/Documentation
Example Usage
Why
vendor.yamlorcomponent.yamlfiles neededReferences
docs/prd/source-provisioner.mdSummary by CodeRabbit
New Features
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.