Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (11)
📝 WalkthroughWalkthroughThis PR adds a reusable dependency graph system (pkg/dependency) and integrates it into Atmos Terraform execution flows. It implements graph construction, filtering, topological ordering, and per-node execution so Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant CLI as atmos CLI
participant Exec as ExecuteTerraformAll / Affected
participant Builder as GraphBuilder
participant Parser as DependencyParser
participant Graph as Graph
participant Sort as TopologicalSort
participant NodeExec as executeTerraformForNode
participant TF as Terraform
User->>CLI: atmos terraform apply --all -s prod
CLI->>Exec: ExecuteTerraformAll(info)
Exec->>Builder: NewBuilder()
Exec->>Exec: addNodesToGraph (register nodes)
Exec->>Parser: NewDependencyParser(builder, nodeMap)
Exec->>Parser: ParseComponentDependencies(...) per component
Parser->>Builder: AddDependency(from→to)
Exec->>Builder: Build()
Builder-->>Graph: Graph (validated)
Exec->>Graph: applyFiltersToGraph (component/stack/query)
Exec->>Sort: TopologicalSort()
Sort-->>Exec: ExecutionOrder [vpc, db, cache, app]
loop per node in order
Exec->>NodeExec: executeTerraformForNode(node, info)
NodeExec->>NodeExec: shouldSkipNode / query check
NodeExec->>NodeExec: updateInfoFromNode
NodeExec->>TF: run "atmos terraform <cmd> <component> -s <stack>"
TF-->>NodeExec: result
end
Exec-->>CLI: completed
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 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 |
|
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. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
golangci-lint found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1516 +/- ##
==========================================
+ Coverage 77.15% 77.16% +0.01%
==========================================
Files 942 951 +9
Lines 89502 90358 +856
==========================================
+ Hits 69051 69726 +675
- Misses 16394 16556 +162
- Partials 4057 4076 +19
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/exec/terraform_utils.go (1)
106-112: Include plan in --all and reverse destroy order
- cmd/terraform_utils.go: in the
if info.All && (…)condition, also match"plan"so--all planroutes throughExecuteTerraformAll.- internal/exec/terraform_all.go: before iterating over
executionOrder, ifinfo.SubCommand == "destroy", reverse the slice so dependents are destroyed before their dependencies.
🧹 Nitpick comments (24)
tests/fixtures/scenarios/terraform-apply-all-dependencies/stacks/deploy/prod.yaml (1)
6-58: “Cross-stack” comment vs data mismatch.The comments say this tests cross-stack deps, but all depends_on targets are within the same stack. Either:
- add a true cross-stack reference (e.g., include stack field in depends_on), or
- adjust the comment to “intra-stack” to avoid confusion.
docs/prd/terraform-dependency-order.md (1)
47-58: Fix markdownlint: add language to fenced code block.Specify a language for the directory tree block to satisfy MD040.
-``` +```text pkg/dependency/ # Reusable dependency graph logic ├── types.go # Core types and interfaces ├── graph.go # Graph structure and operations ├── builder.go # Graph construction ├── sort.go # Topological sort implementation └── filter.go # Graph filtering operations internal/exec/ # Atmos-specific implementation ├── terraform_all.go # --all flag implementation └── terraform_executor.go # Shared execution logic -``` +```internal/exec/terraform_utils_test.go (1)
91-151: Test updated to new graph path looks good.Name, call site, and failure message align with ExecuteTerraformAffectedWithGraph. Consider adding tests for:
- plan --all dependency ordering, and
- destroy --all reverse ordering.
pkg/dependency/sort_test.go (2)
18-24: Don’t ignore AddNode/AddDependency errors.Swallowing errors with
_ = ...weakens tests and can mask failures. Assert/require on these calls.Example pattern:
- _ = graph.AddNode(node1) + assert.NoError(t, graph.AddNode(node1))Or add helpers:
// at file scope func mustAddNode(t *testing.T, g *Graph, n *Node) { t.Helper(); assert.NoError(t, g.AddNode(n)) } func mustAddDep(t *testing.T, g *Graph, from, to string) { t.Helper(); assert.NoError(t, g.AddDependency(from, to)) }Also applies to: 43-52, 73-80, 105-112, 138-144, 164-175, 208-214, 242-248
34-62: Consider table-driven tests to reduce duplication.You can collapse repeated setup/assertions into table cases for readability and easier extension.
Also applies to: 64-96, 98-128, 130-152, 154-198
internal/exec/terraform_all_test.go (3)
56-85: Assert on graph-building errors and prefer require for critical preconditions.If
buildTerraformDependencyGraphfails, subsequent assertions are misleading. Userequire.NoErrorbefore proceeding.Example:
- assert.NoError(t, err) + require.NoError(t, err)Also applies to: 114-130, 158-174
175-203: Don’t ignore AddNode/AddDependency errors in test scaffolding.Assert/require on
AddNode/AddDependencyto keep tests trustworthy.
203-241: Consider table-driven tests for filter scenarios.The three filter subtests can be a single table with inputs/expected IDs.
pkg/dependency/errors.go (1)
5-33: Sentinel errors look good; consider centralizing per repo standard.Repo guidance prefers static errors in errors/errors.go. Either move these or re-export them there to keep a single source of truth.
pkg/dependency/builder.go (1)
60-62: Builder immutability is partial; Graph can still be mutated post-Build.If you need a truly frozen result, either:
- return a cloned read-only view, or
- have Graph carry a sealed flag that AddNode/AddDependency honor.
pkg/dependency/graph_test.go (2)
35-50: Prefer sentinel errors over string matching in assertions.Assert using errors.Is against package errors (e.g., dependency error vars) instead of inspecting error strings. This makes tests robust to message changes.
72-95: Use sentinel errors and table-driven cases for dependency validation.
- Switch to errors.Is for: self‑dependency, missing nodes, and empty IDs.
- Convert the duplicate/self/missing/empty ID checks into a table-driven test for clarity and coverage.
pkg/dependency/sort.go (4)
3-5: Add deterministic ordering to topological sort (import sort).To make execution order reproducible across runs, import sort and sort the ready queue.
Apply this diff:
import ( - "fmt" + "fmt" + "sort" )
19-26: Seed the queue deterministically.Sort the initial zero‑in‑degree queue to avoid map-iteration randomness.
Apply this diff:
// Initialize queue with nodes that have no dependencies queue := []string{} for id, degree := range inDegree { if degree == 0 { queue = append(queue, id) } } + // Ensure deterministic processing order among nodes with equal priority. + sort.Strings(queue)
31-35: Keep queue sorted before each pop for stable ordering.Sorting before each dequeue guarantees deterministic order when multiple nodes become ready in the same step.
Apply this diff:
- for len(queue) > 0 { + for len(queue) > 0 { + // Maintain deterministic order among nodes at the same level. + sort.Strings(queue) // Dequeue the first node currentID := queue[0] queue = queue[1:]
91-95: Remove unused variable ‘processed’.The processed map is written but never read.
Apply this diff:
- levels := [][]Node{} - processed := make(map[string]bool) + levels := [][]Node{} nodeLevel := make(map[string]int) @@ - levels[level] = append(levels[level], *node) - processed[id] = true + levels[level] = append(levels[level], *node)Also applies to: 130-135
internal/exec/terraform_executor.go (1)
85-99: Avoid re-initializing CLI config per node for query evaluation.InitCliConfig inside evaluateNodeQueryFilter runs for every node and can be expensive. Initialize once (in the caller) and pass AtmosConfiguration down to reuse.
Example approach (outline):
- ExecuteTerraformAll obtains atmosConfig.
- Change evaluateNodeQueryFilter(atmosConfig *schema.AtmosConfiguration, node …) and have shouldSkipByQuery pass it through.
- Use the same atmosConfig in EvaluateYqExpression.
internal/exec/terraform_all.go (2)
226-253: Avoid duplicating query evaluation logic.evaluateNodeQuery here and evaluateNodeQueryFilter in terraform_executor.go diverge (empty config vs initialized config, and error handling). Extract a shared helper to keep behavior consistent.
61-74: Optional: log when filters yield zero nodes and exit early.Helps users understand why nothing ran.
Example:
- After determining executionOrder, if len == 0, log.Info("No components matched filters") and return nil.
internal/exec/dependency_parser.go (4)
148-161: Avoid double-logging; let callers decide.addDependencyIfExists logs and callers log again, creating noise. Return errors only; log at one level.
Apply this diff:
func (p *DependencyParser) addDependencyIfExists(fromID, toID string) error { if _, exists := p.nodeMap[toID]; !exists { - log.Warn("Dependency target not found", logFieldFrom, fromID, logFieldTo, toID) return fmt.Errorf("%w: %s", errUtils.ErrDependencyTargetNotFound, toID) } if err := p.builder.AddDependency(fromID, toID); err != nil { - log.Warn("Failed to add dependency", logFieldFrom, fromID, logFieldTo, toID, logFieldError, err) return err } return nil }
46-52: Skip early if source node isn’t in the graph.Pre-check avoids builder errors and needless logging when fromID isn’t part of nodeMap.
Apply this diff:
fromID := fmt.Sprintf("%s-%s", componentName, stackName) // Check for dependencies in settings.depends_on. +if _, exists := p.nodeMap[fromID]; !exists { + log.Debug("Source component not in graph, skipping dependencies.", logFieldFrom, fromID) + return nil +}
21-25: Use a set type for nodeMap.nodeMap values aren’t used; consider map[string]struct{} to signal intent and save memory.
54-58: Prefer a config constant for "depends_on".If there’s a cfg constant, use it for consistency with SettingsSectionName/MetadataSectionName.
pkg/dependency/graph.go (1)
152-181: Clone should deep-copy Metadata to avoid aliasing.Shallow-copying maps can leak mutations across graphs.
Apply this diff:
- newNode := &Node{ + newNode := &Node{ ID: node.ID, Component: node.Component, Stack: node.Stack, Type: node.Type, Dependencies: make([]string, len(node.Dependencies)), Dependents: make([]string, len(node.Dependents)), - Metadata: node.Metadata, // Note: This is a shallow copy of the map Processed: node.Processed, } // Copy slices copy(newNode.Dependencies, node.Dependencies) copy(newNode.Dependents, node.Dependents) + + // Deep-copy metadata map if present. + if node.Metadata != nil { + newNode.Metadata = make(map[string]any, len(node.Metadata)) + for k, v := range node.Metadata { + newNode.Metadata[k] = v + } + }
📜 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 (21)
cmd/terraform_utils.go(2 hunks)docs/prd/terraform-dependency-order.md(1 hunks)errors/errors.go(1 hunks)internal/exec/dependency_parser.go(1 hunks)internal/exec/terraform_affected_graph.go(1 hunks)internal/exec/terraform_all.go(1 hunks)internal/exec/terraform_all_test.go(1 hunks)internal/exec/terraform_executor.go(1 hunks)internal/exec/terraform_utils.go(1 hunks)internal/exec/terraform_utils_test.go(2 hunks)pkg/dependency/builder.go(1 hunks)pkg/dependency/errors.go(1 hunks)pkg/dependency/filter.go(1 hunks)pkg/dependency/graph.go(1 hunks)pkg/dependency/graph_test.go(1 hunks)pkg/dependency/sort.go(1 hunks)pkg/dependency/sort_test.go(1 hunks)pkg/dependency/types.go(1 hunks)tests/fixtures/scenarios/terraform-apply-all-dependencies/atmos.yaml(1 hunks)tests/fixtures/scenarios/terraform-apply-all-dependencies/stacks/deploy/dev.yaml(1 hunks)tests/fixtures/scenarios/terraform-apply-all-dependencies/stacks/deploy/prod.yaml(1 hunks)
🧰 Additional context used
📓 Path-based instructions (7)
cmd/**/*.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
cmd/**/*.go: Use Cobra's recommended command structure with a root command and subcommands
Implement each CLI command in a separate file under cmd/
Use Viper for managing configuration, environment variables, and flags in the CLI
Keep separation of concerns between CLI interface (cmd) and business logic
Use kebab-case for command-line flags
Provide comprehensive help text for all commands and flags
Include examples in Cobra command help
Use Viper for configuration management; support files, env vars, and flags with precedence flags > env > config > defaults
Follow single responsibility; separate command interface from business logic
Provide meaningful user feedback and include progress indicators for long-running operations
Provide clear error messages to users and troubleshooting hints where appropriate
cmd/**/*.go: Use utils.PrintfMarkdown to render embedded markdown content for CLI help/examples.
One Cobra command per file in cmd/ to keep files focused and maintainable.
Send UI prompts/status/progress to stderr and data/results to stdout; never use logging for UI.
Prefer utils.PrintfMessageToTUI for UI messages; only write directly to os.Stderr as a last resort.
For non-standard execution paths, capture telemetry with telemetry.CaptureCmd or telemetry.CaptureCmdString.
Files:
cmd/terraform_utils.go
**/*.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
**/*.go: All code must pass golangci-lint checks
Follow Go error handling idioms and use meaningful error messages
Wrap errors with context using fmt.Errorf("context: %w", err)
Consider custom error types for domain-specific errors
Follow standard Go coding style; run gofmt and goimports
Use snake_case for environment variables
Document complex logic with inline comments
**/*.go: All Go comments must end with periods (enforced by golangci-lint's godot).
Wrap all errors with static errors defined in errors/errors.go; never return dynamic errors directly; use fmt.Errorf with %w and add details after the static error.
Always bind environment variables using viper.BindEnv and provide an ATMOS_ alternative for each external env var.
Use structured logging for system/debug events; logging must not affect execution and should use appropriate levels per docs/logging.md.
Prefer SDKs over shelling out to binaries for cross-platform compatibility; use filepath/os/runtime for portable paths and OS-specific logic.
Files:
cmd/terraform_utils.gopkg/dependency/errors.gointernal/exec/terraform_executor.gointernal/exec/terraform_all.goerrors/errors.gopkg/dependency/sort.gopkg/dependency/types.gointernal/exec/terraform_utils.gopkg/dependency/filter.gopkg/dependency/graph.gointernal/exec/terraform_utils_test.gopkg/dependency/builder.gointernal/exec/terraform_all_test.gopkg/dependency/graph_test.gointernal/exec/terraform_affected_graph.gopkg/dependency/sort_test.gointernal/exec/dependency_parser.go
**/!(*_test).go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
Document all exported functions, types, and methods with Go doc comments
Files:
cmd/terraform_utils.gopkg/dependency/errors.gointernal/exec/terraform_executor.gointernal/exec/terraform_all.goerrors/errors.gopkg/dependency/sort.gopkg/dependency/types.gointernal/exec/terraform_utils.gopkg/dependency/filter.gopkg/dependency/graph.gopkg/dependency/builder.gointernal/exec/terraform_affected_graph.gointernal/exec/dependency_parser.go
pkg/**/*.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
Place business logic in pkg rather than in cmd
Files:
pkg/dependency/errors.gopkg/dependency/sort.gopkg/dependency/types.gopkg/dependency/filter.gopkg/dependency/graph.gopkg/dependency/builder.gopkg/dependency/graph_test.gopkg/dependency/sort_test.go
errors/errors.go
📄 CodeRabbit inference engine (CLAUDE.md)
Define all static errors centrally in errors/errors.go (e.g., ErrInvalidComponent, ErrInvalidStack, ErrInvalidConfig).
Files:
errors/errors.go
**/*_test.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
**/*_test.go: Every new feature must include comprehensive unit tests
Test both happy paths and error conditions
Use table-driven tests for multiple scenarios
**/*_test.go: Use table-driven tests for unit tests and focus on pure functions where possible.
Always use t.Skipf with a clear reason when skipping tests; never use t.Skip.
For CLI tests requiring rebuilt binaries, set a package-level skipReason in TestMain and call os.Exit(m.Run()); individual tests must check and t.Skipf if set; never use log.Fatal for missing/stale binaries.
Mirror test file names to their implementation files (e.g., aws_ssm_store_test.go for aws_ssm_store.go) and co-locate tests with implementation.
Files:
internal/exec/terraform_utils_test.gointernal/exec/terraform_all_test.gopkg/dependency/graph_test.gopkg/dependency/sort_test.go
pkg/**/*_test.go
📄 CodeRabbit inference engine (CLAUDE.md)
Place unit tests under pkg/** matching the package they test.
Files:
pkg/dependency/graph_test.gopkg/dependency/sort_test.go
🧠 Learnings (12)
📓 Common learnings
Learnt from: aknysh
PR: cloudposse/atmos#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.
📚 Learning: 2025-06-23T02:14:30.937Z
Learnt from: aknysh
PR: cloudposse/atmos#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_utils.gointernal/exec/terraform_utils.gointernal/exec/terraform_affected_graph.go
📚 Learning: 2025-09-23T02:30:42.362Z
Learnt from: CR
PR: cloudposse/atmos#0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-09-23T02:30:42.362Z
Learning: Applies to go.{mod,sum} : Manage dependencies with Go modules
Applied to files:
pkg/dependency/graph.go
📚 Learning: 2025-09-23T02:30:42.362Z
Learnt from: CR
PR: cloudposse/atmos#0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-09-23T02:30:42.362Z
Learning: Applies to **/*_test.go : Every new feature must include comprehensive unit tests
Applied to files:
internal/exec/terraform_all_test.gopkg/dependency/graph_test.go
📚 Learning: 2024-10-27T04:41:49.199Z
Learnt from: haitham911
PR: cloudposse/atmos#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/terraform_affected_graph.go
📚 Learning: 2025-04-26T15:54:10.506Z
Learnt from: haitham911
PR: cloudposse/atmos#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/terraform_affected_graph.go
📚 Learning: 2025-09-25T03:30:16.196Z
Learnt from: CR
PR: cloudposse/atmos#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-25T03:30:16.196Z
Learning: Applies to tests/**/*_test.go : Place integration tests under tests/** with *_test.go naming and use fixtures in tests/test-cases/.
Applied to files:
pkg/dependency/sort_test.go
📚 Learning: 2025-03-18T12:26:25.329Z
Learnt from: Listener430
PR: cloudposse/atmos#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:
tests/fixtures/scenarios/terraform-apply-all-dependencies/atmos.yaml
📚 Learning: 2025-09-25T03:30:16.196Z
Learnt from: CR
PR: cloudposse/atmos#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-25T03:30:16.196Z
Learning: Applies to pkg/datafetcher/schema/{config/global/1.0.json,atmos/manifest/1.0.json,stacks/stack-config/1.0.json,vendor/package/1.0.json} : When adding Atmos configuration options, update all schema files: config/global/1.0.json, atmos/manifest/1.0.json, stacks/stack-config/1.0.json, vendor/package/1.0.json under pkg/datafetcher/schema/.
Applied to files:
tests/fixtures/scenarios/terraform-apply-all-dependencies/atmos.yaml
📚 Learning: 2024-11-25T17:17:15.703Z
Learnt from: RoseSecurity
PR: cloudposse/atmos#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/terraform-apply-all-dependencies/atmos.yaml
📚 Learning: 2024-12-01T00:33:20.298Z
Learnt from: aknysh
PR: cloudposse/atmos#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/terraform-apply-all-dependencies/atmos.yaml
📚 Learning: 2025-09-10T21:17:55.273Z
Learnt from: samtholiya
PR: cloudposse/atmos#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:
tests/fixtures/scenarios/terraform-apply-all-dependencies/atmos.yaml
🧬 Code graph analysis (13)
cmd/terraform_utils.go (2)
internal/exec/terraform_affected_graph.go (1)
ExecuteTerraformAffectedWithGraph(15-40)internal/exec/terraform_all.go (1)
ExecuteTerraformAll(15-75)
internal/exec/terraform_executor.go (6)
pkg/dependency/types.go (1)
Node(4-28)pkg/schema/schema.go (1)
ConfigAndStacksInfo(455-532)pkg/config/const.go (1)
MetadataSectionName(66-66)pkg/config/config.go (1)
InitCliConfig(25-62)pkg/utils/yq_utils.go (1)
EvaluateYqExpression(48-98)internal/exec/terraform.go (1)
ExecuteTerraform(64-628)
internal/exec/terraform_all.go (7)
pkg/schema/schema.go (3)
ConfigAndStacksInfo(455-532)Components(353-357)AtmosConfiguration(26-61)pkg/config/config.go (1)
InitCliConfig(25-62)pkg/config/const.go (2)
TerraformComponentType(48-48)MetadataSectionName(66-66)pkg/dependency/types.go (3)
Graph(31-37)Node(4-28)Filter(55-64)pkg/dependency/builder.go (2)
NewBuilder(15-20)GraphBuilder(8-12)internal/exec/dependency_parser.go (1)
NewDependencyParser(28-33)pkg/utils/yq_utils.go (1)
EvaluateYqExpression(48-98)
pkg/dependency/sort.go (2)
pkg/dependency/types.go (3)
Graph(31-37)ExecutionOrder(52-52)Node(4-28)pkg/dependency/errors.go (1)
ErrCircularDependency(29-29)
pkg/dependency/filter.go (2)
pkg/dependency/types.go (3)
Graph(31-37)Filter(55-64)Node(4-28)pkg/dependency/graph.go (1)
NewGraph(13-18)
pkg/dependency/graph.go (2)
pkg/dependency/types.go (2)
Graph(31-37)Node(4-28)pkg/dependency/errors.go (6)
ErrNilNode(8-8)ErrEmptyNodeID(11-11)ErrNodeExists(14-14)ErrEmptyDependencyID(17-17)ErrSelfDependency(20-20)ErrNodeNotFound(23-23)
internal/exec/terraform_utils_test.go (1)
internal/exec/terraform_affected_graph.go (1)
ExecuteTerraformAffectedWithGraph(15-40)
pkg/dependency/builder.go (3)
pkg/dependency/types.go (2)
Graph(31-37)Node(4-28)pkg/dependency/graph.go (1)
NewGraph(13-18)pkg/dependency/errors.go (3)
ErrGraphAlreadyBuilt(26-26)ErrCircularDependency(29-29)ErrNoRootNodes(32-32)
internal/exec/terraform_all_test.go (4)
pkg/schema/schema.go (3)
AtmosConfiguration(26-61)ConfigAndStacksInfo(455-532)Components(353-357)pkg/dependency/graph.go (1)
NewGraph(13-18)pkg/dependency/types.go (1)
Node(4-28)pkg/config/const.go (1)
TerraformComponentType(48-48)
pkg/dependency/graph_test.go (3)
pkg/dependency/graph.go (1)
NewGraph(13-18)pkg/dependency/types.go (1)
Node(4-28)cmd/cmd_utils.go (1)
Contains(750-757)
internal/exec/terraform_affected_graph.go (6)
internal/exec/describe_affected.go (1)
DescribeAffectedCmdArgs(27-48)pkg/schema/schema.go (2)
ConfigAndStacksInfo(455-532)Affected(606-625)internal/exec/describe_affected_helpers.go (3)
ExecuteDescribeAffectedWithTargetRepoPath(346-404)ExecuteDescribeAffectedWithTargetRefClone(29-180)ExecuteDescribeAffectedWithTargetRefCheckout(184-342)pkg/utils/yaml_utils.go (1)
ConvertToYAML(152-166)pkg/dependency/types.go (3)
Graph(31-37)Filter(55-64)Node(4-28)pkg/config/const.go (1)
TerraformComponentType(48-48)
pkg/dependency/sort_test.go (2)
pkg/dependency/graph.go (1)
NewGraph(13-18)pkg/dependency/types.go (1)
Node(4-28)
internal/exec/dependency_parser.go (3)
pkg/dependency/builder.go (1)
GraphBuilder(8-12)pkg/config/const.go (2)
SettingsSectionName(60-60)MetadataSectionName(66-66)errors/errors.go (3)
ErrUnsupportedDependencyType(133-133)ErrMissingDependencyField(134-134)ErrDependencyTargetNotFound(135-135)
🪛 markdownlint-cli2 (0.18.1)
docs/prd/terraform-dependency-order.md
47-47: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Analyze (go)
- GitHub Check: Lint (golangci)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Build (macos-latest, macos)
- GitHub Check: Build (ubuntu-latest, linux)
- GitHub Check: Run pre-commit hooks
- GitHub Check: Summary
🔇 Additional comments (20)
tests/fixtures/scenarios/terraform-apply-all-dependencies/atmos.yaml (1)
1-19: Fixture looks correct for scenario discovery.Paths, filters, and debug logging are appropriate for tests.
tests/fixtures/scenarios/terraform-apply-all-dependencies/stacks/deploy/dev.yaml (1)
6-60: Good dependency chain for ordered execution.The depends_on structure cleanly models vpc -> database/cache -> application.
errors/errors.go (1)
132-136: New static dependency errors are aligned with project conventions.Names/messages are clear and grouped under a labeled section. Ensure all call sites wrap with context using fmt.Errorf("context: %w", Err...).
internal/exec/terraform_utils.go (2)
181-181: Deprecation note placement is correct.“Deprecated:” is in the doc comment group for the exported function. No action needed.
95-97: Routing to graph-based affected path LGTM.Switching to ExecuteTerraformAffectedWithGraph aligns with the new shared graph logic.
pkg/dependency/sort_test.go (1)
200-233: Clarify reflexive reachability/path semantics.Tests assume zero-length paths are “reachable” (node → itself) and FindPath returns a single-element path. Confirm this matches intended API semantics across the package.
Also applies to: 235-254
internal/exec/terraform_affected_graph.go (2)
185-196: Confirm TopologicalSort return type to avoid pointer mismatch.Code takes
&executionOrder[i]. This requires[]dependency.Node. IfTopologicalSortreturns[]*dependency.Node, this becomes**Nodeand won’t compile. Adjust tonode := executionOrder[i]in that case.
149-158: Filter includes dependencies by default; double-check dependents behavior.You always include dependencies, and dependents are guarded by
args.IncludeDependents. Confirm this aligns with CLI semantics for--affected(esp. when both are requested).pkg/dependency/builder.go (1)
41-62: Good validation: cycle detection + root check before build.Kahn’s cycle check with contextual wrapping and root validation is solid.
pkg/dependency/types.go (1)
3-37: Types and docs look solid.Well-structured types with proper GoDoc and field comments.
internal/exec/terraform_executor.go (1)
64-83: Query-eval policy: error currently causes skip. Confirm intended behavior.If YQ evaluation errors, should we fail fast instead of silently skipping? Consider warning or surfacing an error to avoid surprising no-ops.
internal/exec/terraform_all.go (1)
148-162: Helpers exist; no action needed
isComponentEnabled is defined in internal/exec/component_utils.go and walkTerraformComponents in internal/exec/terraform_utils.go.internal/exec/dependency_parser.go (1)
170-178: Confirm enabled flag location.You read metadata.enabled. In some stacks, enabled may be top-level. Confirm we don’t miss disabled components when enabled is outside metadata.
pkg/dependency/graph.go (2)
46-79: AddDependency semantics look solid.Validations, idempotency, and bidirectional edge updates are correct.
81-91: Root identification matches dependency semantics.Roots as nodes with zero Dependencies is correct for Kahn’s algorithm.
pkg/dependency/filter.go (4)
3-10: Filter() flow looks good.Clone+filter nodes and IdentifyRoots on the result is clean.
106-120: Confirm intent: include dependents for FilterByComponent.Including dependents expands the selection “downstream.” Verify this matches CLI expectations for component filtering.
122-150: Dependency/dependent marking is correct and cycle-safe.Visited set via toInclude prevents infinite recursion.
191-205: RemoveNode updates both sides and re-identifies roots.Semantics are correct; idempotent on missing nodes.
cmd/terraform_utils.go (1)
95-97: Good switch to graph-based affected executionNo remaining call sites for
ExecuteTerraformAffectedwere found after switching toExecuteTerraformAffectedWithGraph.
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/exec/terraform_utils_test.go (1)
117-121: Handle pipe errors and close both ends; restore Stderr via defer.Current code ignores os.Pipe errors, leaks the reader, and restores Stderr out-of-band. Fix improves reliability and lint compliance.
- oldStd := os.Stderr - _, w, _ := os.Pipe() - os.Stderr = w + oldStd := os.Stderr + r, w, err := os.Pipe() + if err != nil { + t.Fatalf("Failed to create pipe: %v", err) + } + os.Stderr = w + defer func() { + os.Stderr = oldStd + _ = r.Close() + _ = w.Close() + }() @@ - w.Close() - os.Stderr = oldStd + // Restored and closed in deferred cleanup above. @@ - oldStd := os.Stderr - _, w, _ := os.Pipe() - os.Stderr = w + oldStd := os.Stderr + r, w, err := os.Pipe() + if err != nil { + t.Fatalf("Failed to create pipe: %v", err) + } + os.Stderr = w + defer func() { + os.Stderr = oldStd + _ = r.Close() + _ = w.Close() + }() @@ - w.Close() - os.Stderr = oldStd + // Restored and closed in deferred cleanup above.Also applies to: 148-150, 175-178, 194-196
🧹 Nitpick comments (41)
tests/fixtures/scenarios/terraform-apply-all-dependencies/stacks/deploy/dev.yaml (1)
9-9: Clarify that database/cache order is not strict.Topological sort may schedule database and cache in any order after vpc. Consider updating the comment to avoid implying a strict ordering between them.
- # Expected execution order: vpc -> database -> cache -> application + # Expected dependency order: vpc -> (database, cache in any order) -> applicationpkg/dependency/builder.go (1)
41-62: Consider returning a clone to discourage post-build mutation.Build returns the live graph pointer; callers can still mutate it freely. Returning a defensive clone would better reflect the “built/frozen” intent.
internal/exec/terraform_all_test.go (6)
6-11: Use require for fatal setup errors.Switch to require for setup operations; add import.
"testing" - "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require"
198-202: Assert on AddNode errors instead of discarding them.Discarding errors can mask failures and make tests flaky.
- _ = graph.AddNode(node1) - _ = graph.AddNode(node2) - _ = graph.AddNode(node3) + require.NoError(t, graph.AddNode(node1)) + require.NoError(t, graph.AddNode(node2)) + require.NoError(t, graph.AddNode(node3))
201-203: Assert on AddDependency errors.- _ = graph.AddDependency("database-dev", "vpc-dev") + require.NoError(t, graph.AddDependency("database-dev", "vpc-dev"))
353-354: Use t.Skipf with a clear reason.Guideline: never use t.Skip.
- t.Skip("collectFilteredNodeIDs is not exported") + t.Skipf("collectFilteredNodeIDs is not exported")
416-417: Use t.Skipf with a clear reason.- t.Skip("getComponentName is not exported") + t.Skipf("getComponentName is not exported")
462-463: Use t.Skipf with a clear reason.- t.Skip("validateExecuteTerraformAllArgs is not exported") + t.Skipf("validateExecuteTerraformAllArgs is not exported")cmd/terraform_utils.go (1)
106-112: Ensure destroy runs in reverse topological order.For multi-component destroys, dependents should be destroyed before their dependencies. Confirm ExecuteTerraformAll (or downstream execution) reverses the order for destroy.
Also, end comments with periods to satisfy godot.
- // For commands that need dependency order with --all flag (apply, destroy, deploy) + // For commands that need dependency order with --all flag (apply, destroy, deploy). if info.All && (info.SubCommand == "apply" || info.SubCommand == "destroy" || info.SubCommand == "deploy") { err = e.ExecuteTerraformAll(&info) } else { - // For other commands or when not using --all, use existing query logic + // For other commands or when not using --all, use existing query logic. err = e.ExecuteTerraformQuery(&info) }pkg/dependency/filter_test.go (3)
6-10: Add require to assert fatal setup errors."testing" - "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require"
24-32: Don’t ignore errors when building the test graph.Fail fast if AddNode/AddDependency fails.
- for _, node := range nodes { - _ = graph.AddNode(node) - } + for _, node := range nodes { + require.NoError(t, graph.AddNode(node)) + } - // Add dependencies - _ = graph.AddDependency("database-dev", "vpc-dev") - _ = graph.AddDependency("app-dev", "database-dev") - _ = graph.AddDependency("database-prod", "vpc-prod") + // Add dependencies + require.NoError(t, graph.AddDependency("database-dev", "vpc-dev")) + require.NoError(t, graph.AddDependency("app-dev", "database-dev")) + require.NoError(t, graph.AddDependency("database-prod", "vpc-prod"))Apply the same pattern in other tests in this file where errors are discarded.
146-151: Prefer exact expectations over “greater than 0” where feasible.Asserting exact sizes increases test precision.
docs/prd/terraform-dependency-order.md (3)
47-58: Specify fenced code block language (markdownlint MD040).Label the code fence for the directory tree as text.
-``` +```text pkg/dependency/ # Reusable dependency graph logic ├── types.go # Core types and interfaces ├── graph.go # Graph structure and operations ├── builder.go # Graph construction ├── sort.go # Topological sort implementation └── filter.go # Graph filtering operations internal/exec/ # Atmos-specific implementation ├── terraform_all.go # --all flag implementation └── terraform_executor.go # Shared execution logic--- `135-151`: **Label Go code fences.** Add language to aid syntax highlighting and linters. ```diff -```go +```go // Core node structure type Node struct { ID string // Unique identifier (component-stack) Component string // Component name Stack string // Stack name Dependencies []string // Node IDs this depends on Dependents []string // Node IDs that depend on this Metadata map[string]any } // Dependency graph type Graph struct { Nodes map[string]*Node Roots []string // Nodes with no dependencies }--- `175-187`: **Label API usage snippet as Go.** ```diff -```go +```go // Build dependency graph graph, err := dependency.BuildGraph(components) // Get execution order order, err := graph.TopologicalSort() // Filter for affected components filtered := graph.Filter(affectedIDs, includeDependencies) // Detect cycles hasCycle, cyclePath := graph.DetectCycles()</blockquote></details> <details> <summary>internal/exec/dependency_parser_test.go (2)</summary><blockquote> `237-269`: **Optional: Strengthen assertions on created edges.** After Build(), when the from node exists, assert exact dependency sets to catch duplicates/missing edges. Using a sorted compare improves determinism. --- `271-310`: **Use sentinel errors and assert.ErrorIs in tests** Replace brittle substring assertions with require.Error and assert.ErrorIs against the canonical errors. - internal/exec/dependency_parser_test.go: add `github.com/stretchr/testify/require` and `errUtils "github.com/cloudposse/atmos/errors"` to imports. - In TestDependencyParser_ParseSingleDependency, swap ```go assert.Error(t, err) assert.Contains(t, err.Error(), "unsupported dependency type")with
require.Error(t, err) assert.ErrorIs(t, err, errUtils.ErrUnsupportedDependencyType)
- In TestDependencyParser_ParseDependencyMapEntry, replace
withassert.Error(t, err) assert.Contains(t, err.Error(), tt.errorMsg)require.Error(t, err) switch tt.name { case "missing component field", "component not a string": assert.ErrorIs(t, err, errUtils.ErrMissingDependencyField) case "target not found": assert.ErrorIs(t, err, errUtils.ErrDependencyTargetNotFound) default: t.Fatalf("unexpected error case: %v", err) }pkg/dependency/types.go (1)
51-52: Consider using pointers in ExecutionOrder to avoid unnecessary copies.ExecutionOrder []Node copies Node structs (including slices/maps) on every append/sort operation. Prefer []*Node to reduce allocations and copying.
internal/exec/terraform_executor.go (1)
86-99: Avoid per-node InitCliConfig for query evaluation.InitCliConfig is heavy; doing it per node will hurt --all performance. Initialize once and pass atmosConfig down.
Example approach:
- Compute atmosConfig before iterating nodes (in the caller), pass it into evaluateNodeQueryFilter.
- Change signature to evaluateNodeQueryFilter(node, atmosConfig, query string).
I can draft the refactor if desired.
pkg/dependency/sort.go (2)
19-26: Make topological order deterministic.Map iteration order is random; queue growth is nondeterministic. Sort the queue (and newly enqueued nodes) to stabilize results and tests.
Apply this diff:
@@ - // Initialize queue with nodes that have no dependencies + // Initialize queue with nodes that have no dependencies. queue := []string{} for id, degree := range inDegree { if degree == 0 { queue = append(queue, id) } } + sort.Strings(queue) @@ - for _, dependentID := range currentNode.Dependents { + for _, dependentID := range currentNode.Dependents { inDegree[dependentID]-- if inDegree[dependentID] == 0 { queue = append(queue, dependentID) } } + // Keep processing stable. + sort.Strings(queue)Also add the import:
import ( "fmt" + "sort" )Also applies to: 41-47
91-93: Remove the unused 'processed' map in GetExecutionLevels.It's written but never read; simplifies the function.
Apply this diff:
- processed := make(map[string]bool) @@ - levels[level] = append(levels[level], *node) - processed[id] = true + levels[level] = append(levels[level], *node)Also applies to: 134-135
pkg/dependency/graph.go (1)
157-174: Avoid aliasing Metadata between clones.Clone currently reuses the same Metadata map; mutations in one graph affect the other. Copy the top-level map.
Apply this diff:
for id, node := range g.Nodes { - newNode := &Node{ + // Shallow-copy metadata map to avoid aliasing. + var meta map[string]any + if node.Metadata != nil { + meta = make(map[string]any, len(node.Metadata)) + for k, v := range node.Metadata { + meta[k] = v + } + } + newNode := &Node{ ID: node.ID, Component: node.Component, Stack: node.Stack, Type: node.Type, Dependencies: make([]string, len(node.Dependencies)), Dependents: make([]string, len(node.Dependents)), - Metadata: node.Metadata, // Note: This is a shallow copy of the map + Metadata: meta, Processed: node.Processed, }Note: Deep-copying nested maps/slices is out of scope here; this change prevents top-level aliasing.
internal/exec/dependency_parser.go (5)
3-11: Aggregate parse errors instead of only logging them.Each parser loop logs and continues, but the caller always gets nil. At minimum, return an aggregated error (still after parsing all items) so upstream can decide how to handle partial failures.
import ( + "errors" "fmt" log "github.com/charmbracelet/log" @@ )
74-82: parseDependencyArray: accumulate and return errors.func (p *DependencyParser) parseDependencyArray(fromID, defaultStack string, deps []any) error { - for _, dep := range deps { - if err := p.parseSingleDependency(fromID, defaultStack, dep); err != nil { - log.Warn("Failed to parse dependency", logFieldFrom, fromID, logFieldError, err) - } - } - return nil + var agg error + for _, dep := range deps { + if err := p.parseSingleDependency(fromID, defaultStack, dep); err != nil { + log.Warn("Failed to parse dependency", logFieldFrom, fromID, logFieldError, err) + agg = errors.Join(agg, err) + } + } + return agg }
84-92: parseDependencyMap: accumulate and return errors.func (p *DependencyParser) parseDependencyMap(fromID, defaultStack string, deps map[string]any) error { - for _, dep := range deps { - if err := p.parseSingleDependency(fromID, defaultStack, dep); err != nil { - log.Warn("Failed to parse dependency", logFieldFrom, fromID, logFieldError, err) - } - } - return nil + var agg error + for _, dep := range deps { + if err := p.parseSingleDependency(fromID, defaultStack, dep); err != nil { + log.Warn("Failed to parse dependency", logFieldFrom, fromID, logFieldError, err) + agg = errors.Join(agg, err) + } + } + return agg }
94-102: parseDependencyMapAnyAny: accumulate and return errors.func (p *DependencyParser) parseDependencyMapAnyAny(fromID, defaultStack string, deps map[any]any) error { - for _, dep := range deps { - if err := p.parseSingleDependency(fromID, defaultStack, dep); err != nil { - log.Warn("Failed to parse dependency", logFieldFrom, fromID, logFieldError, err) - } - } - return nil + var agg error + for _, dep := range deps { + if err := p.parseSingleDependency(fromID, defaultStack, dep); err != nil { + log.Warn("Failed to parse dependency", logFieldFrom, fromID, logFieldError, err) + agg = errors.Join(agg, err) + } + } + return agg }
155-159: Wrap builder AddDependency errors with context.Returning the raw error loses context. Wrap with context for better diagnostics. Since underlying builder errors are sentinel values, wrapping preserves them.
- if err := p.builder.AddDependency(fromID, toID); err != nil { - log.Warn("Failed to add dependency", logFieldFrom, fromID, logFieldTo, toID, logFieldError, err) - return err - } + if err := p.builder.AddDependency(fromID, toID); err != nil { + log.Warn("Failed to add dependency", logFieldFrom, fromID, logFieldTo, toID, logFieldError, err) + return fmt.Errorf("add dependency %s -> %s: %w", fromID, toID, err) + }internal/exec/terraform_affected_graph.go (6)
17-20: Add error context when discovering affected components.This currently returns the raw error. Wrap it with context so callers get a clearer failure location.
- affectedList, err := getAffectedComponents(args) - if err != nil { - return err - } + affectedList, err := getAffectedComponents(args) + if err != nil { + return fmt.Errorf("getAffectedComponents: %w", err) + }
28-30: Add error context when logging the affected set.- if err := logAffectedComponents(affectedList); err != nil { - return err - } + if err := logAffectedComponents(affectedList); err != nil { + return fmt.Errorf("logAffectedComponents: %w", err) + }
33-36: Add error context for graph build/filter stage.- filteredGraph, err := buildFilteredDependencyGraph(args, info, affectedList) - if err != nil { - return err - } + filteredGraph, err := buildFilteredDependencyGraph(args, info, affectedList) + if err != nil { + return fmt.Errorf("buildFilteredDependencyGraph: %w", err) + }
176-181: Topological sort errors: consider wrapping with a static sentinel.Per repo guidelines, prefer wrapping with a static sentinel from errors/errors.go (e.g., ErrTopologicalSortFailed) in addition to context. If not available, please add one and wrap here.
192-197: Execution failure: wrap with a static sentinel.Similarly, align error wrapping here with a static sentinel (e.g., ErrTerraformExecutionFailed) and context.
106-114: YAML conversion error should be wrapped with sentinel.Wrap ConvertToYAML errors using a static sentinel (e.g., ErrYAMLSerializationFailed) per coding guidelines.
pkg/dependency/filter.go (2)
152-189: Avoid aliasing original nodes in GetConnectedComponents; clone nodes into subgraphs.The current implementation inserts original node pointers into component graphs. That makes subgraphs share mutable state with the original graph. Prefer cloning into each component subgraph (like Filter does) to preserve isolation.
func (g *Graph) GetConnectedComponents() []*Graph { - visited := make(map[string]bool) - components := []*Graph{} - - // DFS to find all nodes in a connected component - var dfs func(nodeID string, component *Graph) - dfs = func(nodeID string, component *Graph) { - if visited[nodeID] { - return - } - visited[nodeID] = true - - node := g.Nodes[nodeID] - component.Nodes[nodeID] = node - - // Visit all connected nodes (both dependencies and dependents) - for _, depID := range node.Dependencies { - dfs(depID, component) - } - for _, depID := range node.Dependents { - dfs(depID, component) - } - } - - // Find all connected components - for id := range g.Nodes { - if !visited[id] { - component := NewGraph() - dfs(id, component) - component.IdentifyRoots() - components = append(components, component) - } - } - - return components + visited := make(map[string]bool) + components := []*Graph{} + + for id := range g.Nodes { + if visited[id] { + continue + } + + // Discover this component. + toInclude := make(map[string]bool) + var dfs func(nodeID string) + dfs = func(nodeID string) { + if visited[nodeID] { + return + } + visited[nodeID] = true + toInclude[nodeID] = true + node := g.Nodes[nodeID] + for _, depID := range node.Dependencies { + dfs(depID) + } + for _, depID := range node.Dependents { + dfs(depID) + } + } + dfs(id) + + // Clone nodes into an isolated subgraph. + component := NewGraph() + g.copyNodesToFilteredGraph(component, toInclude) + component.IdentifyRoots() + components = append(components, component) + } + + return components }
48-61: Nit: consider copying Metadata maps when cloning.cloneNodeForFilter copies the Metadata map reference. If subgraphs modify Metadata, changes will reflect in the original graph. If immutability is desired, shallow-copy the map.
internal/exec/terraform_all.go (6)
23-23: Add trailing period to comment (golangci-lint godot).- // Get all stacks with terraform components + // Get all stacks with terraform components.
40-40: Add trailing period to comment (golangci-lint godot).- // Build dependency graph + // Build dependency graph.
50-50: Add trailing period to comment (golangci-lint godot).- // Apply filters if specified + // Apply filters if specified.
55-55: Add trailing period to comment (golangci-lint godot).- // Get execution order + // Get execution order.
63-63: Add trailing period to comment (golangci-lint godot).- // Execute components in order + // Execute components in order.
51-53: Prefer passing Atmos config into YQ evaluation for queries.evaluateNodeQuery constructs an empty AtmosConfiguration, missing runtime log settings. Consider threading atmosConfig through applyFiltersToGraph -> filterNodesByQuery -> evaluateNodeQuery to preserve logging config and any future behavior tied to CLI config.
📜 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 (24)
cmd/terraform_utils.go(2 hunks)docs/prd/terraform-dependency-order.md(1 hunks)errors/errors.go(1 hunks)internal/exec/dependency_parser.go(1 hunks)internal/exec/dependency_parser_test.go(1 hunks)internal/exec/terraform_affected_graph.go(1 hunks)internal/exec/terraform_all.go(1 hunks)internal/exec/terraform_all_simple_test.go(1 hunks)internal/exec/terraform_all_test.go(1 hunks)internal/exec/terraform_executor.go(1 hunks)internal/exec/terraform_utils.go(1 hunks)internal/exec/terraform_utils_test.go(2 hunks)pkg/dependency/builder.go(1 hunks)pkg/dependency/errors.go(1 hunks)pkg/dependency/filter.go(1 hunks)pkg/dependency/filter_test.go(1 hunks)pkg/dependency/graph.go(1 hunks)pkg/dependency/graph_test.go(1 hunks)pkg/dependency/sort.go(1 hunks)pkg/dependency/sort_test.go(1 hunks)pkg/dependency/types.go(1 hunks)tests/fixtures/scenarios/terraform-apply-all-dependencies/atmos.yaml(1 hunks)tests/fixtures/scenarios/terraform-apply-all-dependencies/stacks/deploy/dev.yaml(1 hunks)tests/fixtures/scenarios/terraform-apply-all-dependencies/stacks/deploy/prod.yaml(1 hunks)
🧰 Additional context used
📓 Path-based instructions (7)
**/*.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
**/*.go: All code must pass golangci-lint checks
Follow Go error handling idioms and use meaningful error messages
Wrap errors with context using fmt.Errorf("context: %w", err)
Consider custom error types for domain-specific errors
Follow standard Go coding style; run gofmt and goimports
Use snake_case for environment variables
Document complex logic with inline comments
**/*.go: All Go comments must end with periods (enforced by golangci-lint's godot).
Wrap all errors with static errors defined in errors/errors.go; never return dynamic errors directly; use fmt.Errorf with %w and add details after the static error.
Always bind environment variables using viper.BindEnv and provide an ATMOS_ alternative for each external env var.
Use structured logging for system/debug events; logging must not affect execution and should use appropriate levels per docs/logging.md.
Prefer SDKs over shelling out to binaries for cross-platform compatibility; use filepath/os/runtime for portable paths and OS-specific logic.
Files:
internal/exec/terraform_utils.gopkg/dependency/filter_test.goerrors/errors.gopkg/dependency/sort.gopkg/dependency/filter.gointernal/exec/terraform_affected_graph.gopkg/dependency/builder.gointernal/exec/dependency_parser.gointernal/exec/terraform_all_simple_test.gopkg/dependency/types.gopkg/dependency/errors.gointernal/exec/terraform_all_test.gopkg/dependency/sort_test.gopkg/dependency/graph_test.gointernal/exec/terraform_all.gointernal/exec/terraform_executor.gointernal/exec/terraform_utils_test.gopkg/dependency/graph.gointernal/exec/dependency_parser_test.gocmd/terraform_utils.go
**/!(*_test).go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
Document all exported functions, types, and methods with Go doc comments
Files:
internal/exec/terraform_utils.goerrors/errors.gopkg/dependency/sort.gopkg/dependency/filter.gointernal/exec/terraform_affected_graph.gopkg/dependency/builder.gointernal/exec/dependency_parser.gopkg/dependency/types.gopkg/dependency/errors.gointernal/exec/terraform_all.gointernal/exec/terraform_executor.gopkg/dependency/graph.gocmd/terraform_utils.go
pkg/**/*.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
Place business logic in pkg rather than in cmd
Files:
pkg/dependency/filter_test.gopkg/dependency/sort.gopkg/dependency/filter.gopkg/dependency/builder.gopkg/dependency/types.gopkg/dependency/errors.gopkg/dependency/sort_test.gopkg/dependency/graph_test.gopkg/dependency/graph.go
**/*_test.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
**/*_test.go: Every new feature must include comprehensive unit tests
Test both happy paths and error conditions
Use table-driven tests for multiple scenarios
**/*_test.go: Use table-driven tests for unit tests and focus on pure functions where possible.
Always use t.Skipf with a clear reason when skipping tests; never use t.Skip.
For CLI tests requiring rebuilt binaries, set a package-level skipReason in TestMain and call os.Exit(m.Run()); individual tests must check and t.Skipf if set; never use log.Fatal for missing/stale binaries.
Mirror test file names to their implementation files (e.g., aws_ssm_store_test.go for aws_ssm_store.go) and co-locate tests with implementation.
Files:
pkg/dependency/filter_test.gointernal/exec/terraform_all_simple_test.gointernal/exec/terraform_all_test.gopkg/dependency/sort_test.gopkg/dependency/graph_test.gointernal/exec/terraform_utils_test.gointernal/exec/dependency_parser_test.go
pkg/**/*_test.go
📄 CodeRabbit inference engine (CLAUDE.md)
Place unit tests under pkg/** matching the package they test.
Files:
pkg/dependency/filter_test.gopkg/dependency/sort_test.gopkg/dependency/graph_test.go
errors/errors.go
📄 CodeRabbit inference engine (CLAUDE.md)
Define all static errors centrally in errors/errors.go (e.g., ErrInvalidComponent, ErrInvalidStack, ErrInvalidConfig).
Files:
errors/errors.go
cmd/**/*.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
cmd/**/*.go: Use Cobra's recommended command structure with a root command and subcommands
Implement each CLI command in a separate file under cmd/
Use Viper for managing configuration, environment variables, and flags in the CLI
Keep separation of concerns between CLI interface (cmd) and business logic
Use kebab-case for command-line flags
Provide comprehensive help text for all commands and flags
Include examples in Cobra command help
Use Viper for configuration management; support files, env vars, and flags with precedence flags > env > config > defaults
Follow single responsibility; separate command interface from business logic
Provide meaningful user feedback and include progress indicators for long-running operations
Provide clear error messages to users and troubleshooting hints where appropriate
cmd/**/*.go: Use utils.PrintfMarkdown to render embedded markdown content for CLI help/examples.
One Cobra command per file in cmd/ to keep files focused and maintainable.
Send UI prompts/status/progress to stderr and data/results to stdout; never use logging for UI.
Prefer utils.PrintfMessageToTUI for UI messages; only write directly to os.Stderr as a last resort.
For non-standard execution paths, capture telemetry with telemetry.CaptureCmd or telemetry.CaptureCmdString.
Files:
cmd/terraform_utils.go
🧠 Learnings (19)
📚 Learning: 2025-06-23T02:14:30.937Z
Learnt from: aknysh
PR: cloudposse/atmos#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:
internal/exec/terraform_utils.gointernal/exec/terraform_affected_graph.gointernal/exec/terraform_all.gocmd/terraform_utils.go
📚 Learning: 2025-09-23T02:30:42.362Z
Learnt from: CR
PR: cloudposse/atmos#0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-09-23T02:30:42.362Z
Learning: Applies to **/*_test.go : Every new feature must include comprehensive unit tests
Applied to files:
pkg/dependency/filter_test.gointernal/exec/terraform_all_simple_test.gointernal/exec/terraform_all_test.gopkg/dependency/sort_test.gopkg/dependency/graph_test.gointernal/exec/dependency_parser_test.go
📚 Learning: 2024-12-07T16:19:01.683Z
Learnt from: aknysh
PR: cloudposse/atmos#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/terraform_all_simple_test.gointernal/exec/terraform_executor.go
📚 Learning: 2025-09-25T03:30:16.196Z
Learnt from: CR
PR: cloudposse/atmos#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-25T03:30:16.196Z
Learning: Applies to **/*_test.go : Use table-driven tests for unit tests and focus on pure functions where possible.
Applied to files:
internal/exec/terraform_all_simple_test.go
📚 Learning: 2025-09-23T02:30:42.362Z
Learnt from: CR
PR: cloudposse/atmos#0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-09-23T02:30:42.362Z
Learning: Applies to **/*_test.go : Use table-driven tests for multiple scenarios
Applied to files:
internal/exec/terraform_all_simple_test.go
📚 Learning: 2024-10-31T19:23:45.538Z
Learnt from: osterman
PR: cloudposse/atmos#727
File: internal/exec/terraform.go:65-66
Timestamp: 2024-10-31T19:23:45.538Z
Learning: The variable `shouldCheckStack` in `ExecuteTerraform` controls whether validation is performed.
Applied to files:
internal/exec/terraform_all_simple_test.go
📚 Learning: 2025-04-11T22:06:46.999Z
Learnt from: samtholiya
PR: cloudposse/atmos#1147
File: internal/exec/validate_schema.go:42-57
Timestamp: 2025-04-11T22:06:46.999Z
Learning: The "ExecuteAtmosValidateSchemaCmd" function in internal/exec/validate_schema.go has been reviewed and confirmed to have acceptable cognitive complexity despite static analysis warnings. The function uses a clean structure with only three if statements for error handling and delegates complex operations to helper methods.
Applied to files:
internal/exec/terraform_all_simple_test.go
📚 Learning: 2024-11-25T17:17:15.703Z
Learnt from: RoseSecurity
PR: cloudposse/atmos#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/terraform-apply-all-dependencies/atmos.yaml
📚 Learning: 2025-09-25T03:30:16.196Z
Learnt from: CR
PR: cloudposse/atmos#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-25T03:30:16.196Z
Learning: Applies to pkg/datafetcher/schema/{config/global/1.0.json,atmos/manifest/1.0.json,stacks/stack-config/1.0.json,vendor/package/1.0.json} : When adding Atmos configuration options, update all schema files: config/global/1.0.json, atmos/manifest/1.0.json, stacks/stack-config/1.0.json, vendor/package/1.0.json under pkg/datafetcher/schema/.
Applied to files:
tests/fixtures/scenarios/terraform-apply-all-dependencies/atmos.yaml
📚 Learning: 2025-03-18T12:26:25.329Z
Learnt from: Listener430
PR: cloudposse/atmos#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:
tests/fixtures/scenarios/terraform-apply-all-dependencies/atmos.yaml
📚 Learning: 2025-09-10T21:17:55.273Z
Learnt from: samtholiya
PR: cloudposse/atmos#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:
tests/fixtures/scenarios/terraform-apply-all-dependencies/atmos.yaml
📚 Learning: 2024-12-01T00:33:20.298Z
Learnt from: aknysh
PR: cloudposse/atmos#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/terraform-apply-all-dependencies/atmos.yaml
📚 Learning: 2025-01-25T03:51:57.689Z
Learnt from: Listener430
PR: cloudposse/atmos#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/terraform-apply-all-dependencies/atmos.yaml
📚 Learning: 2025-09-25T03:30:16.196Z
Learnt from: CR
PR: cloudposse/atmos#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-25T03:30:16.196Z
Learning: Applies to tests/**/*_test.go : Place integration tests under tests/** with *_test.go naming and use fixtures in tests/test-cases/.
Applied to files:
pkg/dependency/sort_test.go
📚 Learning: 2025-09-25T03:30:16.196Z
Learnt from: CR
PR: cloudposse/atmos#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-25T03:30:16.196Z
Learning: Applies to pkg/**/*_test.go : Place unit tests under pkg/** matching the package they test.
Applied to files:
pkg/dependency/sort_test.go
📚 Learning: 2024-11-02T15:35:09.958Z
Learnt from: aknysh
PR: cloudposse/atmos#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_executor.go
📚 Learning: 2024-10-27T04:41:49.199Z
Learnt from: haitham911
PR: cloudposse/atmos#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/terraform_executor.go
📚 Learning: 2025-04-26T15:54:10.506Z
Learnt from: haitham911
PR: cloudposse/atmos#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/terraform_executor.go
📚 Learning: 2024-11-12T03:16:02.910Z
Learnt from: aknysh
PR: cloudposse/atmos#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/terraform_executor.go
🧬 Code graph analysis (16)
pkg/dependency/filter_test.go (3)
pkg/dependency/graph.go (1)
NewGraph(13-18)pkg/dependency/types.go (2)
Node(4-28)Filter(55-64)pkg/config/const.go (2)
TerraformComponentType(48-48)HelmfileComponentType(49-49)
pkg/dependency/sort.go (2)
pkg/dependency/types.go (3)
Graph(31-37)ExecutionOrder(52-52)Node(4-28)pkg/dependency/errors.go (1)
ErrCircularDependency(29-29)
pkg/dependency/filter.go (2)
pkg/dependency/types.go (3)
Graph(31-37)Filter(55-64)Node(4-28)pkg/dependency/graph.go (1)
NewGraph(13-18)
internal/exec/terraform_affected_graph.go (6)
internal/exec/describe_affected.go (1)
DescribeAffectedCmdArgs(27-48)pkg/schema/schema.go (2)
ConfigAndStacksInfo(455-532)Affected(606-625)internal/exec/describe_affected_helpers.go (3)
ExecuteDescribeAffectedWithTargetRepoPath(346-404)ExecuteDescribeAffectedWithTargetRefClone(29-180)ExecuteDescribeAffectedWithTargetRefCheckout(184-342)pkg/utils/yaml_utils.go (1)
ConvertToYAML(152-166)pkg/dependency/types.go (3)
Graph(31-37)Filter(55-64)Node(4-28)pkg/config/const.go (1)
TerraformComponentType(48-48)
pkg/dependency/builder.go (3)
pkg/dependency/types.go (2)
Graph(31-37)Node(4-28)pkg/dependency/graph.go (1)
NewGraph(13-18)pkg/dependency/errors.go (3)
ErrGraphAlreadyBuilt(26-26)ErrCircularDependency(29-29)ErrNoRootNodes(32-32)
internal/exec/dependency_parser.go (3)
pkg/dependency/builder.go (1)
GraphBuilder(8-12)pkg/config/const.go (2)
SettingsSectionName(60-60)MetadataSectionName(66-66)errors/errors.go (3)
ErrUnsupportedDependencyType(133-133)ErrMissingDependencyField(134-134)ErrDependencyTargetNotFound(135-135)
internal/exec/terraform_all_simple_test.go (2)
pkg/schema/schema.go (1)
ConfigAndStacksInfo(455-532)internal/exec/terraform_all.go (1)
ExecuteTerraformAll(15-75)
internal/exec/terraform_all_test.go (5)
pkg/schema/schema.go (3)
AtmosConfiguration(26-61)ConfigAndStacksInfo(455-532)Components(353-357)pkg/dependency/graph.go (1)
NewGraph(13-18)pkg/dependency/types.go (1)
Node(4-28)pkg/config/const.go (1)
TerraformComponentType(48-48)internal/exec/terraform_all.go (1)
ExecuteTerraformAll(15-75)
pkg/dependency/sort_test.go (2)
pkg/dependency/graph.go (1)
NewGraph(13-18)pkg/dependency/types.go (1)
Node(4-28)
pkg/dependency/graph_test.go (3)
pkg/dependency/graph.go (1)
NewGraph(13-18)pkg/dependency/types.go (1)
Node(4-28)cmd/cmd_utils.go (1)
Contains(750-757)
internal/exec/terraform_all.go (7)
pkg/schema/schema.go (3)
ConfigAndStacksInfo(455-532)Components(353-357)AtmosConfiguration(26-61)pkg/config/config.go (1)
InitCliConfig(25-62)pkg/config/const.go (2)
TerraformComponentType(48-48)MetadataSectionName(66-66)pkg/dependency/types.go (3)
Graph(31-37)Node(4-28)Filter(55-64)pkg/dependency/builder.go (2)
NewBuilder(15-20)GraphBuilder(8-12)internal/exec/dependency_parser.go (1)
NewDependencyParser(28-33)pkg/utils/yq_utils.go (1)
EvaluateYqExpression(48-98)
internal/exec/terraform_executor.go (6)
pkg/dependency/types.go (1)
Node(4-28)pkg/schema/schema.go (1)
ConfigAndStacksInfo(455-532)pkg/config/const.go (1)
MetadataSectionName(66-66)pkg/config/config.go (1)
InitCliConfig(25-62)pkg/utils/yq_utils.go (1)
EvaluateYqExpression(48-98)internal/exec/terraform.go (1)
ExecuteTerraform(64-628)
internal/exec/terraform_utils_test.go (1)
internal/exec/terraform_affected_graph.go (1)
ExecuteTerraformAffectedWithGraph(15-40)
pkg/dependency/graph.go (2)
pkg/dependency/types.go (2)
Graph(31-37)Node(4-28)pkg/dependency/errors.go (6)
ErrNilNode(8-8)ErrEmptyNodeID(11-11)ErrNodeExists(14-14)ErrEmptyDependencyID(17-17)ErrSelfDependency(20-20)ErrNodeNotFound(23-23)
internal/exec/dependency_parser_test.go (5)
pkg/dependency/builder.go (1)
NewBuilder(15-20)internal/exec/dependency_parser.go (2)
NewDependencyParser(28-33)DependencyParser(22-25)pkg/config/const.go (2)
MetadataSectionName(66-66)SettingsSectionName(60-60)pkg/dependency/types.go (1)
Node(4-28)cmd/cmd_utils.go (1)
Contains(750-757)
cmd/terraform_utils.go (3)
internal/exec/terraform_affected_graph.go (1)
ExecuteTerraformAffectedWithGraph(15-40)internal/exec/terraform_all.go (1)
ExecuteTerraformAll(15-75)internal/exec/terraform_utils.go (1)
ExecuteTerraformQuery(430-467)
🪛 markdownlint-cli2 (0.18.1)
docs/prd/terraform-dependency-order.md
47-47: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Build (macos-latest, macos)
- GitHub Check: Build (ubuntu-latest, linux)
- GitHub Check: Run pre-commit hooks
- GitHub Check: Analyze (go)
- GitHub Check: Lint (golangci)
- GitHub Check: Summary
🔇 Additional comments (13)
tests/fixtures/scenarios/terraform-apply-all-dependencies/stacks/deploy/prod.yaml (1)
31-58: Good dependency chain coverage.Fixture cleanly models the two-layer dependency fan-out; great fuel for the new topo sort tests.
tests/fixtures/scenarios/terraform-apply-all-dependencies/stacks/deploy/dev.yaml (1)
6-60: Fixture looks good.Dependencies are modeled correctly for the test scenario.
errors/errors.go (1)
132-136: Good addition of dependency error sentinels.Names and messages are clear and consistent with existing error patterns.
tests/fixtures/scenarios/terraform-apply-all-dependencies/atmos.yaml (1)
1-19: LGTM.Fixture settings align with test needs.
internal/exec/terraform_all_simple_test.go (1)
11-55: Verify ExecuteTerraformAll enforces these validations.Tests expect:
- Missing stack -> error containing "stack is required".
- Component provided with --all -> error containing "component argument can't be used".
Ensure ExecuteTerraformAll performs early validation and returns wrapped static errors (errors.ErrMissingStack and errors.ErrInvalidTerraformComponentWithMultiComponentFlags).
If needed, add this guard (in internal/exec/terraform_all.go):
// At function start: if info.Stack == "" { return fmt.Errorf(errors.ErrStringWrappingFormat, errors.ErrMissingStack, "`--all` requires a stack") } if info.ComponentFromArg != "" { return fmt.Errorf(errors.ErrStringWrappingFormat, errors.ErrInvalidTerraformComponentWithMultiComponentFlags, "component provided with `--all`") }pkg/dependency/graph_test.go (2)
19-50: Solid coverage of node add edge cases.Nil, empty ID, and duplicate scenarios are well asserted.
145-177: Nice clone isolation checks.Verifies node mutation does not affect the original graph.
internal/exec/terraform_utils.go (1)
181-181: No remaining usages of ExecuteTerraformAffected
Verified with ripgrep that only the function’s own definition remains and no external call sites exist. You can safely remove this deprecated function in a future release.internal/exec/terraform_all_test.go (1)
243-286: Verify expectations: ExecuteTerraformAll currently doesn’t enforce these validations.The function snippet shows no explicit stack/argument validation; those checks live in cmd via checkTerraformFlags. Either:
- add validation inside ExecuteTerraformAll, or
- adapt tests to exercise the CLI path that performs the checks.
Please confirm desired contract.
cmd/terraform_utils.go (1)
95-96: Good: switched to graph-aware affected execution.Routing --affected through ExecuteTerraformAffectedWithGraph aligns both paths on the same dependency logic.
internal/exec/terraform_executor.go (1)
64-83: Confirm skip-on-query-error semantics and data scope.Currently, any query evaluation error causes a skip, and the query runs only against node.Metadata[cfg.MetadataSectionName]. Confirm:
- Is skip-on-error intended vs. fail-fast?
- Should the YQ query evaluate against the full component context (settings/vars) instead of just metadata?
internal/exec/dependency_parser.go (1)
116-131: LGTM: field validation and defaulting.Validates component/stack fields and defaults stack to caller’s stack. Error wrapping uses static sentinels. Looks solid.
pkg/dependency/errors.go (1)
5-33: LGTM: well-scoped sentinel errors.Clear, exported, and idiomatic sentinel set for the graph domain. Names and messages are consistent.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
docs/prd/terraform-dependency-order.md (2)
88-92: Explicitly call out reverse-order destroy.Under FR-3.1, add a bullet that destroy runs in reverse topological order to avoid dependency violations.
Apply:
#### FR-3.1: Ordered Execution - System SHALL execute components in dependency order +- For destroy, System SHALL execute components in reverse dependency order - System SHALL respect dry-run mode - System SHALL support all Terraform subcommands (apply, destroy, plan)
281-285: Clarify parallel execution scope.Note that parallelism is out of scope for this PR but is a potential follow-up, with ordering preserved within levels.
Apply:
1. Should we support parallel execution of independent components? + - Out of scope for this PR. Consider future enhancement that executes nodes within the same topological level in parallel while preserving level ordering.pkg/dependency/sort.go (1)
89-152: Remove unused variable and tidy levels computation.
processedis never read. Drop it. No behavior change.Apply:
- processed := make(map[string]bool) nodeLevel := make(map[string]int) @@ - for id, node := range g.Nodes { + for id, node := range g.Nodes { level := nodeLevel[id] levels[level] = append(levels[level], *node) - processed[id] = true }internal/exec/terraform_all.go (3)
61-76: Use provided helper for reverse order or keep as-is.Optional: replace manual reversal with Graph.ReverseTopologicalSort for clarity.
Apply:
- executionOrder, err := graph.TopologicalSort() + executionOrder, err := graph.TopologicalSort() if err != nil { return fmt.Errorf("error determining execution order: %w", err) } // For destroy command, reverse the execution order to destroy dependents before dependencies if info.SubCommand == "destroy" { - for i, j := 0, len(executionOrder)-1; i < j; i, j = i+1, j-1 { - executionOrder[i], executionOrder[j] = executionOrder[j], executionOrder[i] - } + rev, err := graph.ReverseTopologicalSort() + if err != nil { + return fmt.Errorf("error determining execution order: %w", err) + } + executionOrder = rev log.Info("Processing components in reverse dependency order for destroy", "count", len(executionOrder)) } else {
50-51: Fix godot: ensure comments end with periods.A few comments miss trailing periods; linter will fail.
Examples:
- // Apply filters if specified + // Apply filters if specified. @@ - // Get execution order + // Get execution order. @@ - // For destroy command, reverse the execution order to destroy dependents before dependencies + // For destroy command, reverse the execution order to destroy dependents before dependencies. @@ - // Execute components in order + // Execute components in order. @@ - // Build the final graph. + // Build the final graph. @@ - // shouldSkipComponentForGraph determines if a component should be skipped when building the graph. + // shouldSkipComponentForGraph determines if a component should be skipped when building the graph. @@ - // applyFiltersToGraph applies query and component filters to the graph. + // applyFiltersToGraph applies query and component filters to the graph. @@ - // evaluateNodeQuery evaluates a YQ query expression against a node's metadata. + // evaluateNodeQuery evaluates a YQ query expression against a node's metadata. @@ - // hasFilters checks if any filters are specified. + // hasFilters checks if any filters are specified. @@ - // getAllNodeIDs returns all node IDs from the graph. + // getAllNodeIDs returns all node IDs from the graph.Also applies to: 61-65, 67-75, 77-89, 100-115, 120-176, 178-199, 240-267, 284-296
254-267: Pass real Atmos config to YQ evaluation (optional).Use the initialized config for consistent logging/behavior instead of a zero-value config.
Proposed signature change and call sites:
- Change evaluateNodeQuery to accept atmosConfig: evaluateNodeQuery(node *dependency.Node, query string, atmosConfig *schema.AtmosConfiguration).
- Wire through from applyFiltersToGraph if available.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
cmd/terraform_utils.go(2 hunks)docs/prd/terraform-dependency-order.md(1 hunks)internal/exec/terraform_all.go(1 hunks)internal/exec/terraform_all_test.go(1 hunks)pkg/dependency/sort.go(1 hunks)pkg/dependency/sort_test.go(1 hunks)tests/fixtures/scenarios/terraform-apply-all-dependencies/stacks/deploy/prod.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/fixtures/scenarios/terraform-apply-all-dependencies/stacks/deploy/prod.yaml
- pkg/dependency/sort_test.go
🧰 Additional context used
📓 Path-based instructions (5)
pkg/**/*.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
Place business logic in pkg rather than in cmd
Files:
pkg/dependency/sort.go
**/*.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
**/*.go: All code must pass golangci-lint checks
Follow Go error handling idioms and use meaningful error messages
Wrap errors with context using fmt.Errorf("context: %w", err)
Consider custom error types for domain-specific errors
Follow standard Go coding style; run gofmt and goimports
Use snake_case for environment variables
Document complex logic with inline comments
**/*.go: All Go comments must end with periods (enforced by golangci-lint's godot).
Wrap all errors with static errors defined in errors/errors.go; never return dynamic errors directly; use fmt.Errorf with %w and add details after the static error.
Always bind environment variables using viper.BindEnv and provide an ATMOS_ alternative for each external env var.
Use structured logging for system/debug events; logging must not affect execution and should use appropriate levels per docs/logging.md.
Prefer SDKs over shelling out to binaries for cross-platform compatibility; use filepath/os/runtime for portable paths and OS-specific logic.
Files:
pkg/dependency/sort.gointernal/exec/terraform_all_test.gocmd/terraform_utils.gointernal/exec/terraform_all.go
**/!(*_test).go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
Document all exported functions, types, and methods with Go doc comments
Files:
pkg/dependency/sort.gocmd/terraform_utils.gointernal/exec/terraform_all.go
**/*_test.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
**/*_test.go: Every new feature must include comprehensive unit tests
Test both happy paths and error conditions
Use table-driven tests for multiple scenarios
**/*_test.go: Use table-driven tests for unit tests and focus on pure functions where possible.
Always use t.Skipf with a clear reason when skipping tests; never use t.Skip.
For CLI tests requiring rebuilt binaries, set a package-level skipReason in TestMain and call os.Exit(m.Run()); individual tests must check and t.Skipf if set; never use log.Fatal for missing/stale binaries.
Mirror test file names to their implementation files (e.g., aws_ssm_store_test.go for aws_ssm_store.go) and co-locate tests with implementation.
Files:
internal/exec/terraform_all_test.go
cmd/**/*.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
cmd/**/*.go: Use Cobra's recommended command structure with a root command and subcommands
Implement each CLI command in a separate file under cmd/
Use Viper for managing configuration, environment variables, and flags in the CLI
Keep separation of concerns between CLI interface (cmd) and business logic
Use kebab-case for command-line flags
Provide comprehensive help text for all commands and flags
Include examples in Cobra command help
Use Viper for configuration management; support files, env vars, and flags with precedence flags > env > config > defaults
Follow single responsibility; separate command interface from business logic
Provide meaningful user feedback and include progress indicators for long-running operations
Provide clear error messages to users and troubleshooting hints where appropriate
cmd/**/*.go: Use utils.PrintfMarkdown to render embedded markdown content for CLI help/examples.
One Cobra command per file in cmd/ to keep files focused and maintainable.
Send UI prompts/status/progress to stderr and data/results to stdout; never use logging for UI.
Prefer utils.PrintfMessageToTUI for UI messages; only write directly to os.Stderr as a last resort.
For non-standard execution paths, capture telemetry with telemetry.CaptureCmd or telemetry.CaptureCmdString.
Files:
cmd/terraform_utils.go
🧠 Learnings (14)
📓 Common learnings
Learnt from: aknysh
PR: cloudposse/atmos#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.
📚 Learning: 2025-09-23T02:30:42.362Z
Learnt from: CR
PR: cloudposse/atmos#0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-09-23T02:30:42.362Z
Learning: Applies to **/*_test.go : Every new feature must include comprehensive unit tests
Applied to files:
internal/exec/terraform_all_test.go
📚 Learning: 2025-09-25T03:30:16.196Z
Learnt from: CR
PR: cloudposse/atmos#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-25T03:30:16.196Z
Learning: Applies to **/*.go : All Go comments must end with periods (enforced by golangci-lint's godot).
Applied to files:
internal/exec/terraform_all_test.go
📚 Learning: 2025-09-23T02:30:42.362Z
Learnt from: CR
PR: cloudposse/atmos#0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-09-23T02:30:42.362Z
Learning: Applies to **/*.go : Document complex logic with inline comments
Applied to files:
internal/exec/terraform_all_test.go
📚 Learning: 2025-09-23T02:30:42.362Z
Learnt from: CR
PR: cloudposse/atmos#0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-09-23T02:30:42.362Z
Learning: Applies to **/!(*_test).go : Document all exported functions, types, and methods with Go doc comments
Applied to files:
internal/exec/terraform_all_test.go
📚 Learning: 2025-06-23T02:14:30.937Z
Learnt from: aknysh
PR: cloudposse/atmos#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_utils.gointernal/exec/terraform_all.go
📚 Learning: 2025-01-09T22:37:01.004Z
Learnt from: samtholiya
PR: cloudposse/atmos#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_utils.go
📚 Learning: 2024-10-27T04:41:49.199Z
Learnt from: haitham911
PR: cloudposse/atmos#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/terraform_all.go
📚 Learning: 2024-11-02T15:35:09.958Z
Learnt from: aknysh
PR: cloudposse/atmos#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_all.go
📚 Learning: 2024-12-07T16:19:01.683Z
Learnt from: aknysh
PR: cloudposse/atmos#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/terraform_all.go
📚 Learning: 2024-10-23T21:36:40.262Z
Learnt from: osterman
PR: cloudposse/atmos#740
File: cmd/cmd_utils.go:340-359
Timestamp: 2024-10-23T21:36:40.262Z
Learning: In the Go codebase for Atmos, when reviewing functions like `checkAtmosConfig` in `cmd/cmd_utils.go`, avoid suggesting refactoring to return errors instead of calling `os.Exit` if such changes would significantly increase the scope due to the need to update multiple call sites.
Applied to files:
internal/exec/terraform_all.go
📚 Learning: 2024-10-31T19:25:41.298Z
Learnt from: osterman
PR: cloudposse/atmos#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:
internal/exec/terraform_all.go
📚 Learning: 2025-04-26T15:54:10.506Z
Learnt from: haitham911
PR: cloudposse/atmos#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/terraform_all.go
📚 Learning: 2025-09-25T03:30:16.196Z
Learnt from: CR
PR: cloudposse/atmos#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-25T03:30:16.196Z
Learning: Applies to **/*.go : Wrap all errors with static errors defined in errors/errors.go; never return dynamic errors directly; use fmt.Errorf with %w and add details after the static error.
Applied to files:
internal/exec/terraform_all.go
🧬 Code graph analysis (4)
pkg/dependency/sort.go (2)
pkg/dependency/types.go (3)
Graph(31-37)ExecutionOrder(52-52)Node(4-28)pkg/dependency/errors.go (1)
ErrCircularDependency(29-29)
internal/exec/terraform_all_test.go (5)
pkg/schema/schema.go (3)
AtmosConfiguration(26-61)ConfigAndStacksInfo(455-532)Components(353-357)pkg/dependency/graph.go (1)
NewGraph(13-18)pkg/dependency/types.go (1)
Node(4-28)pkg/config/const.go (1)
TerraformComponentType(48-48)internal/exec/terraform_all.go (1)
ExecuteTerraformAll(15-57)
cmd/terraform_utils.go (3)
internal/exec/terraform_affected_graph.go (1)
ExecuteTerraformAffectedWithGraph(15-40)internal/exec/terraform_all.go (1)
ExecuteTerraformAll(15-57)internal/exec/terraform_utils.go (1)
ExecuteTerraformQuery(430-467)
internal/exec/terraform_all.go (7)
pkg/schema/schema.go (3)
ConfigAndStacksInfo(455-532)Components(353-357)AtmosConfiguration(26-61)pkg/config/config.go (1)
InitCliConfig(25-62)pkg/config/const.go (2)
TerraformComponentType(48-48)MetadataSectionName(66-66)pkg/dependency/types.go (3)
Graph(31-37)Node(4-28)Filter(55-64)pkg/dependency/builder.go (2)
NewBuilder(15-20)GraphBuilder(8-12)internal/exec/dependency_parser.go (1)
NewDependencyParser(28-33)pkg/utils/yq_utils.go (1)
EvaluateYqExpression(48-98)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Lint (golangci)
- GitHub Check: Analyze (go)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Build (ubuntu-latest, linux)
- GitHub Check: Run pre-commit hooks
- GitHub Check: Summary
🔇 Additional comments (6)
cmd/terraform_utils.go (2)
95-97: Switch to graph-aware affected flow: looks good.Routing
--affectedthroughExecuteTerraformAffectedWithGraphaligns both paths on the same dependency logic.
106-112: Fix godot: add trailing periods to comments.Comments must end with periods; lint will fail.
Apply:
- // For commands that need dependency order with --all flag (plan, apply, destroy, deploy) + // For commands that need dependency order with --all flag (plan, apply, destroy, deploy). @@ - // For other commands or when not using --all, use existing query logic + // For other commands or when not using --all, use existing query logic.internal/exec/terraform_all_test.go (1)
14-86: Fix godot: add trailing periods to comments.Several comments lack trailing periods; golangci-lint will fail.
Example patch (apply pattern across file):
- // Test building dependency graph from stacks + // Test building dependency graph from stacks. @@ - // Verify nodes exist + // Verify nodes exist. @@ - // Verify execution order + // Verify execution order. @@ - // Test that abstract components are filtered out + // Test that abstract components are filtered out. @@ - // Test that disabled components are filtered out + // Test that disabled components are filtered out. @@ - // Create a test graph + // Create a test graph. @@ - t.Run("filter by stack", func(t *testing.T) { + t.Run("filter by stack.", func(t *testing.T) { @@ - t.Run("filter by components", func(t *testing.T) { + t.Run("filter by components.", func(t *testing.T) { @@ - t.Run("no filters", func(t *testing.T) { + t.Run("no filters.", func(t *testing.T) {Also applies to: 88-113, 132-174, 176-242
pkg/dependency/sort.go (1)
154-192: Guard against missing node IDs in FindPath.Avoid panics when IDs are absent and handle unknown nodes gracefully.
Apply:
func (g *Graph) FindPath(fromID, toID string) ([]string, bool) { + if _, ok := g.Nodes[fromID]; !ok { + return nil, false + } + if _, ok := g.Nodes[toID]; !ok { + return nil, false + } if fromID == toID { return []string{fromID}, true } @@ - node := g.Nodes[currentID] + node, ok := g.Nodes[currentID] + if !ok { + // Unknown node encountered; abort this branch. + return false + } for _, depID := range node.Dependencies { if dfs(depID) { return trueinternal/exec/terraform_all.go (2)
14-21: Wrap with static error sentinel (repo style).Use errors from errors/errors.go per guidelines.
Apply:
-import ( - "fmt" +import ( + "fmt" @@ - cfg "github.com/cloudposse/atmos/pkg/config" + errs "github.com/cloudposse/atmos/errors" + cfg "github.com/cloudposse/atmos/pkg/config"And:
- if err != nil { - return fmt.Errorf("error initializing CLI config: %w", err) - } + if err != nil { + return fmt.Errorf("%w: %v", errs.ErrInitCLIConfig, err) + }Repeat sentinel wraps similarly below:
- Describe stacks: ErrExecuteDescribeStacks.
- Build graph: ErrBuildTerraformDependencyGraph.
- Execution order: ErrTopologicalSort (in executeInDependencyOrder).
- Per-node exec: ErrTerraformExecutionFailed.
Add sentinels if missing.
50-53: Query-only filtering drops everything; seed with all nodes.When only Query is set, base set is empty and filter returns an empty graph.
Apply:
func applyFiltersToGraph(graph *dependency.Graph, _ map[string]any, info *schema.ConfigAndStacksInfo) *dependency.Graph { // Collect node IDs based on filters. nodeIDs := collectFilteredNodeIDs(graph, info) + // If no base filters matched, start from all nodes so query can narrow down. + if len(nodeIDs) == 0 { + nodeIDs = getAllNodeIDs(graph) + } + // Apply query filter if specified. if info.Query != "" { nodeIDs = filterNodesByQuery(graph, nodeIDs, info.Query) } - // If no filters applied, include all nodes. - if !hasFilters(info) { - nodeIDs = getAllNodeIDs(graph) - } - // Filter the graph. return graph.Filter(dependency.Filter{ NodeIDs: nodeIDs, IncludeDependencies: true, // Include what these components depend on. IncludeDependents: false, // Don't include what depends on these. }) }Also applies to: 178-199
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
errors/errors.go (2)
132-140: Add sentinels for graph build/sort/exec to align with error‑wrapping policy.You’re wrapping dynamic errors elsewhere; add dedicated sentinels so callers can use errors.Is and satisfy the repo guideline.
// Dependency errors. ErrUnsupportedDependencyType = errors.New("unsupported dependency type") ErrMissingDependencyField = errors.New("dependency missing required field") ErrDependencyTargetNotFound = errors.New("dependency target not found") + // Terraform graph/sort/execution errors. + ErrBuildTerraformDependencyGraph = errors.New("failed to build terraform dependency graph") + ErrTopologicalSort = errors.New("failed to determine execution order") + ErrTerraformExecutionFailed = errors.New("failed to execute terraform") // Terraform --all flag errors. ErrStackRequiredWithAllFlag = errors.New("stack is required when using --all flag") ErrComponentWithAllFlagConflict = errors.New("component argument can't be used with --all flag")
137-140: Avoid duplicate sentinel; reuse existing multi‑component conflict error.
ErrComponentWithAllFlagConflictduplicatesErrInvalidTerraformComponentWithMultiComponentFlags. Prefer the existing one for consistency.
- Remove
ErrComponentWithAllFlagConflicthere.- In internal/exec/terraform_all.go Line 21, return
errUtils.ErrInvalidTerraformComponentWithMultiComponentFlagsinstead.I can provide the exact diffs if you want to make this change now.
internal/exec/dependency_parser_test.go (2)
243-246: Assert builder errors instead of discarding them.Dropping errors hides failures; assert them to make tests trustworthy.
- _ = builder.AddNode(&dependency.Node{ - ID: nodeID, - }) + assert.NoError(t, builder.AddNode(&dependency.Node{ID: nodeID}))- graph, _ := builder.Build() + graph, err := builder.Build() + assert.NoError(t, err)- _ = builder.AddNode(&dependency.Node{ - ID: "app-dev", - }) + assert.NoError(t, builder.AddNode(&dependency.Node{ID: "app-dev"}))- _ = builder.AddNode(&dependency.Node{ - ID: nodeID, - }) + assert.NoError(t, builder.AddNode(&dependency.Node{ID: nodeID}))- graph, _ := builder.Build() + graph, err := builder.Build() + assert.NoError(t, err)Also applies to: 321-325, 259-260, 418-421, 426-429, 506-507
333-336: Prefer errors.Is over string matching.Use sentinel checks for stability.
Example:
// Before: assert.Contains(t, err.Error(), "unsupported dependency type") // After: assert.ErrorIs(t, err, errors.ErrUnsupportedDependencyType)Do similarly for “missing required field” and “not found” cases with their sentinels.
Also applies to: 437-440
internal/exec/terraform_all.go (2)
253-258: Defensive nil check to avoid potential panic.If nodeIDs ever contains an unknown ID, this avoids a nil deref.
for _, nodeID := range nodeIDs { - node := graph.Nodes[nodeID] + node := graph.Nodes[nodeID] + if node == nil { + continue + } if evaluateNodeQuery(node, query) { filteredNodeIDs = append(filteredNodeIDs, nodeID) } }
21-23: Unify flag-conflict error with existing sentinel.Reuse
ErrInvalidTerraformComponentWithMultiComponentFlagsinstead of the new duplicate.- if info.ComponentFromArg != "" { - return errUtils.ErrComponentWithAllFlagConflict - } + if info.ComponentFromArg != "" { + return errUtils.ErrInvalidTerraformComponentWithMultiComponentFlags + }
📜 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)
errors/errors.go(1 hunks)internal/exec/dependency_parser_test.go(1 hunks)internal/exec/terraform_all.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*_test.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
**/*_test.go: Every new feature must include comprehensive unit tests
Test both happy paths and error conditions
Use table-driven tests for multiple scenarios
**/*_test.go: Use table-driven tests for unit tests and focus on pure functions where possible.
Always use t.Skipf with a clear reason when skipping tests; never use t.Skip.
For CLI tests requiring rebuilt binaries, set a package-level skipReason in TestMain and call os.Exit(m.Run()); individual tests must check and t.Skipf if set; never use log.Fatal for missing/stale binaries.
Mirror test file names to their implementation files (e.g., aws_ssm_store_test.go for aws_ssm_store.go) and co-locate tests with implementation.
Files:
internal/exec/dependency_parser_test.go
**/*.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
**/*.go: All code must pass golangci-lint checks
Follow Go error handling idioms and use meaningful error messages
Wrap errors with context using fmt.Errorf("context: %w", err)
Consider custom error types for domain-specific errors
Follow standard Go coding style; run gofmt and goimports
Use snake_case for environment variables
Document complex logic with inline comments
**/*.go: All Go comments must end with periods (enforced by golangci-lint's godot).
Wrap all errors with static errors defined in errors/errors.go; never return dynamic errors directly; use fmt.Errorf with %w and add details after the static error.
Always bind environment variables using viper.BindEnv and provide an ATMOS_ alternative for each external env var.
Use structured logging for system/debug events; logging must not affect execution and should use appropriate levels per docs/logging.md.
Prefer SDKs over shelling out to binaries for cross-platform compatibility; use filepath/os/runtime for portable paths and OS-specific logic.
Files:
internal/exec/dependency_parser_test.goerrors/errors.gointernal/exec/terraform_all.go
**/!(*_test).go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
Document all exported functions, types, and methods with Go doc comments
Files:
errors/errors.gointernal/exec/terraform_all.go
errors/errors.go
📄 CodeRabbit inference engine (CLAUDE.md)
Define all static errors centrally in errors/errors.go (e.g., ErrInvalidComponent, ErrInvalidStack, ErrInvalidConfig).
Files:
errors/errors.go
🧠 Learnings (11)
📓 Common learnings
Learnt from: aknysh
PR: cloudposse/atmos#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.
📚 Learning: 2025-09-23T02:30:42.362Z
Learnt from: CR
PR: cloudposse/atmos#0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-09-23T02:30:42.362Z
Learning: Applies to **/*_test.go : Every new feature must include comprehensive unit tests
Applied to files:
internal/exec/dependency_parser_test.go
📚 Learning: 2025-09-25T03:30:16.196Z
Learnt from: CR
PR: cloudposse/atmos#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-25T03:30:16.196Z
Learning: Applies to errors/errors.go : Define all static errors centrally in errors/errors.go (e.g., ErrInvalidComponent, ErrInvalidStack, ErrInvalidConfig).
Applied to files:
errors/errors.go
📚 Learning: 2024-10-27T04:41:49.199Z
Learnt from: haitham911
PR: cloudposse/atmos#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/terraform_all.go
📚 Learning: 2024-12-07T16:19:01.683Z
Learnt from: aknysh
PR: cloudposse/atmos#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/terraform_all.go
📚 Learning: 2024-11-02T15:35:09.958Z
Learnt from: aknysh
PR: cloudposse/atmos#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_all.go
📚 Learning: 2025-06-23T02:14:30.937Z
Learnt from: aknysh
PR: cloudposse/atmos#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:
internal/exec/terraform_all.go
📚 Learning: 2024-10-23T21:36:40.262Z
Learnt from: osterman
PR: cloudposse/atmos#740
File: cmd/cmd_utils.go:340-359
Timestamp: 2024-10-23T21:36:40.262Z
Learning: In the Go codebase for Atmos, when reviewing functions like `checkAtmosConfig` in `cmd/cmd_utils.go`, avoid suggesting refactoring to return errors instead of calling `os.Exit` if such changes would significantly increase the scope due to the need to update multiple call sites.
Applied to files:
internal/exec/terraform_all.go
📚 Learning: 2024-10-31T19:25:41.298Z
Learnt from: osterman
PR: cloudposse/atmos#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:
internal/exec/terraform_all.go
📚 Learning: 2025-04-26T15:54:10.506Z
Learnt from: haitham911
PR: cloudposse/atmos#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/terraform_all.go
📚 Learning: 2025-09-25T03:30:16.196Z
Learnt from: CR
PR: cloudposse/atmos#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-25T03:30:16.196Z
Learning: Applies to **/*.go : Wrap all errors with static errors defined in errors/errors.go; never return dynamic errors directly; use fmt.Errorf with %w and add details after the static error.
Applied to files:
internal/exec/terraform_all.go
🧬 Code graph analysis (2)
internal/exec/dependency_parser_test.go (5)
pkg/dependency/builder.go (1)
NewBuilder(15-20)internal/exec/dependency_parser.go (2)
NewDependencyParser(28-33)DependencyParser(22-25)pkg/config/const.go (2)
MetadataSectionName(66-66)SettingsSectionName(60-60)pkg/dependency/types.go (1)
Node(4-28)cmd/cmd_utils.go (1)
Contains(750-757)
internal/exec/terraform_all.go (8)
pkg/schema/schema.go (3)
ConfigAndStacksInfo(455-532)Components(353-357)AtmosConfiguration(26-61)errors/errors.go (2)
ErrStackRequiredWithAllFlag(138-138)ErrComponentWithAllFlagConflict(139-139)pkg/config/config.go (1)
InitCliConfig(25-62)pkg/config/const.go (2)
TerraformComponentType(48-48)MetadataSectionName(66-66)pkg/dependency/types.go (3)
Graph(31-37)Node(4-28)Filter(55-64)pkg/dependency/builder.go (2)
NewBuilder(15-20)GraphBuilder(8-12)internal/exec/dependency_parser.go (1)
NewDependencyParser(28-33)pkg/utils/yq_utils.go (1)
EvaluateYqExpression(48-98)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Run pre-commit hooks
- GitHub Check: Analyze (go)
- GitHub Check: Lint (golangci)
- GitHub Check: Build (ubuntu-latest, linux)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Summary
🔇 Additional comments (2)
internal/exec/terraform_all.go (2)
25-28: Wrap with static sentinels per repo policy.Replace plain fmt.Errorf wraps with sentinels from errors/errors.go.
@@ - atmosConfig, err := cfg.InitCliConfig(*info, true) + atmosConfig, err := cfg.InitCliConfig(*info, true) if err != nil { - return fmt.Errorf("error initializing CLI config: %w", err) + return fmt.Errorf("%w: %v", errUtils.ErrFailedToInitConfig, err) } @@ if err != nil { - return fmt.Errorf("error describing stacks: %w", err) + return fmt.Errorf("%w: %v", errUtils.ErrExecuteDescribeStacks, err) } @@ graph, err := buildTerraformDependencyGraph( &atmosConfig, stacks, info, ) if err != nil { - return fmt.Errorf("error building dependency graph: %w", err) + return fmt.Errorf("%w: %v", errUtils.ErrBuildTerraformDependencyGraph, err) } @@ executionOrder, err := graph.TopologicalSort() if err != nil { - return fmt.Errorf("error determining execution order: %w", err) + return fmt.Errorf("%w: %v", errUtils.ErrTopologicalSort, err) } @@ - if err := executeTerraformForNode(node, info); err != nil { - return fmt.Errorf("error executing terraform for component %s in stack %s: %w", node.Component, node.Stack, err) + if err := executeTerraformForNode(node, info); err != nil { + return fmt.Errorf("%w: component=%s stack=%s: %v", errUtils.ErrTerraformExecutionFailed, node.Component, node.Stack, err) } } @@ - if err := addNodesToGraph(stacks, builder, nodeMap); err != nil { - return nil, fmt.Errorf("error adding nodes to dependency graph: %w", err) + if err := addNodesToGraph(stacks, builder, nodeMap); err != nil { + return nil, fmt.Errorf("%w: %v", errUtils.ErrBuildTerraformDependencyGraph, err) } @@ - if err := buildGraphDependencies(stacks, builder, nodeMap); err != nil { - return nil, fmt.Errorf("error building dependencies: %w", err) + if err := buildGraphDependencies(stacks, builder, nodeMap); err != nil { + return nil, fmt.Errorf("%w: %v", errUtils.ErrBuildTerraformDependencyGraph, err) } @@ - graph, err := builder.Build() - if err != nil { - return nil, fmt.Errorf("error finalizing dependency graph: %w", err) + graph, err := builder.Build() + if err != nil { + return nil, fmt.Errorf("%w: %v", errUtils.ErrBuildTerraformDependencyGraph, err) }Also applies to: 46-47, 55-57, 71-74, 91-93, 109-114, 115-117, 121-123
187-208: Query-only filtering drops all nodes (bug). Seed base-set with all nodes.When only Query is provided, the base set is empty and the query runs over nothing.
func applyFiltersToGraph(graph *dependency.Graph, _ map[string]any, info *schema.ConfigAndStacksInfo) *dependency.Graph { // Collect node IDs based on filters. nodeIDs := collectFilteredNodeIDs(graph, info) + // If base set is empty, seed with all nodes so queries run over the full graph. + if len(nodeIDs) == 0 { + nodeIDs = getAllNodeIDs(graph) + } + // Apply query filter if specified. if info.Query != "" { nodeIDs = filterNodesByQuery(graph, nodeIDs, info.Query) } - // If no filters applied, include all nodes. - if !hasFilters(info) { - nodeIDs = getAllNodeIDs(graph) - } - // Filter the graph. return graph.Filter(dependency.Filter{ NodeIDs: nodeIDs, IncludeDependencies: true, // Include what these components depend on. IncludeDependents: false, // Don't include what depends on these. }) }
|
💥 This pull request now has conflicts. Could you fix it @osterman? 🙏 |
|
Warning Changelog Entry RequiredThis PR is labeled Action needed: Add a new blog post in Example filename: Alternatively: If this change doesn't require a changelog entry, remove the |
|
Warning Changelog Entry RequiredThis PR is labeled Action needed: Add a new blog post in Example filename: Alternatively: If this change doesn't require a changelog entry, remove the |
|
Warning Changelog Entry RequiredThis PR is labeled Action needed: Add a new blog post in Example filename: Alternatively: If this change doesn't require a changelog entry, remove the |
osterman
force-pushed
the
feature/dev-3512-implement-atmos-terraform-apply-all-in-dependency-order
branch
from
d324ff3 to
8645064
Compare
|
Warning Changelog Entry RequiredThis PR is labeled Action needed: Add a new blog post in Example filename: Alternatively: If this change doesn't require a changelog entry, remove the |
|
Warning Changelog Entry RequiredThis PR is labeled Action needed: Add a new blog post in Example filename: Alternatively: If this change doesn't require a changelog entry, remove the |
|
💥 This pull request now has conflicts. Could you fix it @osterman? 🙏 |
mergify
bot
added
conflict
|
Warning Changelog Entry RequiredThis PR is labeled Action needed: Add a new blog post in Example filename: Alternatively: If this change doesn't require a changelog entry, remove the |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (8)
internal/exec/terraform_all_test.go (3)
338-346: Test body does nothing - consider removing or implementing properly.The test iterates through cases but only assigns
ttto_, providing no actual coverage. Either remove this test or add assertions that exercise the behavior through the exported graph-building functions.for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - // This function is internal to terraform_all.go, we'll test it indirectly - // through buildTerraformDependencyGraph tests. - _ = tt // Avoid unused variable warning. - // Test covered by buildTerraformDependencyGraph tests. + stacks := map[string]any{ + "dev": map[string]any{ + "components": map[string]any{ + "terraform": map[string]any{ + tt.componentName: tt.component, + }, + }, + }, + } + atmosConfig := &schema.AtmosConfiguration{} + info := &schema.ConfigAndStacksInfo{} + graph, err := buildTerraformDependencyGraph(atmosConfig, stacks, info) + require.NoError(t, err) + _, exists := graph.GetNode(tt.componentName + "-dev") + assert.Equal(t, tt.expectInclude, exists) }) }
353-413: Remove always-skipped tests per coding guidelines.Tests with
t.Skip()that test non-exported functions add no value. Per guidelines, remove always-skipped tests instead of keeping dead code.
415-513: Same issue - remove these always-skipped tests.Lines 416-459 and 462-513 contain tests that immediately skip. Either remove them or implement proper indirect testing through exported APIs.
internal/exec/dependency_parser.go (1)
76-104: Parse errors are logged but not propagated - incomplete graphs may go unnoticed.When
parseSingleDependencyfails, the error is logged but the function continues and returnsnil. This could result in incomplete dependency graphs without clear indication. Consider collecting errors and returning them, or at minimum warning at a higher level.func (p *DependencyParser) parseDependencyArray(fromID, defaultStack string, deps []any) error { + var errs []error for _, dep := range deps { if err := p.parseSingleDependency(fromID, defaultStack, dep); err != nil { log.Warn("Failed to parse dependency", logFieldFrom, fromID, logFieldError, err) + errs = append(errs, err) } } - return nil + if len(errs) > 0 { + return fmt.Errorf("failed to parse %d dependencies for %s", len(errs), fromID) + } + return nil }internal/exec/terraform_executor.go (2)
134-143: Unused parameter in isNodeComponentEnabled.The
componentNameparameter (named_) is not used. Consider removing it from the signature.-func isNodeComponentEnabled(metadata map[string]any, _ string) bool { +func isNodeComponentEnabled(metadata map[string]any) bool {Note: Update the call site in
shouldSkipNodeaccordingly.
86-100: PassatmosConfigas a parameter to avoid redundant initialization when query filters are applied.
evaluateNodeQueryFilterreinitializescfg.InitCliConfigfor every node when a query filter is present. SinceExecuteTerraformAllalready initializes the config once, passing it as a parameter would eliminate unnecessary overhead on large dependency graphs.internal/exec/terraform_affected_graph.go (2)
142-154: Inconsistent error wrapping - use sentinel errors for consistency.Lines 143 and 153 use plain
%wwrapping while other places use sentinel errors. For consistency and easier error checking:- return nil, fmt.Errorf("error describing stacks: %w", err) + return nil, fmt.Errorf("%w: %v", errUtils.ErrDescribeStacks, err) ... - return nil, fmt.Errorf("error building dependency graph: %w", err) + return nil, fmt.Errorf("%w: %v", errUtils.ErrBuildDepGraph, err)
183-187: Use sentinel error for topological sort failure.- return fmt.Errorf("error determining execution order: %w", err) + return fmt.Errorf("%w: %v", errUtils.ErrTopologicalOrder, err)
📜 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 (11)
docs/prd/terraform-dependency-order.md(1 hunks)errors/errors.go(1 hunks)internal/exec/dependency_parser.go(1 hunks)internal/exec/dependency_parser_test.go(1 hunks)internal/exec/terraform_affected_graph.go(1 hunks)internal/exec/terraform_all.go(1 hunks)internal/exec/terraform_all_simple_test.go(1 hunks)internal/exec/terraform_all_test.go(1 hunks)internal/exec/terraform_executor.go(1 hunks)internal/exec/terraform_utils.go(3 hunks)internal/exec/terraform_utils_test.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- internal/exec/terraform_utils_test.go
- internal/exec/terraform_all_simple_test.go
- internal/exec/dependency_parser_test.go
- internal/exec/terraform_utils.go
🧰 Additional context used
📓 Path-based instructions (3)
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/terraform-dependency-order.md
**/*.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
**/*.go: Use Viper for managing configuration, environment variables, and flags in CLI commands
Use interfaces for external dependencies to facilitate mocking and consider using testify/mock for creating mock implementations
All code must pass golangci-lint checks
Follow Go's error handling idioms: use meaningful error messages, wrap errors with context usingfmt.Errorf("context: %w", err), and consider using custom error types for domain-specific errors
Follow standard Go coding style: usegofmtandgoimportsto format code, prefer short descriptive variable names, use kebab-case for command-line flags, and snake_case for environment variables
Document all exported functions, types, and methods following Go's documentation conventions
Document complex logic with inline comments in Go code
Support configuration via files, environment variables, and flags following the precedence order: flags > environment variables > config file > defaults
Provide clear error messages to users, include troubleshooting hints when appropriate, and log detailed errors for debugging
**/*.go: NEVER use fmt.Fprintf(os.Stdout/Stderr) or fmt.Println(); use data.* or ui.* functions instead
All comments must end with periods (enforced by godot linter)
Organize imports in three groups separated by blank lines, sorted alphabetically: 1) Go stdlib, 2) 3rd-party (NOT cloudposse/atmos), 3) Atmos packages; maintain aliases: cfg, log, u, errUtils
Adddefer perf.Track(atmosConfig, "pkg.FuncName")()+ blank line to all public functions for performance tracking; use nil if no atmosConfig param
All errors MUST be wrapped using static errors defined in errors/errors.go; use errors.Join for combining multiple errors; use fmt.Errorf with %w for adding string context; use error builder for complex errors; use errors.Is() for error checking; NEVER use dynamic errors directly
Use go.uber.org/mock/mockgen with //go:generate directives for mock generation; never create manual mocks
Keep files small...
Files:
internal/exec/terraform_all_test.gointernal/exec/terraform_executor.gointernal/exec/dependency_parser.gointernal/exec/terraform_affected_graph.goerrors/errors.gointernal/exec/terraform_all.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:
internal/exec/terraform_all_test.go
🧠 Learnings (54)
📓 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: osterman
Repo: cloudposse/atmos PR: 1533
File: pkg/config/load.go:585-637
Timestamp: 2025-09-27T20:50:20.564Z
Learning: In the cloudposse/atmos repository, command merging prioritizes precedence over display ordering. Help commands are displayed lexicographically regardless of internal array order, so the mergeCommandArrays function focuses on ensuring the correct precedence chain (top-level file wins) rather than maintaining specific display order.
📚 Learning: 2025-01-17T00:18:57.769Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 944
File: go.mod:206-206
Timestamp: 2025-01-17T00:18:57.769Z
Learning: For indirect dependencies with license compliance issues in the cloudposse/atmos repository, the team prefers to handle them in follow-up PRs rather than blocking the current changes, as these issues often require deeper investigation of the dependency tree.
Applied to files:
docs/prd/terraform-dependency-order.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/terraform-dependency-order.md
📚 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/terraform_all_test.go
📚 Learning: 2025-11-24T17:35:37.209Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-11-24T17:35:37.209Z
Learning: Applies to **/*_test.go : Every new feature must include comprehensive unit tests targeting >80% code coverage for all packages
Applied to files:
internal/exec/terraform_all_test.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:
internal/exec/terraform_all_test.gointernal/exec/terraform_executor.gointernal/exec/terraform_all.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/terraform_all_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:
internal/exec/terraform_all_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/terraform_all_test.gointernal/exec/terraform_executor.gointernal/exec/terraform_affected_graph.gointernal/exec/terraform_all.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/terraform_all_test.gointernal/exec/terraform_executor.gointernal/exec/terraform_all.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:
internal/exec/terraform_all_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:
internal/exec/terraform_all_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/terraform_all_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 : All comments must end with periods (enforced by godot linter)
Applied to files:
internal/exec/terraform_all_test.gointernal/exec/terraform_all.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 : Document complex logic with inline comments in Go code
Applied to files:
internal/exec/terraform_all_test.gointernal/exec/terraform_all.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:
internal/exec/terraform_all_test.gointernal/exec/terraform_all.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:
internal/exec/terraform_all_test.gointernal/exec/terraform_executor.gointernal/exec/terraform_all.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 : All code must pass golangci-lint checks
Applied to files:
internal/exec/terraform_all_test.gointernal/exec/terraform_all.go
📚 Learning: 2025-11-24T17:35:37.209Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-11-24T17:35:37.209Z
Learning: Applies to **/*.go : Follow standard Go coding style: use `gofmt` and `goimports` to format code, prefer short descriptive variable names, use kebab-case for command-line flags, and snake_case for environment variables
Applied to files:
internal/exec/terraform_all_test.gointernal/exec/terraform_all.go
📚 Learning: 2024-10-31T19:23:45.538Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 727
File: internal/exec/terraform.go:65-66
Timestamp: 2024-10-31T19:23:45.538Z
Learning: The variable `shouldCheckStack` in `ExecuteTerraform` controls whether validation is performed.
Applied to files:
internal/exec/terraform_all_test.gointernal/exec/terraform_executor.gointernal/exec/terraform_affected_graph.gointernal/exec/terraform_all.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:
internal/exec/terraform_all_test.gointernal/exec/terraform_executor.gointernal/exec/terraform_affected_graph.gointernal/exec/terraform_all.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:
internal/exec/terraform_all_test.go
📚 Learning: 2025-04-11T22:06:46.999Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 1147
File: internal/exec/validate_schema.go:42-57
Timestamp: 2025-04-11T22:06:46.999Z
Learning: The "ExecuteAtmosValidateSchemaCmd" function in internal/exec/validate_schema.go has been reviewed and confirmed to have acceptable cognitive complexity despite static analysis warnings. The function uses a clean structure with only three if statements for error handling and delegates complex operations to helper methods.
Applied to files:
internal/exec/terraform_all_test.gointernal/exec/terraform_all.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/terraform_all_test.gointernal/exec/terraform_all.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:
internal/exec/terraform_all_test.gointernal/exec/terraform_affected_graph.goerrors/errors.gointernal/exec/terraform_all.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/terraform_all_test.gointernal/exec/terraform_executor.gointernal/exec/terraform_affected_graph.gointernal/exec/terraform_all.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:
internal/exec/terraform_all_test.gointernal/exec/terraform_all.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/terraform_executor.gointernal/exec/terraform_affected_graph.goerrors/errors.gointernal/exec/terraform_all.go
📚 Learning: 2025-10-03T18:02:08.535Z
Learnt from: Benbentwo
Repo: cloudposse/atmos PR: 1475
File: internal/exec/terraform.go:269-272
Timestamp: 2025-10-03T18:02:08.535Z
Learning: In internal/exec/terraform.go, when auth.TerraformPreHook fails, the error is logged but execution continues. This is a deliberate design choice to allow Terraform commands to proceed even if authentication setup fails, rather than failing fast.
Applied to files:
internal/exec/terraform_executor.gointernal/exec/terraform_affected_graph.go
📚 Learning: 2024-11-24T19:13:10.287Z
Learnt from: haitham911
Repo: cloudposse/atmos PR: 727
File: internal/exec/terraform_clean.go:407-416
Timestamp: 2024-11-24T19:13:10.287Z
Learning: In `internal/exec/terraform_clean.go`, when `getStackTerraformStateFolder` returns an error in the `handleCleanSubCommand` function, the error is logged, and the process continues without returning the error.
Applied to files:
internal/exec/terraform_executor.gointernal/exec/terraform_affected_graph.gointernal/exec/terraform_all.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/terraform_executor.gointernal/exec/terraform_affected_graph.goerrors/errors.gointernal/exec/terraform_all.go
📚 Learning: 2024-10-23T21:36:40.262Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 740
File: cmd/cmd_utils.go:340-359
Timestamp: 2024-10-23T21:36:40.262Z
Learning: In the Go codebase for Atmos, when reviewing functions like `checkAtmosConfig` in `cmd/cmd_utils.go`, avoid suggesting refactoring to return errors instead of calling `os.Exit` if such changes would significantly increase the scope due to the need to update multiple call sites.
Applied to files:
internal/exec/terraform_executor.gointernal/exec/dependency_parser.gointernal/exec/terraform_affected_graph.gointernal/exec/terraform_all.go
📚 Learning: 2025-04-04T02:03:23.676Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 1185
File: internal/exec/yaml_func_store.go:26-26
Timestamp: 2025-04-04T02:03:23.676Z
Learning: The Atmos codebase currently uses `log.Fatal` for error handling in multiple places. The maintainers are aware this isn't an ideal pattern (should only be used in main() or init() functions) and plan to address it comprehensively in a separate PR. CodeRabbit should not flag these issues or push for immediate changes until that refactoring is complete.
Applied to files:
internal/exec/terraform_executor.gointernal/exec/dependency_parser.gointernal/exec/terraform_affected_graph.gointernal/exec/terraform_all.go
📚 Learning: 2025-11-24T17:35:37.209Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-11-24T17:35:37.209Z
Learning: Applies to **/*.go : Follow Go's error handling idioms: use meaningful error messages, wrap errors with context using `fmt.Errorf("context: %w", err)`, and consider using custom error types for domain-specific errors
Applied to files:
internal/exec/terraform_executor.gointernal/exec/dependency_parser.gointernal/exec/terraform_affected_graph.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:
internal/exec/terraform_executor.gointernal/exec/terraform_affected_graph.gointernal/exec/terraform_all.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 : All errors MUST be wrapped using static errors defined in errors/errors.go; use errors.Join for combining multiple errors; use fmt.Errorf with %w for adding string context; use error builder for complex errors; use errors.Is() for error checking; NEVER use dynamic errors directly
Applied to files:
internal/exec/terraform_executor.gointernal/exec/terraform_affected_graph.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:
internal/exec/terraform_executor.go
📚 Learning: 2025-10-22T14:55:44.014Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1695
File: pkg/auth/manager.go:169-171
Timestamp: 2025-10-22T14:55:44.014Z
Learning: Go 1.20+ supports multiple %w verbs in fmt.Errorf, which returns an error implementing Unwrap() []error. This is valid and does not panic. Atmos uses Go 1.24.8 and configures errorlint with errorf-multi: true to validate this pattern.
Applied to files:
internal/exec/terraform_executor.gointernal/exec/dependency_parser.gointernal/exec/terraform_affected_graph.go
📚 Learning: 2025-09-10T22:38:42.212Z
Learnt from: Benbentwo
Repo: cloudposse/atmos PR: 1475
File: pkg/auth/identities/aws/user.go:141-145
Timestamp: 2025-09-10T22:38:42.212Z
Learning: The user confirmed that the errors package has an error string wrapping format, contradicting the previous learning about ErrWrappingFormat being invalid. The current usage of fmt.Errorf(errUtils.ErrWrappingFormat, errUtils.ErrAuthAwsFileManagerFailed, err) appears to be the correct pattern.
Applied to files:
internal/exec/terraform_executor.gointernal/exec/terraform_affected_graph.go
📚 Learning: 2025-09-10T22:38:42.212Z
Learnt from: Benbentwo
Repo: cloudposse/atmos PR: 1475
File: pkg/auth/identities/aws/user.go:141-145
Timestamp: 2025-09-10T22:38:42.212Z
Learning: ErrWrappingFormat is correctly defined as "%w: %w" in the errors package and is used throughout the codebase to wrap two error types together. The usage fmt.Errorf(errUtils.ErrWrappingFormat, errUtils.ErrAuthAwsFileManagerFailed, err) is the correct pattern when both arguments are error types.
Applied to files:
internal/exec/terraform_executor.gointernal/exec/terraform_affected_graph.go
📚 Learning: 2025-06-02T14:12:02.710Z
Learnt from: milldr
Repo: cloudposse/atmos PR: 1229
File: internal/exec/workflow_test.go:0-0
Timestamp: 2025-06-02T14:12:02.710Z
Learning: In the atmos codebase, workflow error handling was refactored to use `PrintErrorMarkdown` followed by returning specific error variables (like `ErrWorkflowNoSteps`, `ErrInvalidFromStep`, `ErrInvalidWorkflowStepType`, `ErrWorkflowStepFailed`) instead of `PrintErrorMarkdownAndExit`. This pattern allows proper error testing without the function terminating the process with `os.Exit`, enabling unit tests to assert on error conditions while maintaining excellent user-facing error formatting.
Applied to files:
internal/exec/terraform_executor.gointernal/exec/terraform_affected_graph.gointernal/exec/terraform_all.go
📚 Learning: 2025-06-02T14:12:02.710Z
Learnt from: milldr
Repo: cloudposse/atmos PR: 1229
File: internal/exec/workflow_test.go:0-0
Timestamp: 2025-06-02T14:12:02.710Z
Learning: In the atmos codebase, workflow error handling was refactored to use `PrintErrorMarkdown` followed by returning the error instead of `PrintErrorMarkdownAndExit`. This pattern allows proper error testing without the function terminating the process with `os.Exit`, enabling unit tests to assert on error conditions.
Applied to files:
internal/exec/terraform_executor.gointernal/exec/terraform_affected_graph.gointernal/exec/terraform_all.go
📚 Learning: 2025-12-13T06:07:37.766Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1686
File: docs/prd/tool-dependencies-integration.md:58-64
Timestamp: 2025-12-13T06:07:37.766Z
Learning: cloudposse/atmos: For PRD docs (docs/prd/*.md), markdownlint issues like MD040/MD010/MD034 can be handled in a separate documentation cleanup commit and should not block the current PR.
Applied to files:
internal/exec/terraform_executor.gointernal/exec/terraform_affected_graph.go
📚 Learning: 2024-12-03T05:18:49.169Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 810
File: internal/exec/terraform_utils.go:40-213
Timestamp: 2024-12-03T05:18:49.169Z
Learning: In the context of the Atmos project, it's acceptable for functions like `execTerraformOutput` to remain as single functions if they perform a single purpose, such as retrieving Terraform outputs for a component in a stack, even if the function is lengthy.
Applied to files:
internal/exec/terraform_executor.go
📚 Learning: 2025-10-08T06:48:07.499Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 1602
File: internal/exec/stack_processor_utils.go:968-1003
Timestamp: 2025-10-08T06:48:07.499Z
Learning: The `FindComponentDependenciesLegacy` function in `internal/exec/stack_processor_utils.go` is legacy code that is not actively used and is kept only for backward compatibility purposes.
Applied to files:
internal/exec/dependency_parser.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/dependency_parser.gointernal/exec/terraform_affected_graph.go
📚 Learning: 2025-04-04T02:03:21.906Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 1185
File: internal/exec/yaml_func_store.go:71-72
Timestamp: 2025-04-04T02:03:21.906Z
Learning: The codebase currently uses `log.Fatal` for error handling in library functions, which terminates the program. There is a plan to refactor this approach in a separate PR to improve API design by returning error messages instead of terminating execution.
Applied to files:
internal/exec/dependency_parser.gointernal/exec/terraform_affected_graph.go
📚 Learning: 2025-11-24T17:35:37.209Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-11-24T17:35:37.209Z
Learning: Applies to **/*.go : Provide clear error messages to users, include troubleshooting hints when appropriate, and log detailed errors for debugging
Applied to files:
internal/exec/dependency_parser.gointernal/exec/terraform_affected_graph.go
📚 Learning: 2025-06-07T19:28:21.289Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 1266
File: cmd/describe_affected.go:0-0
Timestamp: 2025-06-07T19:28:21.289Z
Learning: In the Atmos codebase, using panic for unsupported flag types in flag processing functions like setDescribeAffectedFlagValueInCliArgs is the expected behavior rather than returning errors. This pattern is preferred for developer errors when unsupported types are added to the flagsKeyValue map.
Applied to files:
internal/exec/dependency_parser.go
📚 Learning: 2024-11-10T19:28:17.365Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 768
File: internal/exec/vendor_utils.go:0-0
Timestamp: 2024-11-10T19:28:17.365Z
Learning: When TTY is not supported, log the downgrade message at the Warn level using `u.LogWarning(cliConfig, ...)` instead of `fmt.Println`.
Applied to files:
internal/exec/dependency_parser.go
📚 Learning: 2025-01-07T20:38:09.618Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 896
File: cmd/editor_config.go:37-40
Timestamp: 2025-01-07T20:38:09.618Z
Learning: Error handling suggestion for `cmd.Help()` in `cmd/editor_config.go` was deferred as the code is planned for future modifications.
Applied to files:
internal/exec/dependency_parser.go
📚 Learning: 2025-01-25T04:01:58.095Z
Learnt from: Listener430
Repo: cloudposse/atmos PR: 934
File: internal/exec/docs_generate.go:98-101
Timestamp: 2025-01-25T04:01:58.095Z
Learning: In the `generateSingleReadme` function of the docs generation feature (internal/exec/docs_generate.go), errors from `fetchAndParseYAML` should be logged and skipped rather than causing early returns. This is by design to process all inputs and collect all errors, instead of failing fast on the first error.
Applied to files:
internal/exec/dependency_parser.go
📚 Learning: 2025-02-13T07:30:28.946Z
Learnt from: Listener430
Repo: cloudposse/atmos PR: 1061
File: internal/exec/go_getter_utils.go:74-75
Timestamp: 2025-02-13T07:30:28.946Z
Learning: In the `CustomGitDetector.Detect` method of `internal/exec/go_getter_utils.go`, verbose debug logging of raw URLs is intentionally kept for debugging purposes, despite potential credential exposure risks.
Applied to files:
internal/exec/terraform_affected_graph.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_affected_graph.go
🧬 Code graph analysis (3)
internal/exec/terraform_all_test.go (5)
pkg/schema/schema.go (2)
AtmosConfiguration(53-96)ConfigAndStacksInfo(646-742)pkg/dependency/graph.go (1)
NewGraph(13-18)pkg/dependency/types.go (1)
Node(4-28)pkg/config/const.go (1)
TerraformComponentType(57-57)internal/exec/terraform_all.go (1)
ExecuteTerraformAll(16-67)
internal/exec/dependency_parser.go (4)
pkg/dependency/builder.go (1)
GraphBuilder(6-10)pkg/config/const.go (2)
SettingsSectionName(70-70)MetadataSectionName(76-76)pkg/logger/log.go (1)
Warn(44-46)errors/errors.go (3)
ErrUnsupportedDependencyType(468-468)ErrMissingDependencyField(469-469)ErrDependencyTargetNotFound(470-470)
internal/exec/terraform_affected_graph.go (4)
errors/errors.go (3)
ErrDescribeAffected(478-478)ErrFormatForLogging(482-482)ErrBuildDepGraph(480-480)internal/exec/describe_affected_helpers.go (3)
ExecuteDescribeAffectedWithTargetRepoPath(351-411)ExecuteDescribeAffectedWithTargetRefClone(30-183)ExecuteDescribeAffectedWithTargetRefCheckout(187-347)pkg/dependency/types.go (3)
Graph(31-37)Filter(55-64)Node(4-28)pkg/config/const.go (1)
TerraformComponentType(57-57)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Acceptance Tests (macos)
- GitHub Check: Acceptance Tests (windows)
- GitHub Check: Summary
🔇 Additional comments (13)
errors/errors.go (1)
467-484: Well-organized error sentinels for the new dependency graph features.The new sentinels provide clear categorization: dependency-related errors,
--allflag validation, and terraform execution errors. This aligns with the repo's error handling guidelines.docs/prd/terraform-dependency-order.md (1)
1-59: Solid PRD covering the dependency-ordered execution feature.The document provides clear architecture, functional requirements, and testing strategy. The proposed package structure aligns with the implementation.
internal/exec/terraform_all_test.go (2)
14-86: Good test coverage for dependency graph construction.Tests verify graph size, node existence, dependencies, and topological order. The assertions properly validate the vpc → database → app dependency chain.
176-242: Well-structured filter tests with subtests.Clean use of table-driven subtests for filter by stack, components, and no-filter scenarios.
internal/exec/dependency_parser.go (2)
24-36: Clean parser design with builder and node validation.The dependency parser encapsulates graph building and target validation nicely.
106-122: Good support for multiple dependency formats including string shorthand.Handles map variants and the string shorthand cleanly, addressing the previous review feedback.
internal/exec/terraform_executor.go (1)
15-35: Clean orchestration flow for per-node execution.The
executeTerraformForNodefunction has a clear structure: validate → filter → update → execute. Easy to follow.internal/exec/terraform_affected_graph.go (2)
107-120: Good sanitization of affected components for logging.Only logs component@stack pairs, avoiding potential secrets in Settings. Addresses previous review feedback.
15-41: Clear orchestration flow for affected component execution.The function has a logical progression: get affected → log → build graph → execute in order.
internal/exec/terraform_all.go (4)
69-99: Clean handling of destroy command with reverse order.The in-place reversal for destroy ensures dependents are destroyed before their dependencies.
188-215: Filter logic correctly handles query-only case now.Seeds with all node IDs when base set is empty, addressing the previous review feedback about empty graphs.
130-153: No action needed.walkTerraformComponentsis defined ininternal/exec/terraform_utils.goand is properly accessible within theinternal/execpackage.
270-283: Empty AtmosConfiguration is safe and intentional.
EvaluateYqExpressionuses theatmosConfigparameter only for performance tracking (which is marked "reserved for future use") and logger configuration (which explicitly handles empty configs). The core YQ evaluation logic is independent of configuration state.
Dependency Review✅ No vulnerabilities or license issues found.Scanned FilesNone |
Added extensive test suites for the new dependency execution functionality: ## Test Coverage Added: - DependencyParser: Complete unit tests for dependency parsing logic - Graph filtering: Tests for Filter, FilterByType, FilterByStack, FilterByComponent - Graph operations: Tests for RemoveNode, GetConnectedComponents - Build dependency graph: Tests for graph construction from stack configurations - Apply filters: Tests for query and component filtering ## Coverage Results: - pkg/dependency: 89.5% coverage (excellent) - internal/exec: 51.5% coverage (limited by unexported functions) All tests pass successfully, ensuring the refactored code maintains 100% backward compatibility while improving code quality. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
- Add 'plan' command to --all flag dependency-ordered execution - Implement reverse execution order for 'destroy' command - Add deterministic ordering to topological sort for reproducible runs - Fix test error handling - properly assert AddNode/AddDependency errors - Fix markdown linting by adding language specifier to code blocks - Update comment from 'cross-stack' to 'intra-stack' for accuracy - Refactor ExecuteTerraformAll to reduce function complexity These changes improve reliability, test quality, and ensure consistent execution order across runs. The destroy command now properly removes dependents before their dependencies. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
…idation - Fixed test setup in dependency_parser_test.go to properly add source nodes to graph builder before testing dependencies - Fixed TestDependencyParser_ParseSingleDependency by adding all required nodes to the graph - Fixed TestDependencyParser_ParseDependencyMapEntry by adding source node to graph builder - Corrected TestDependencyParser_AddDependencyIfExists to expect self-dependency as an error - Added proper validation for --all flag usage in ExecuteTerraformAll function - Added static error definitions for --all flag validation errors - Fixed linting errors by using static errors instead of dynamic ones All tests now pass successfully and code meets linting standards
Refactored the GetConnectedComponents function to reduce cognitive complexity from 39 to approximately 10, making the code more maintainable and testable. Key improvements: - Extract reusable node cloning functions into node_helpers.go - Create dedicated functions for graph traversal (findConnectedComponent, traverseConnectedNodes) - Extract component building logic (buildComponentGraph) - Simplify main GetConnectedComponents to orchestrate helper functions - Update existing Clone and cloneNodeForFilter to use new helpers - Add comprehensive unit tests for all new functions Benefits: - Reduced cognitive complexity below threshold (from 39 to ~10) - Improved testability with single-responsibility functions - Better code reuse across graph operations - No breaking changes - all existing tests pass 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
- Add sentinel errors for dependency graph operations - Return error for unknown depends_on formats (instead of just logging) - Use sentinel errors consistently in terraform_all.go and terraform_executor.go - Wrap builder errors with context in pkg/dependency/builder.go - Add guard for missing nodes in FindPath to prevent panics - Fix diamond test assertion to use ElementsMatch for proper verification - Fix godot comment formatting (trailing periods) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
- Fix date typo in PRD (2024 → 2025) - Add trailing period to comment (godot linter) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Announces the new dependency-ordered execution for the --all flag, explaining topological sorting, reverse order for destroy, and circular dependency detection. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Added changelog link and updated description for the `plan --all` and `apply --all` milestone in the Feature Parity with Terragrunt initiative. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
osterman
force-pushed
the
feature/dev-3512-implement-atmos-terraform-apply-all-in-dependency-order
branch
from
5e82866 to
9006762
Compare
|
💥 This pull request now has conflicts. Could you fix it @osterman? 🙏 |
Merge branch 'main' and fix all lint issues: - Resolve merge conflicts in terraform_utils.go and roadmap.js - Fix isAbstractComponent redeclaration by renaming to isAbstractMetadata - Fix errorlint: change %v to %w for proper error wrapping (16 fixes) - Add missing perf.Track() calls to all public functions (25+ functions) - Fix cyclomatic complexity by extracting helpers - Fix function-length by compacting blank lines - Fix unparam: remove unused error returns from parse functions - Extract repeated error format string to constant - Reduce argument count with affectedDepOrderParams struct Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
github-actions
bot
added
size/l
…rderParams The executeTerraformAffectedComponentInDepOrder function was refactored to accept *affectedDepOrderParams instead of individual string parameters, but the test mocks still used the old signature, causing a panic in CI. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
github-actions
bot
added
size/xl
aknysh
deleted the
feature/dev-3512-implement-atmos-terraform-apply-all-in-dependency-order
branch
|
These changes were released in v1.209.0. |
1 similar comment
|
These changes were released in v1.209.0. |
2 participants
what
atmos terraform apply --allcommand--alland--affectedflags can usewhy
--allflag was processing components without respecting dependency order, which could lead to deployment failures--affectedfunctionality and needed to be generalizedKey Features
--allflag--alland--affectedImplementation Details
New Dependency Graph Package (
pkg/dependency/)Created a generalized, reusable dependency graph implementation:
Terraform Execution Updates
--allflagTesting
Example
Given this configuration:
Running
atmos terraform apply --allwill execute in order:Testing
go test ./pkg/dependency/...go test ./internal/exec -run TestBuildTerraformDependencyGraphatmos terraform plan --all --dry-runintests/fixtures/scenarios/terraform-apply-all-dependencies/references
atmos terraform deploy $component --all#292Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.