feat: Atmos List Instances, Pro Upload #1254
Conversation
|
💥 This pull request now has conflicts. Could you fix it @milldr? 🙏 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1254 +/- ##
==========================================
+ Coverage 56.72% 56.87% +0.15%
==========================================
Files 280 284 +4
Lines 29708 30281 +573
==========================================
+ Hits 16851 17223 +372
- Misses 11045 11223 +178
- Partials 1812 1835 +23
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 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: 1
🧹 Nitpick comments (3)
pkg/utils/slice_test.go (2)
70-152: Add explicit test for empty flag name behavior.SliceRemoveFlag returns a shallow copy when flagName == "". Add a case to lock this in.
Apply:
@@ { name: "flag name with special characters", input: []string{"--flag-name", "--flag-name=value", "other"}, flagName: "flag-name", expected: []string{"other"}, }, + { + name: "empty flag name (no-op, returns copy)", + input: []string{"--flag1", "value1"}, + flagName: "", + expected: []string{"--flag1", "value1"}, + },
154-254: Edge cases: values that look like flags.SliceRemoveFlagAndValue keeps a following value if it starts with "-". Add cases for negative numbers and values like "-foo" to capture intended semantics.
Apply:
@@ { name: "mixed flag forms", input: []string{"--flag1", "value1", "--flag1=value2", "other"}, flagName: "flag1", expected: []string{"other"}, }, + { + name: "spaced value starting with dash is preserved", + input: []string{"--flag1", "-42", "other"}, + flagName: "flag1", + expected: []string{"-42", "other"}, + }, + { + name: "spaced value starting with single dash text is preserved", + input: []string{"--flag1", "-foo", "--x"}, + flagName: "flag1", + expected: []string{"-foo", "--x"}, + },Also confirm this is the desired UX for your CLI parser.
pkg/utils/slice_utils.go (1)
137-139: Fix doc comment to satisfy godot and GoDoc style (complete first sentence).The first line ends mid-sentence; make it a full sentence ending with a period.
Apply:
-// SliceRemoveFlagAndValue removes --flag and an optional following value (if the next arg -// -// does not start with "-"). It preserves order of remaining args. +// SliceRemoveFlagAndValue removes a flag (--name) and an optional following value if the next +// argument does not start with "-". It preserves the order of remaining args.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
pkg/utils/slice_test.go(1 hunks)pkg/utils/slice_utils.go(6 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
**/*.go: Use Viper for managing configuration, environment variables, and flags
Use interfaces for external dependencies to facilitate mocking
All code must pass golangci-lint checks
Follow Go's error handling idioms
Use meaningful error messages
Wrap errors with context using fmt.Errorf("context: %w", err)
Consider using a custom error type for domain-specific errors
Follow standard Go coding style
Use gofmt and goimports to format code
Prefer short, descriptive variable names
Use snake_case for environment variables
Document all exported functions, types, and methods
Document complex logic with inline comments
Follow Go's documentation conventions
Use Viper for configuration management
Support configuration via files, environment variables, and flags
Follow the precedence order: flags > environment variables > config file > defaults
**/*.go: All comments in Go code must end with periods (enforced by golangci-lint godot).
Wrap all returned errors using static errors from the errors package; never return dynamic errors directly.
Always bind environment variables with viper.BindEnv() and provide ATMOS_ alternatives for each external var.
Separate structured logging from UI output: use stderr for prompts/errors to user; stdout only for data; never use logging for UI.
Most text UI must go to stderr; only data/results to stdout. Prefer utils.PrintfMessageToTUI for UI messages.
For non-standard execution paths, capture telemetry with telemetry.CaptureCmd or telemetry.CaptureCmdString and never capture user data.
Ensure cross-platform compatibility: prefer SDKs to external binaries, use filepath/os/runtime for portability, and add build constraints when needed.
Files:
pkg/utils/slice_test.gopkg/utils/slice_utils.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 testing multiple scenarios
Include integration tests for command flows
Test CLI end-to-end when possible
Use test fixtures for complex inputs
Consider using testify/mock for creating mock implementations
**/*_test.go: Always use t.Skipf() with a reason for skipped tests; never use t.Skip() or reasonless t.Skipf().
Test files must mirror implementation structure and naming (e.g., aws_ssm_store_test.go).
Use table-driven unit tests and mock interfaces for external dependencies; target >80% coverage.
Files:
pkg/utils/slice_test.go
pkg/**/*_test.go
📄 CodeRabbit inference engine (CLAUDE.md)
Unit tests reside under pkg/ alongside implementations.
Files:
pkg/utils/slice_test.go
🧠 Learnings (1)
📚 Learning: 2025-09-07T15:13:17.831Z
Learnt from: CR
PR: cloudposse/atmos#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-07T15:13:17.831Z
Learning: Applies to **/*.go : All comments in Go code must end with periods (enforced by golangci-lint godot).
Applied to files:
pkg/utils/slice_utils.go
🧬 Code graph analysis (1)
pkg/utils/slice_test.go (1)
pkg/utils/slice_utils.go (4)
SliceOfInterfacesToSliceOfStrings(62-70)SliceRemoveString(95-105)SliceRemoveFlag(110-135)SliceRemoveFlagAndValue(139-159)
⏰ 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). (3)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Analyze (go)
- GitHub Check: Lint (golangci)
🔇 Additional comments (9)
pkg/utils/slice_test.go (2)
22-69: Solid, table-driven coverage for SliceRemoveString.Covers presence, absence, empty slice, duplicates, tail removal. Nice touch with range-var rebinding.
61-63: Godot: comment punctuation in tests.Inline comment should end with a period to appease golangci-lint’s godot rule.
Apply:
- tc := tc // rebind to avoid range-variable capture + tc := tc // Rebind to avoid range-variable capture.pkg/utils/slice_utils.go (7)
4-7: Imports updated appropriately.Switch to standard errors package is correct.
9-13: Sentinel errors are well-scoped and idiomatic.Good names and messages.
60-70: Converter (fmt.Sprintf path) LGTM.Clear doc, simple and safe behavior.
91-106: First-occurrence removal is correct and avoids retaining references.Behavior and docs match; mutation note is helpful.
107-136: Flag-only removal is correct; copy semantics for empty flag name are good.Capacity preallocation and no aliasing are handled.
140-159: Behavior for values starting with "-" merits an explicit note.Current logic treats any next token starting with "-" as another flag, not a value (e.g., "-42"). Clarify in the doc or adjust if you intend to remove such values.
I can update docs and tests to codify this either way once you confirm intended semantics.
72-89: Wrap returned errors with context; align with project error policy.Return ErrNilInput is unwrapped; and the non-string error wraps with sentinel first, not “context: %w” form. Use consistent contextual wrapping per guidelines.
Apply:
-func SliceOfInterfacesToSliceOfStringsWithTypeAssertion(input []any) ([]string, error) { +func SliceOfInterfacesToSliceOfStringsWithTypeAssertion(input []any) ([]string, error) { if input == nil { - return nil, ErrNilInput + return nil, fmt.Errorf("utils.SliceOfInterfacesToSliceOfStringsWithTypeAssertion: %w", ErrNilInput) } output := make([]string, len(input)) for i, current := range input { s, ok := current.(string) if !ok { - return nil, fmt.Errorf("%w: index=%d, got=%T", ErrNonStringElement, i, current) + return nil, fmt.Errorf( + "utils.SliceOfInterfacesToSliceOfStringsWithTypeAssertion: index=%d, got=%T: %w", + i, current, ErrNonStringElement, + ) } output[i] = s } return output, nil }
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
pkg/utils/slice_test.go (4)
10-21: Add a mixed‑types case for SliceOfInterfacesToSliceOfStrings.This function stringifies any type; add coverage for ints/bools/nil.
Apply this addition:
func TestSliceOfInterfacesToSliceOfStrings(t *testing.T) { var input []any input = append(input, "a") input = append(input, "b") input = append(input, "c") result := SliceOfInterfacesToSliceOfStrings(input) assert.Equal(t, len(input), len(result)) assert.Equal(t, input[0].(string), result[0]) assert.Equal(t, input[1].(string), result[1]) assert.Equal(t, input[2].(string), result[2]) } + +func TestSliceOfInterfacesToSliceOfStrings_MixedTypes(t *testing.T) { + input := []any{"a", 1, true, nil} + got := SliceOfInterfacesToSliceOfStrings(input) + assert.Equal(t, []string{"a", "1", "true", "<nil>"}, got) +}
71-153: Add empty flagName (no‑op) case and verify clone semantics.Function clones when flagName == ""; assert behavior and non‑aliasing.
Apply this diff inside testCases and subtest:
testCases := []struct { name string input []string flagName string expected []string }{ + { + name: "empty flag name (no-op, returns clone)", + input: []string{"--flag1", "value1"}, + flagName: "", + expected: []string{"--flag1", "value1"}, + }, } @@ t.Run(tc.name, func(t *testing.T) { result := SliceRemoveFlag(tc.input, tc.flagName) assert.Equal(t, tc.expected, result) + if tc.flagName == "" && len(tc.input) > 0 && len(result) > 0 { + // Ensure a clone was returned (different backing array). + assert.NotSame(t, &tc.input[0], &result[0]) + } })
155-255: Deduplicate identical cases to reduce noise.There are repeated scenarios (e.g., “remove flag with spaced value”, “remove flag with spaced value followed by another flag”). Keep one of each.
Example minimalization:
testCases := []struct { @@ - { name: "remove flag without value", input: []string{"--flag1", "value1", "--flag2", "value2"}, flagName: "flag1", expected: []string{"--flag2", "value2"} }, - { name: "remove flag with spaced value", input: []string{"--flag1", "value1", "--flag2", "value2"}, flagName: "flag1", expected: []string{"--flag2", "value2"} }, + { name: "remove flag with spaced value", input: []string{"--flag1", "value1", "--flag2", "value2"}, flagName: "flag1", expected: []string{"--flag2", "value2"} }, @@ - { name: "remove flag with spaced value followed by another flag", input: []string{"--flag1", "value1", "--flag2", "value2"}, flagName: "flag1", expected: []string{"--flag2", "value2"} }, - { name: "remove flag with spaced value followed by another flag", input: []string{"--flag1", "value1", "--flag2", "value2"}, flagName: "flag1", expected: []string{"--flag2", "value2"} }, + { name: "remove flag with spaced value followed by another flag", input: []string{"--flag1", "value1", "--flag2", "value2"}, flagName: "flag1", expected: []string{"--flag2", "value2"} },Also consider t.Parallel within subtests for all table-driven tests.
- t.Run(tc.name, func(t *testing.T) { + t.Run(tc.name, func(t *testing.T) { + t.Parallel()
348-384: Prefer ErrorIs over Equal for ErrNilInput.Future-proof against wrapped returns.
Apply this diff:
- if tc.errorType == ErrNilInput { - assert.Equal(t, ErrNilInput, err) + if tc.errorType == ErrNilInput { + assert.ErrorIs(t, err, ErrNilInput)Optional: add t.Parallel in the subtests here as well.
- t.Run(tc.name, func(t *testing.T) { + t.Run(tc.name, func(t *testing.T) { + t.Parallel()
📜 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)
pkg/config/load.go(1 hunks)pkg/schema/schema.go(4 hunks)pkg/utils/slice_test.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/config/load.go
🧰 Additional context used
📓 Path-based instructions (3)
**/*.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
**/*.go: Use Viper for managing configuration, environment variables, and flags
Use interfaces for external dependencies to facilitate mocking
All code must pass golangci-lint checks
Follow Go's error handling idioms
Use meaningful error messages
Wrap errors with context using fmt.Errorf("context: %w", err)
Consider using a custom error type for domain-specific errors
Follow standard Go coding style
Use gofmt and goimports to format code
Prefer short, descriptive variable names
Use snake_case for environment variables
Document all exported functions, types, and methods
Document complex logic with inline comments
Follow Go's documentation conventions
Use Viper for configuration management
Support configuration via files, environment variables, and flags
Follow the precedence order: flags > environment variables > config file > defaults
**/*.go: All comments in Go code must end with periods (enforced by golangci-lint godot).
Wrap all returned errors using static errors from the errors package; never return dynamic errors directly.
Always bind environment variables with viper.BindEnv() and provide ATMOS_ alternatives for each external var.
Separate structured logging from UI output: use stderr for prompts/errors to user; stdout only for data; never use logging for UI.
Most text UI must go to stderr; only data/results to stdout. Prefer utils.PrintfMessageToTUI for UI messages.
For non-standard execution paths, capture telemetry with telemetry.CaptureCmd or telemetry.CaptureCmdString and never capture user data.
Ensure cross-platform compatibility: prefer SDKs to external binaries, use filepath/os/runtime for portability, and add build constraints when needed.
Files:
pkg/utils/slice_test.gopkg/schema/schema.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 testing multiple scenarios
Include integration tests for command flows
Test CLI end-to-end when possible
Use test fixtures for complex inputs
Consider using testify/mock for creating mock implementations
**/*_test.go: Always use t.Skipf() with a reason for skipped tests; never use t.Skip() or reasonless t.Skipf().
Test files must mirror implementation structure and naming (e.g., aws_ssm_store_test.go).
Use table-driven unit tests and mock interfaces for external dependencies; target >80% coverage.
Files:
pkg/utils/slice_test.go
pkg/**/*_test.go
📄 CodeRabbit inference engine (CLAUDE.md)
Unit tests reside under pkg/ alongside implementations.
Files:
pkg/utils/slice_test.go
🧠 Learnings (6)
📚 Learning: 2025-07-01T02:22:25.901Z
Learnt from: CR
PR: cloudposse/atmos#0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-07-01T02:22:25.901Z
Learning: Applies to **/*_test.go : Test both happy paths and error conditions
Applied to files:
pkg/utils/slice_test.go
📚 Learning: 2025-07-01T02:22:25.901Z
Learnt from: CR
PR: cloudposse/atmos#0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-07-01T02:22:25.901Z
Learning: Applies to **/*_test.go : Use test fixtures for complex inputs
Applied to files:
pkg/utils/slice_test.go
📚 Learning: 2025-07-01T02:22:25.901Z
Learnt from: CR
PR: cloudposse/atmos#0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-07-01T02:22:25.901Z
Learning: Applies to **/*_test.go : Consider using testify/mock for creating mock implementations
Applied to files:
pkg/utils/slice_test.go
📚 Learning: 2025-07-01T02:22:25.901Z
Learnt from: CR
PR: cloudposse/atmos#0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-07-01T02:22:25.901Z
Learning: Applies to **/*_test.go : Use table-driven tests for testing multiple scenarios
Applied to files:
pkg/utils/slice_test.go
📚 Learning: 2025-07-01T02:22:25.901Z
Learnt from: CR
PR: cloudposse/atmos#0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-07-01T02:22:25.901Z
Learning: Applies to **/*_test.go : Every new feature must include comprehensive unit tests
Applied to files:
pkg/utils/slice_test.go
📚 Learning: 2025-09-07T15:13:17.831Z
Learnt from: CR
PR: cloudposse/atmos#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-07T15:13:17.831Z
Learning: Applies to **/*.go : All comments in Go code must end with periods (enforced by golangci-lint godot).
Applied to files:
pkg/schema/schema.go
🧬 Code graph analysis (1)
pkg/utils/slice_test.go (2)
pkg/utils/slice_utils.go (7)
SliceOfInterfacesToSliceOfStrings(62-70)SliceRemoveString(95-105)SliceRemoveFlag(110-135)SliceRemoveFlagAndValue(139-159)ErrNilInput(11-11)ErrNonStringElement(12-12)SliceOfInterfacesToSliceOfStringsWithTypeAssertion(74-89)cmd/cmd_utils.go (1)
Contains(750-757)
⏰ 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). (4)
- GitHub Check: Build (macos-latest, macos)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Analyze (go)
- GitHub Check: Lint (golangci)
🔇 Additional comments (7)
pkg/utils/slice_test.go (2)
4-4: Import of fmt is appropriate.Used for precise error message assertions; good call.
23-69: Solid table for SliceRemoveString.Covers presence, absence, empty, duplicates, last element.
pkg/schema/schema.go (5)
14-14: Godot punctuation fix looks good.Doc comment now ends with a period; satisfies golangci-lint godot.
662-662: Doc comment punctuation fixed.Ends with a period; consistent with godot.
299-299: Approve — WorkspacesEnabled nil-safety verifiedrg shows two usages in internal/exec/terraform_utils.go (lines ~164 and ~173) that guard WorkspacesEnabled != nil before deref; no change required. The suggested helper for centralized defaulting is optional.
712-716: ComponentManifest: omitempty OK — kind is validated; apiVersion is not
- readComponentManifest enforces Kind == "Component" (pkg/list/list_vendor.go:493–497).
- APIVersion isn’t checked; tests construct manifests without apiVersion (pkg/list/list_vendor_test.go). Decide whether apiVersion should be required — if so, add a validation in readComponentManifest to reject empty/invalid apiVersion.
719-724: Lint: comments missing terminal periods — verification requiredAutomated scans failed in the sandbox (rg/PCRE and Python here-doc errors). Run this locally to list // comments that do not end with a period (excludes //go: and //nolint):
awk ' /^\s*\/\// { c=$0; sub(/^\s*\/\//,"",c); if (c ~ /^\s*(go:|nolint)/) next; if (c !~ /\.\s*$/) print FILENAME ":" NR ": " $0 }' pkg/schema/schema.goPreviously flagged: lines 719–724, 731–733, 257–261.
|
💥 This pull request now has conflicts. Could you fix it @milldr? 🙏 |
milldr
changed the title
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/git/git.go (1)
3-9: Create a centralized errors package (errors/errors.go) and use it from pkg/git/git.go.No errors package was found in the repo; many Err* vars are defined across files (e.g. pkg/validator/schema_validator.go: ErrSchemaNotFound, pkg/utils/file_utils.go: ErrFailedToProcessHclFile, pkg/utils/yaml_func_env.go: ErrInvalidAtmosYAMLFunction). Add errors/errors.go exporting static Err* values, import it in pkg/git/git.go (alias e.g. repoerrors "github.com/cloudposse/atmos/errors"), and replace direct fmt.Errorf uses with wrapped static errors per repo guidelines (fmt.Errorf("%w: ", repoerrors.ErrX)).
🧹 Nitpick comments (7)
pkg/utils/slice_test.go (6)
10-21: Add mixed-type and nil cases for SliceOfInterfacesToSliceOfStrings.
Broaden coverage to verify fmt-stringification and nil behavior.Add this test:
func TestSliceOfInterfacesToSliceOfStrings_MixedAndNil(t *testing.T) { t.Parallel() cases := []struct { name string input []any expected []string }{ {"nil", nil, nil}, {"empty", []any{}, []string{}}, {"mixed types", []any{"a", 1, true, 3.14, nil}, []string{"a", "1", "true", "3.14", "<nil>"}}, } for _, tc := range cases { tc := tc t.Run(tc.name, func(t *testing.T) { got := SliceOfInterfacesToSliceOfStrings(tc.input) assert.Equal(t, tc.expected, got) }) } }
23-69: Add nil-slice case for SliceRemoveString.
Covers nil input semantics (current implementation returns nil).Apply this diff inside testCases:
{ name: "remove last element", input: []string{"a", "b", "c"}, remove: "c", expected: []string{"a", "b"}, }, + { + name: "nil slice", + input: nil, + remove: "a", + expected: nil, + },
71-153: Add empty flagName (no-op) case for SliceRemoveFlag.
Implementation returns a copy when flagName == "".Apply this diff inside testCases:
{ name: "flag name with special characters", input: []string{"--flag-name", "--flag-name=value", "other"}, flagName: "flag-name", expected: []string{"other"}, }, + { + name: "empty flag name (no-op)", + input: []string{"--flag1", "value1"}, + flagName: "", + expected: []string{"--flag1", "value1"}, + },
155-255: Remove duplicate test cases in SliceRemoveFlagAndValue.
Several scenarios are identical; trim to reduce noise and runtime.Apply this diff to drop duplicates:
{ - name: "remove flag with spaced value", - input: []string{"--flag1", "value1", "--flag2", "value2"}, - flagName: "flag1", - expected: []string{"--flag2", "value2"}, - }, - { name: "remove flag with equals value", input: []string{"--flag1=value1", "--flag2", "value2"}, flagName: "flag1", expected: []string{"--flag2", "value2"}, }, @@ { - name: "remove flag with spaced value followed by another flag", - input: []string{"--flag1", "value1", "--flag2", "value2"}, - flagName: "flag1", - expected: []string{"--flag2", "value2"}, - }, - { name: "remove multiple occurrences", input: []string{"--flag1", "value1", "--flag1", "value2", "other"}, flagName: "flag1", expected: []string{"other"}, },
375-381: Prefer ErrorIs over direct equality for ErrNilInput.
More robust if wrapping is introduced later.Apply:
- assert.Equal(t, ErrNilInput, err) + assert.ErrorIs(t, err, ErrNilInput)
363-401: Mark helpers with t.Helper() for better failure reports.
Improves test diagnostics.Apply:
func assertErrorCase(t *testing.T, tc *struct { @@ }, err error, result []string, ) { + t.Helper() assert.Error(t, err) assert.Nil(t, result) @@ } // assertNonStringElementError validates ErrNonStringElement specific assertions. func assertNonStringElementError(t *testing.T, input []any, err error) { + t.Helper() // Verify the error wraps ErrNonStringElement assert.ErrorIs(t, err, ErrNonStringElement)pkg/git/git.go (1)
109-114: Interface shape is serviceable; minor naming/coupling nits.
- Consider renaming GetLocalRepo() to GetLocalRepoInfo() to avoid confusion with the package-level GetLocalRepo() that returns *git.Repository.
- Consider avoiding go-git types in the interface (accepting a repo couples consumers); you can keep the helper on the concrete type.
📜 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)
pkg/git/git.go(2 hunks)pkg/pro/api_client.go(7 hunks)pkg/utils/slice_test.go(2 hunks)tests/fixtures/scenarios/atmos-pro/atmos.yaml(1 hunks)tests/fixtures/scenarios/atmos-pro/stacks/deploy/nonprod.yaml(1 hunks)tests/fixtures/scenarios/atmos-pro/stacks/deploy/prod.yaml(1 hunks)tests/fixtures/scenarios/atmos-pro/stacks/mixins/atmos-pro.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- tests/fixtures/scenarios/atmos-pro/stacks/mixins/atmos-pro.yaml
- tests/fixtures/scenarios/atmos-pro/stacks/deploy/prod.yaml
- tests/fixtures/scenarios/atmos-pro/stacks/deploy/nonprod.yaml
- tests/fixtures/scenarios/atmos-pro/atmos.yaml
🧰 Additional context used
📓 Path-based instructions (4)
pkg/**/*.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
Place business logic in pkg rather than in cmd
Group related functionality in pkg/ subpackages by domain
Files:
pkg/utils/slice_test.gopkg/git/git.gopkg/pro/api_client.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: Test file naming symmetry: test files mirror implementation structure with _test.go suffix
Unit tests should focus on pure functions and use table-driven tests
ALWAYS use t.Skipf() instead of t.Skip() and provide a clear reason for skipped tests
NEVER use t.Skipf() without a reason
Files:
pkg/utils/slice_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 comments must end with periods - comments should be complete sentences (enforced by golangci-lint's godot linter)
All errors MUST be wrapped using static errors defined in errors/errors.go; never use dynamic errors directly
Always wrap errors with the static error first and add context-specific details after the static error; use %w verb to preserve error chain
ALWAYS use viper.BindEnv() for environment variable binding, and EVERY env var MUST have an ATMOS_ alternative
Distinguish between logging and UI output: only use logging for system/debug info, and use stderr/UI for prompts, progress, errors, and data for piping should go to stdout
Most text UI MUST go to stderr; only data/results go to stdout for proper piping; use utils.PrintfMessageToTUI() for UI messages
Atmos MUST work on Linux, macOS, and Windows—write portable implementations and prefer SDKs over calling binaries when available
ALWAYS compile after making changes; verify no compilation errors; run tests after successful compile; fix compilation errors immediately before proceeding
Write a test to reproduce the bug before fixing, run the test and confirm failure, fix the bug iteratively, and verify fix doesn't break existing tests
Files:
pkg/utils/slice_test.gopkg/git/git.gopkg/pro/api_client.go
**/!(*_test).go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
Document all exported functions, types, and methods with Go doc comments
Files:
pkg/git/git.gopkg/pro/api_client.go
🧠 Learnings (18)
📚 Learning: 2025-09-23T02:30:42.353Z
Learnt from: CR
PR: cloudposse/atmos#0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-09-23T02:30:42.353Z
Learning: Applies to **/*_test.go : Test both happy paths and error conditions
Applied to files:
pkg/utils/slice_test.go
📚 Learning: 2025-09-23T03:44:59.100Z
Learnt from: CR
PR: cloudposse/atmos#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-23T03:44:59.100Z
Learning: Applies to tests/**/*.go : Shared test utilities should be placed in the tests/ directory for integration tests
Applied to files:
pkg/utils/slice_test.go
📚 Learning: 2025-09-23T02:30:42.353Z
Learnt from: CR
PR: cloudposse/atmos#0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-09-23T02:30:42.353Z
Learning: Applies to **/*_test.go : Use table-driven tests for multiple scenarios
Applied to files:
pkg/utils/slice_test.go
📚 Learning: 2025-09-23T03:44:59.100Z
Learnt from: CR
PR: cloudposse/atmos#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-23T03:44:59.100Z
Learning: Applies to **/*_test.go : Unit tests should focus on pure functions and use table-driven tests
Applied to files:
pkg/utils/slice_test.go
📚 Learning: 2025-09-23T02:30:42.353Z
Learnt from: CR
PR: cloudposse/atmos#0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-09-23T02:30:42.353Z
Learning: Applies to **/*_test.go : Every new feature must include comprehensive unit tests
Applied to files:
pkg/utils/slice_test.go
📚 Learning: 2025-09-23T03:44:59.100Z
Learnt from: CR
PR: cloudposse/atmos#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-23T03:44:59.100Z
Learning: Applies to **/*.go : All comments must end with periods - comments should be complete sentences (enforced by golangci-lint's godot linter)
Applied to files:
pkg/pro/api_client.go
📚 Learning: 2025-09-23T03:44:59.100Z
Learnt from: CR
PR: cloudposse/atmos#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-23T03:44:59.100Z
Learning: Applies to **/*.go : All errors MUST be wrapped using static errors defined in errors/errors.go; never use dynamic errors directly
Applied to files:
pkg/pro/api_client.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:
pkg/pro/api_client.go
📚 Learning: 2025-09-23T03:44:59.100Z
Learnt from: CR
PR: cloudposse/atmos#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-23T03:44:59.100Z
Learning: Applies to errors/errors.go : Define all static errors in errors/errors.go and use these for wrapping errors
Applied to files:
pkg/pro/api_client.go
📚 Learning: 2024-12-02T21:26:32.337Z
Learnt from: osterman
PR: cloudposse/atmos#808
File: pkg/config/config.go:478-483
Timestamp: 2024-12-02T21:26:32.337Z
Learning: In the 'atmos' project, when reviewing Go code like `pkg/config/config.go`, avoid suggesting file size checks after downloading remote configs if such checks aren't implemented elsewhere in the codebase.
Applied to files:
pkg/pro/api_client.go
📚 Learning: 2024-11-13T21:37:07.852Z
Learnt from: Cerebrovinny
PR: cloudposse/atmos#764
File: internal/exec/describe_stacks.go:289-295
Timestamp: 2024-11-13T21:37:07.852Z
Learning: In the `internal/exec/describe_stacks.go` file of the `atmos` project written in Go, avoid extracting the stack name handling logic into a helper function within the `ExecuteDescribeStacks` method, even if the logic appears duplicated.
Applied to files:
pkg/pro/api_client.go
📚 Learning: 2025-09-23T03:44:59.100Z
Learnt from: CR
PR: cloudposse/atmos#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-23T03:44:59.100Z
Learning: Applies to **/*.go : Always wrap errors with the static error first and add context-specific details after the static error; use %w verb to preserve error chain
Applied to files:
pkg/pro/api_client.go
📚 Learning: 2025-09-10T22:38:42.212Z
Learnt from: Benbentwo
PR: cloudposse/atmos#1475
File: pkg/auth/identities/aws/user.go:141-145
Timestamp: 2025-09-10T22:38:42.212Z
Learning: The user confirmed that the errors package has an error string wrapping format, contradicting the previous learning about ErrWrappingFormat being invalid. The current usage of fmt.Errorf(errUtils.ErrWrappingFormat, errUtils.ErrAuthAwsFileManagerFailed, err) appears to be the correct pattern.
Applied to files:
pkg/pro/api_client.go
📚 Learning: 2025-04-04T02:03:23.676Z
Learnt from: aknysh
PR: cloudposse/atmos#1185
File: internal/exec/yaml_func_store.go:26-26
Timestamp: 2025-04-04T02:03:23.676Z
Learning: The Atmos codebase currently uses `log.Fatal` for error handling in multiple places. The maintainers are aware this isn't an ideal pattern (should only be used in main() or init() functions) and plan to address it comprehensively in a separate PR. CodeRabbit should not flag these issues or push for immediate changes until that refactoring is complete.
Applied to files:
pkg/pro/api_client.go
📚 Learning: 2025-09-13T18:06:07.674Z
Learnt from: samtholiya
PR: cloudposse/atmos#1466
File: toolchain/list.go:39-42
Timestamp: 2025-09-13T18:06:07.674Z
Learning: In the cloudposse/atmos repository, for UI messages in the toolchain package, use utils.PrintfMessageToTUI instead of log.Error or fmt.Fprintln(os.Stderr, ...). Import pkg/utils with alias "u" to follow the established pattern.
Applied to files:
pkg/pro/api_client.go
📚 Learning: 2025-09-10T22:38:42.212Z
Learnt from: Benbentwo
PR: cloudposse/atmos#1475
File: pkg/auth/identities/aws/user.go:141-145
Timestamp: 2025-09-10T22:38:42.212Z
Learning: ErrWrappingFormat is correctly defined as "%w: %w" in the errors package and is used throughout the codebase to wrap two error types together. The usage fmt.Errorf(errUtils.ErrWrappingFormat, errUtils.ErrAuthAwsFileManagerFailed, err) is the correct pattern when both arguments are error types.
Applied to files:
pkg/pro/api_client.go
📚 Learning: 2025-06-02T14:12:02.710Z
Learnt from: milldr
PR: cloudposse/atmos#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:
pkg/pro/api_client.go
📚 Learning: 2025-06-02T14:12:02.710Z
Learnt from: milldr
PR: cloudposse/atmos#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:
pkg/pro/api_client.go
🧬 Code graph analysis (2)
pkg/utils/slice_test.go (1)
pkg/utils/slice_utils.go (7)
SliceOfInterfacesToSliceOfStrings(62-70)SliceRemoveString(95-105)SliceRemoveFlag(110-135)SliceRemoveFlagAndValue(139-159)ErrNilInput(11-11)ErrNonStringElement(12-12)SliceOfInterfacesToSliceOfStringsWithTypeAssertion(74-89)
pkg/pro/api_client.go (10)
pkg/pro/dtos/atmos_api_response.go (1)
AtmosApiResponse(3-10)pkg/pro/dtos/instances.go (2)
InstancesUploadRequest(9-15)InstanceStatusUploadRequest(18-28)errors/errors.go (20)
ErrWrappingFormat(8-8)ErrFailedToGetGitHubOIDCToken(102-102)ErrStringWrappingFormat(9-9)ErrOIDCWorkspaceIDRequired(94-94)ErrOIDCTokenExchangeFailed(95-95)ErrFailedToCreateRequest(86-86)ErrFailedToMarshalPayload(87-87)ErrFailedToCreateAuthRequest(88-88)ErrFailedToMakeRequest(89-89)ErrFailedToUploadStacks(90-90)ErrFailedToReadResponseBody(91-91)ErrFailedToUnmarshalAPIResponse(105-105)ErrFailedToLockStack(92-92)ErrFailedToUnlockStack(93-93)ErrAPIResponseError(107-107)ErrNotInGitHubActions(97-97)ErrFailedToGetOIDCToken(98-98)ErrFailedToDecodeOIDCResponse(99-99)ErrFailedToExchangeOIDCToken(100-100)ErrFailedToDecodeTokenResponse(101-101)pkg/config/const.go (1)
AtmosProWorkspaceIDEnvVarName(107-107)pkg/utils/json_utils.go (1)
ConvertToJSON(101-114)pkg/schema/pro.go (1)
StackLockActionParams(19-26)pkg/pro/dtos/lock_stack.go (1)
LockStackResponse(14-26)pkg/pro/dtos/unlock_stack.go (1)
UnlockStackResponse(7-10)pkg/pro/dtos/get_github_oidc_token.go (1)
GetGitHubOIDCResponse(4-6)pkg/pro/dtos/exchange_github_oidc_token.go (1)
ExchangeGitHubOIDCTokenResponse(10-15)
⏰ 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). (3)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Run pre-commit hooks
- GitHub Check: Lint (golangci)
🔇 Additional comments (18)
pkg/pro/api_client.go (16)
8-8: Import added for net package.Good addition to support the enhanced HTTP client configuration with timeout controls.
14-14: Proper error utilities import.Correctly imports the centralized error utilities package with alias for consistent error wrapping patterns.
22-35: Well-structured timeout constants and log keys.The timeout constants provide sensible defaults for HTTP operations, and the log key constants will help maintain consistency across logging calls.
37-71: Centralized logging helpers look solid.The
logProAPIResponseandlogAndReturnProAPIErrorfunctions provide consistent structured logging for Pro API operations. The error message formatting with trace ID is particularly useful for debugging.
73-77: Interface definition follows Go conventions.The
AtmosProAPIClientInterfaceprovides a clean contract for the upload operations. Using pointer parameters is appropriate for the struct sizes involved.
88-95: Constructor properly initializes HTTP client.The updated constructor correctly creates an HTTP client with the default timeout, removing the logger dependency as intended.
98-139: Environment-based constructor handles authentication flows well.The function properly handles both API token and OIDC authentication paths with appropriate error wrapping using the centralized error utilities.
141-151: Request creation helper is straightforward.The
getAuthenticatedRequestfunction correctly sets up authenticated requests with proper error wrapping.
154-182: Upload method uses consistent patterns.The
UploadAffectedStacksmethod follows the established error handling and logging patterns. The use ofhandleAPIResponsecentralizes response processing.
184-229: Stack lock action helper reduces duplication effectively.The
doStackLockActionhelper successfully eliminates the code duplication between lock/unlock operations while maintaining type safety through interface{} and type assertions.
231-250: Lock stack method properly uses the helper.Clean implementation that delegates to the shared helper with appropriate parameters.
252-271: Unlock stack method mirrors lock pattern.Consistent with the lock implementation, using the same helper pattern.
273-306: Response handler provides robust error handling.The
handleAPIResponsefunction handles both successful and error cases appropriately, with fallback logic for unparseable responses and proper error message formatting.
308-352: OIDC token retrieval updated with centralized client.The function now uses the
getHTTPClientWithTimeouthelper for consistent timeout behavior and proper error wrapping.
354-367: HTTP client helper provides comprehensive timeout configuration.The
getHTTPClientWithTimeoutfunction sets up reasonable timeouts across all phases of HTTP operations (dial, idle, response headers, etc.).
369-420: Token exchange function enhanced with better logging.The OIDC token exchange now includes structured logging of API responses and uses the centralized HTTP client helper for consistent timeout behavior.
pkg/utils/slice_test.go (1)
4-4: LGTM on fmt import.
Used for precise error message assertions.pkg/git/git.go (1)
116-118: Concrete type looks good.Zero-field struct for stateless implementation is appropriate.
|
💥 This pull request now has conflicts. Could you fix it @milldr? 🙏 |
…into feat/atmos-list-upload
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
errors/errors.go (1)
58-71: Tighten git error messages for clarity.Suggest small wording tweaks to reduce ambiguity.
- ErrInvalidGitPort = errors.New("invalid port number") + ErrInvalidGitPort = errors.New("invalid git port number") @@ - ErrLocalRepoFetch = errors.New("local repo unavailable") + ErrLocalRepoFetch = errors.New("failed to fetch from local repository") @@ - ErrHeadLookup = errors.New("HEAD not found") + ErrHeadLookup = errors.New("git HEAD not found")pkg/utils/slice_utils.go (2)
67-84: Wrap ErrNilInput with context (align with error policy).Return a wrapped sentinel (not bare) and include function context in both error paths.
Apply this diff:
func SliceOfInterfacesToSliceOfStringsWithTypeAssertion(input []any) ([]string, error) { if input == nil { - return nil, errUtils.ErrNilInput + return nil, fmt.Errorf("%w: utils.SliceOfInterfacesToSliceOfStringsWithTypeAssertion", errUtils.ErrNilInput) } @@ - if !ok { - return nil, fmt.Errorf("%w: index=%d, got=%T", errUtils.ErrNonStringElement, i, current) - } + if !ok { + return nil, fmt.Errorf("%w: utils.SliceOfInterfacesToSliceOfStringsWithTypeAssertion: index=%d, got=%T", errUtils.ErrNonStringElement, i, current) + }
132-154: Optional: respect “--” end-of-options sentinel.Common CLI behavior stops flag parsing after “--”.
Apply this diff:
for i := 0; i < len(args); i++ { arg := args[i] + if arg == "--" { + // Preserve all remaining args verbatim after end-of-options. + out = append(out, args[i:]...) + break + } if arg == "--"+flagName {pkg/utils/slice_test.go (2)
157-257: Deduplicate repeated test case.“remove flag with spaced value followed by another flag” appears twice.
Apply this diff to remove the duplicate:
@@ - { - name: "remove flag with spaced value followed by another flag", - input: []string{"--flag1", "value1", "--flag2", "value2"}, - flagName: "flag1", - expected: []string{"--flag2", "value2"}, - },
259-404: Adjust nil-input assertion to tolerate wrapped sentinel.If you wrap ErrNilInput per policy, assert via ErrorIs.
Apply this diff:
@@ switch tc.errorType { case errUtils.ErrNilInput: - assert.Equal(t, errUtils.ErrNilInput, err) + assert.ErrorIs(t, err, errUtils.ErrNilInput) case errUtils.ErrNonStringElement: assertNonStringElementError(t, tc.input, err) }pkg/git/git.go (2)
121-132: Align method name with interface rename to avoid confusion.Mirror the interface change and keep behavior intact.
Apply:
-// GetLocalRepo returns information about the local git repository. -func (d *DefaultGitRepo) GetLocalRepo() (*RepoInfo, error) { +// GetLocalRepoInfo returns information about the local git repository. +func (d *DefaultGitRepo) GetLocalRepoInfo() (*RepoInfo, error) { repo, err := GetLocalRepo() if err != nil { return nil, fmt.Errorf("%w: failed to get local repository: %v", errUtils.ErrFailedToGetLocalRepo, err) } info, err := GetRepoInfo(repo) if err != nil { return nil, fmt.Errorf("%w: failed to get repository info: %v", errUtils.ErrFailedToGetRepoInfo, err) } return &info, nil }
150-163: Wrap with static sentinels and context (don’t return bare sentinels).Return contextual errors to conform with the repo’s error policy and keep consistency with other methods.
Apply:
func (d *DefaultGitRepo) GetCurrentCommitSHA() (string, error) { repo, err := GetLocalRepo() if err != nil { - return "", errUtils.ErrLocalRepoFetch + return "", fmt.Errorf("%w: failed to get local repository: %v", errUtils.ErrFailedToGetLocalRepo, err) } ref, err := repo.Head() if err != nil { - return "", errUtils.ErrHeadLookup + return "", fmt.Errorf("%w: failed to get HEAD reference: %v", errUtils.ErrHeadLookup, err) } return ref.Hash().String(), nil }
📜 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 (4)
errors/errors.go(2 hunks)pkg/git/git.go(2 hunks)pkg/utils/slice_test.go(2 hunks)pkg/utils/slice_utils.go(6 hunks)
🧰 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
Group related functionality in pkg/ subpackages by domain
Files:
pkg/utils/slice_utils.gopkg/utils/slice_test.gopkg/git/git.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 comments must end with periods - comments should be complete sentences (enforced by golangci-lint's godot linter)
All errors MUST be wrapped using static errors defined in errors/errors.go; never use dynamic errors directly
Always wrap errors with the static error first and add context-specific details after the static error; use %w verb to preserve error chain
ALWAYS use viper.BindEnv() for environment variable binding, and EVERY env var MUST have an ATMOS_ alternative
Distinguish between logging and UI output: only use logging for system/debug info, and use stderr/UI for prompts, progress, errors, and data for piping should go to stdout
Most text UI MUST go to stderr; only data/results go to stdout for proper piping; use utils.PrintfMessageToTUI() for UI messages
Atmos MUST work on Linux, macOS, and Windows—write portable implementations and prefer SDKs over calling binaries when available
ALWAYS compile after making changes; verify no compilation errors; run tests after successful compile; fix compilation errors immediately before proceeding
Write a test to reproduce the bug before fixing, run the test and confirm failure, fix the bug iteratively, and verify fix doesn't break existing tests
Files:
pkg/utils/slice_utils.gopkg/utils/slice_test.goerrors/errors.gopkg/git/git.go
**/!(*_test).go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
Document all exported functions, types, and methods with Go doc comments
Files:
pkg/utils/slice_utils.goerrors/errors.gopkg/git/git.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: Test file naming symmetry: test files mirror implementation structure with _test.go suffix
Unit tests should focus on pure functions and use table-driven tests
ALWAYS use t.Skipf() instead of t.Skip() and provide a clear reason for skipped tests
NEVER use t.Skipf() without a reason
Files:
pkg/utils/slice_test.go
errors/errors.go
📄 CodeRabbit inference engine (CLAUDE.md)
Define all static errors in errors/errors.go and use these for wrapping errors
Files:
errors/errors.go
🧠 Learnings (9)
📚 Learning: 2025-09-23T03:44:59.100Z
Learnt from: CR
PR: cloudposse/atmos#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-23T03:44:59.100Z
Learning: Applies to **/*.go : All comments must end with periods - comments should be complete sentences (enforced by golangci-lint's godot linter)
Applied to files:
pkg/utils/slice_utils.goerrors/errors.go
📚 Learning: 2025-09-23T02:30:42.353Z
Learnt from: CR
PR: cloudposse/atmos#0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-09-23T02:30:42.353Z
Learning: Applies to **/*_test.go : Test both happy paths and error conditions
Applied to files:
pkg/utils/slice_test.go
📚 Learning: 2025-09-23T02:30:42.353Z
Learnt from: CR
PR: cloudposse/atmos#0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-09-23T02:30:42.353Z
Learning: Applies to cmd/**/*.go : Provide clear error messages to users and troubleshooting hints where appropriate
Applied to files:
errors/errors.go
📚 Learning: 2025-09-23T02:30:42.353Z
Learnt from: CR
PR: cloudposse/atmos#0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-09-23T02:30:42.353Z
Learning: Applies to **/*.go : Follow Go error handling idioms and use meaningful error messages
Applied to files:
errors/errors.go
📚 Learning: 2025-09-23T02:30:42.353Z
Learnt from: CR
PR: cloudposse/atmos#0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-09-23T02:30:42.353Z
Learning: Applies to cmd/**/*.go : Use kebab-case for command-line flags
Applied to files:
errors/errors.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:
errors/errors.go
📚 Learning: 2025-09-23T03:44:59.100Z
Learnt from: CR
PR: cloudposse/atmos#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-23T03:44:59.100Z
Learning: Applies to **/*.go : Always wrap errors with the static error first and add context-specific details after the static error; use %w verb to preserve error chain
Applied to files:
pkg/git/git.go
📚 Learning: 2025-09-23T02:30:42.353Z
Learnt from: CR
PR: cloudposse/atmos#0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-09-23T02:30:42.353Z
Learning: Applies to **/*.go : Wrap errors with context using fmt.Errorf("context: %w", err)
Applied to files:
pkg/git/git.go
📚 Learning: 2024-12-05T02:48:53.818Z
Learnt from: aknysh
PR: cloudposse/atmos#809
File: cmd/cmd_utils.go:470-477
Timestamp: 2024-12-05T02:48:53.818Z
Learning: The function `GetLatestGitHubRepoRelease` in the Go codebase already uses a context with a timeout, so wrapping it with an additional context is unnecessary.
Applied to files:
pkg/git/git.go
🧬 Code graph analysis (3)
pkg/utils/slice_utils.go (1)
errors/errors.go (2)
ErrNilInput(73-73)ErrNonStringElement(74-74)
pkg/utils/slice_test.go (2)
pkg/utils/slice_utils.go (5)
SliceOfInterfacesToSliceOfStrings(57-65)SliceRemoveString(90-100)SliceRemoveFlag(105-130)SliceRemoveFlagAndValue(134-154)SliceOfInterfacesToSliceOfStringsWithTypeAssertion(69-84)errors/errors.go (2)
ErrNilInput(73-73)ErrNonStringElement(74-74)
pkg/git/git.go (1)
errors/errors.go (4)
ErrFailedToGetLocalRepo(67-67)ErrFailedToGetRepoInfo(68-68)ErrLocalRepoFetch(69-69)ErrHeadLookup(70-70)
⏰ 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). (8)
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Lint (golangci)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Build (macos-latest, macos)
- GitHub Check: Analyze (go)
- GitHub Check: Lint (golangci)
- GitHub Check: Run pre-commit hooks
🔇 Additional comments (19)
errors/errors.go (5)
72-75: LGTM on slice utility errors.Names and messages are clear and actionable.
79-79: LGTM: improved stack error guidance.Helpful, user-facing guidance in the message.
116-125: LGTM on exec error additions.Messages are clear and consistent.
126-131: LGTM on list error additions.Terminology matches the new “list instances” UX.
92-115: No alias needed for ErrFailedToMarshalRequestBody; keep both upload errors.
- ErrFailedToMarshalRequestBody: no references found in the codebase — no deprecated alias required.
- ErrFailedToUploadStacks vs ErrFailedToUploadInstances: both are actively used (pkg/pro/api_client.go, pkg/pro/api_client_upload_affected_stacks_test.go, pkg/pro/api_client_instances.go, pkg/pro/api_client_instances_test.go, pkg/list/list_instances.go) — keep both constants; add deprecated aliases only if you intentionally unify names later.
pkg/utils/slice_utils.go (9)
7-8: Good: centralized errors import.Using the shared errors package aligns with policy.
10-18: LGTM: doc + implementation.
20-28: LGTM: doc + implementation.
30-38: LGTM: doc + implementation.
40-48: LGTM: doc + implementation.
50-54: LGTM: doc + implementation.
55-65: LGTM: clarified behavior and formatting.
86-101: LGTM: first-occurrence removal with in-place compaction.Zeroing last element prevents retention; good callout in comment.
102-131: LGTM: non-mutating flag removal handles both forms.pkg/utils/slice_test.go (3)
18-23: LGTM: updated callsite for no-error variant.
25-71: LGTM: solid table-driven coverage.
73-155: LGTM: comprehensive scenarios for both flag forms.pkg/git/git.go (2)
4-11: LGTM on imports.Using the errUtils alias and fmt for contextual wrapping aligns with the repo’s error policy.
134-148: Good contextual wrapping.Delegation plus path context on error preserves the sentinel and aids diagnostics.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
pkg/git/git.go (1)
111-116: Add Go doc comments for exported functions in this file.golangci-lint requires doc comments for exported symbols. Add for GetLocalRepo, GetRepoConfig, GetRepoInfo, and OpenWorktreeAwareRepo.
Example (apply above each function):
// GetLocalRepo opens the local Git repository in the current working directory with worktree support.// GetRepoConfig returns the repository configuration and removes deprecated options (e.g., 'untrackedCache') if present.// GetRepoInfo extracts local and remote metadata from a Git repository.// OpenWorktreeAwareRepo opens a Git repository at the given path with worktree support enabled.internal/exec/pro_test.go (1)
196-201: Assert DTO fields to validate drift and metadata.The test defines expectedDrift but doesn’t assert it or other payload fields.
Apply:
- // Set up mock expectations for pro client - mockProClient.On("UploadInstanceStatus", mock.AnythingOfType("*dtos.InstanceStatusUploadRequest")).Return(nil) + // Set up mock expectations for pro client with payload validation. + mockProClient. + On("UploadInstanceStatus", mock.AnythingOfType("*dtos.InstanceStatusUploadRequest")). + Run(func(args mock.Arguments) { + dto := args.Get(0).(*dtos.InstanceStatusUploadRequest) + assert.Equal(t, tc.expectedDrift, dto.HasDrift) + assert.Equal(t, testRepoInfo.RepoUrl, dto.RepoURL) + assert.Equal(t, testRepoInfo.RepoName, dto.RepoName) + assert.Equal(t, testRepoInfo.RepoOwner, dto.RepoOwner) + assert.Equal(t, testRepoInfo.RepoHost, dto.RepoHost) + assert.Equal(t, info.Stack, dto.Stack) + assert.Equal(t, info.Component, dto.Component) + }). + Return(nil)Also applies to: 206-216
internal/exec/pro.go (2)
227-230: Use structured logging for errors.Leverage key-value logging instead of fmt.Sprintf.
- log.Warn(fmt.Sprintf("Failed to get current git SHA: %v", err)) + log.Warn("Failed to get current git SHA.", "error", err)
172-172: Terminate exported comment with a period.Satisfy godot linter.
-// ExecuteProUnlockCommand executes `atmos pro unlock` command +// ExecuteProUnlockCommand executes `atmos pro unlock` command.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
internal/exec/pro.go(7 hunks)internal/exec/pro_test.go(1 hunks)pkg/git/git.go(2 hunks)
🧰 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
Group related functionality in pkg/ subpackages by domain
Files:
pkg/git/git.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 comments must end with periods - comments should be complete sentences (enforced by golangci-lint's godot linter)
All errors MUST be wrapped using static errors defined in errors/errors.go; never use dynamic errors directly
Always wrap errors with the static error first and add context-specific details after the static error; use %w verb to preserve error chain
ALWAYS use viper.BindEnv() for environment variable binding, and EVERY env var MUST have an ATMOS_ alternative
Distinguish between logging and UI output: only use logging for system/debug info, and use stderr/UI for prompts, progress, errors, and data for piping should go to stdout
Most text UI MUST go to stderr; only data/results go to stdout for proper piping; use utils.PrintfMessageToTUI() for UI messages
Atmos MUST work on Linux, macOS, and Windows—write portable implementations and prefer SDKs over calling binaries when available
ALWAYS compile after making changes; verify no compilation errors; run tests after successful compile; fix compilation errors immediately before proceeding
Write a test to reproduce the bug before fixing, run the test and confirm failure, fix the bug iteratively, and verify fix doesn't break existing tests
Files:
pkg/git/git.gointernal/exec/pro.gointernal/exec/pro_test.go
**/!(*_test).go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
Document all exported functions, types, and methods with Go doc comments
Files:
pkg/git/git.gointernal/exec/pro.go
internal/exec/**.go
📄 CodeRabbit inference engine (CLAUDE.md)
Prefer extending existing utilities in internal/exec/ and pkg/ over creating new ones—search for existing methods before implementation
Files:
internal/exec/pro.gointernal/exec/pro_test.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: Test file naming symmetry: test files mirror implementation structure with _test.go suffix
Unit tests should focus on pure functions and use table-driven tests
ALWAYS use t.Skipf() instead of t.Skip() and provide a clear reason for skipped tests
NEVER use t.Skipf() without a reason
Files:
internal/exec/pro_test.go
🧠 Learnings (31)
📚 Learning: 2025-09-23T03:44:59.100Z
Learnt from: CR
PR: cloudposse/atmos#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-23T03:44:59.100Z
Learning: Applies to **/*.go : Always wrap errors with the static error first and add context-specific details after the static error; use %w verb to preserve error chain
Applied to files:
pkg/git/git.go
📚 Learning: 2024-12-05T02:48:53.818Z
Learnt from: aknysh
PR: cloudposse/atmos#809
File: cmd/cmd_utils.go:470-477
Timestamp: 2024-12-05T02:48:53.818Z
Learning: The function `GetLatestGitHubRepoRelease` in the Go codebase already uses a context with a timeout, so wrapping it with an additional context is unnecessary.
Applied to files:
pkg/git/git.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/pro.go
📚 Learning: 2024-12-07T16:16:13.038Z
Learnt from: Listener430
PR: cloudposse/atmos#825
File: internal/exec/helmfile_generate_varfile.go:28-31
Timestamp: 2024-12-07T16:16:13.038Z
Learning: In `internal/exec/helmfile_generate_varfile.go`, the `--help` command (`./atmos helmfile generate varfile --help`) works correctly without requiring stack configurations, and the only change needed was to make `ProcessCommandLineArgs` exportable by capitalizing its name.
Applied to files:
internal/exec/pro.go
📚 Learning: 2025-09-23T03:44:59.100Z
Learnt from: CR
PR: cloudposse/atmos#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-23T03:44:59.100Z
Learning: Applies to **/*.go : All comments must end with periods - comments should be complete sentences (enforced by golangci-lint's godot linter)
Applied to files:
internal/exec/pro.go
📚 Learning: 2025-04-23T15:02:50.246Z
Learnt from: osterman
PR: cloudposse/atmos#1202
File: pkg/utils/yaml_func_exec.go:104-104
Timestamp: 2025-04-23T15:02:50.246Z
Learning: In the Atmos codebase, direct calls to `os.Getenv` should be avoided. Instead, use `viper.BindEnv` for environment variable access. This provides a consistent approach to configuration management across the codebase.
Applied to files:
internal/exec/pro.go
📚 Learning: 2025-09-23T03:44:59.100Z
Learnt from: CR
PR: cloudposse/atmos#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-23T03:44:59.100Z
Learning: Applies to **/*.go : ALWAYS use viper.BindEnv() for environment variable binding, and EVERY env var MUST have an ATMOS_ alternative
Applied to files:
internal/exec/pro.go
📚 Learning: 2025-08-29T20:57:35.423Z
Learnt from: osterman
PR: cloudposse/atmos#1433
File: cmd/theme_list.go:33-36
Timestamp: 2025-08-29T20:57:35.423Z
Learning: In the Atmos codebase, avoid using viper.SetEnvPrefix("ATMOS") with viper.AutomaticEnv() because canonical environment variable names are not exclusive to Atmos and could cause conflicts. Instead, use selective environment variable binding through the setEnv function in pkg/config/load.go with bindEnv(v, "config.key", "ENV_VAR_NAME") for specific environment variables.
Applied to files:
internal/exec/pro.go
📚 Learning: 2025-09-23T04:43:31.840Z
Learnt from: osterman
PR: cloudposse/atmos#1430
File: pkg/config/config.go:98-107
Timestamp: 2025-09-23T04:43:31.840Z
Learning: In the Atmos codebase, NO_PAGER environment variable handling is intentionally kept as direct os.Getenv() access in pkg/config/config.go rather than using Viper binding, because adding no_pager to the config file would be confusing for users. This is an acknowledged exception to the normal Viper binding pattern for environment variables.
Applied to files:
internal/exec/pro.go
📚 Learning: 2025-09-23T02:30:42.353Z
Learnt from: CR
PR: cloudposse/atmos#0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-09-23T02:30:42.353Z
Learning: Applies to cmd/**/*.go : Use Viper for managing configuration, environment variables, and flags in the CLI
Applied to files:
internal/exec/pro.go
📚 Learning: 2025-09-23T02:30:42.353Z
Learnt from: CR
PR: cloudposse/atmos#0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-09-23T02:30:42.353Z
Learning: Applies to cmd/**/*.go : Use Viper for configuration management; support files, env vars, and flags with precedence flags > env > config > defaults
Applied to files:
internal/exec/pro.go
📚 Learning: 2025-09-23T02:30:42.353Z
Learnt from: CR
PR: cloudposse/atmos#0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-09-23T02:30:42.353Z
Learning: Applies to **/*.go : Use snake_case for environment variables
Applied to files:
internal/exec/pro.go
📚 Learning: 2025-04-10T20:48:22.687Z
Learnt from: samtholiya
PR: cloudposse/atmos#1147
File: pkg/config/load.go:0-0
Timestamp: 2025-04-10T20:48:22.687Z
Learning: In the `bindEnv` function in `pkg/config/load.go`, panic is used deliberately instead of returning errors because errors from `BindEnv` would only occur due to developer mistakes. Using panic helps with early detection of these developer errors during initialization.
Applied to files:
internal/exec/pro.go
📚 Learning: 2025-02-20T18:33:16.567Z
Learnt from: samtholiya
PR: cloudposse/atmos#1077
File: pkg/downloader/custom_github_detector.go:47-68
Timestamp: 2025-02-20T18:33:16.567Z
Learning: Go doesn't have a built-in secure string type. For sensitive data like tokens, using environment variables (which are protected by the OS) is a common and secure practice.
Applied to files:
internal/exec/pro.go
📚 Learning: 2025-01-07T20:38:09.618Z
Learnt from: samtholiya
PR: cloudposse/atmos#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/pro.go
📚 Learning: 2025-04-04T02:03:23.676Z
Learnt from: aknysh
PR: cloudposse/atmos#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/pro.go
📚 Learning: 2024-10-28T01:51:30.811Z
Learnt from: osterman
PR: cloudposse/atmos#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/pro.go
📚 Learning: 2025-09-23T02:30:42.353Z
Learnt from: CR
PR: cloudposse/atmos#0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-09-23T02:30:42.353Z
Learning: Applies to cmd/**/*.go : Provide clear error messages to users and troubleshooting hints where appropriate
Applied to files:
internal/exec/pro.go
📚 Learning: 2025-09-10T22:38:42.212Z
Learnt from: Benbentwo
PR: cloudposse/atmos#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/pro.go
📚 Learning: 2024-12-02T21:26:32.337Z
Learnt from: osterman
PR: cloudposse/atmos#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/pro.go
📚 Learning: 2024-10-20T13:12:46.499Z
Learnt from: haitham911
PR: cloudposse/atmos#736
File: pkg/config/const.go:6-6
Timestamp: 2024-10-20T13:12:46.499Z
Learning: In `cmd/cmd_utils.go`, it's acceptable to have hardcoded references to `atmos.yaml` in logs, and it's not necessary to update them to use the `CliConfigFileName` constant.
Applied to files:
internal/exec/pro.go
📚 Learning: 2025-09-10T22:38:42.212Z
Learnt from: Benbentwo
PR: cloudposse/atmos#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/pro.go
📚 Learning: 2025-01-30T19:30:59.120Z
Learnt from: samtholiya
PR: cloudposse/atmos#959
File: cmd/workflow.go:74-74
Timestamp: 2025-01-30T19:30:59.120Z
Learning: Error handling for `cmd.Usage()` is not required in the Atmos CLI codebase, as confirmed by the maintainer.
Applied to files:
internal/exec/pro.go
📚 Learning: 2025-09-13T18:06:07.674Z
Learnt from: samtholiya
PR: cloudposse/atmos#1466
File: toolchain/list.go:39-42
Timestamp: 2025-09-13T18:06:07.674Z
Learning: In the cloudposse/atmos repository, for UI messages in the toolchain package, use utils.PrintfMessageToTUI instead of log.Error or fmt.Fprintln(os.Stderr, ...). Import pkg/utils with alias "u" to follow the established pattern.
Applied to files:
internal/exec/pro.go
📚 Learning: 2025-09-23T03:44:59.100Z
Learnt from: CR
PR: cloudposse/atmos#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-23T03:44:59.100Z
Learning: Applies to **/*.go : Atmos MUST work on Linux, macOS, and Windows—write portable implementations and prefer SDKs over calling binaries when available
Applied to files:
internal/exec/pro.go
📚 Learning: 2025-07-05T20:59:02.914Z
Learnt from: aknysh
PR: cloudposse/atmos#1363
File: internal/exec/template_utils.go:18-18
Timestamp: 2025-07-05T20:59:02.914Z
Learning: In the Atmos project, gomplate v4 is imported with a blank import (`_ "github.com/hairyhenderson/gomplate/v4"`) alongside v3 imports to resolve AWS SDK version conflicts. V3 uses older AWS SDK versions that conflict with newer AWS modules used by Atmos. A full migration to v4 requires extensive refactoring due to API changes and should be handled in a separate PR.
Applied to files:
internal/exec/pro.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:
internal/exec/pro.go
📚 Learning: 2025-09-23T03:44:59.100Z
Learnt from: CR
PR: cloudposse/atmos#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-23T03:44:59.100Z
Learning: Applies to cmd/*.go : Co-locate tests with implementation (_test.go alongside .go files in cmd/)
Applied to files:
internal/exec/pro_test.go
📚 Learning: 2025-09-23T03:44:59.100Z
Learnt from: CR
PR: cloudposse/atmos#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-23T03:44:59.100Z
Learning: Applies to **/*_test.go : Unit tests should focus on pure functions and use table-driven tests
Applied to files:
internal/exec/pro_test.go
📚 Learning: 2025-09-23T02:30:42.353Z
Learnt from: CR
PR: cloudposse/atmos#0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-09-23T02:30:42.353Z
Learning: Applies to **/*_test.go : Use table-driven tests for multiple scenarios
Applied to files:
internal/exec/pro_test.go
📚 Learning: 2025-09-23T02:30:42.353Z
Learnt from: CR
PR: cloudposse/atmos#0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-09-23T02:30:42.353Z
Learning: Consider using testify/mock for creating mock implementations
Applied to files:
internal/exec/pro_test.go
🧬 Code graph analysis (3)
pkg/git/git.go (1)
errors/errors.go (4)
ErrFailedToGetLocalRepo(67-67)ErrFailedToGetRepoInfo(68-68)ErrLocalRepoFetch(69-69)ErrHeadLookup(70-70)
internal/exec/pro.go (7)
errors/errors.go (13)
ErrWrappingFormat(8-8)ErrFailedToProcessArgs(119-119)ErrFailedToInitConfig(120-120)ErrFailedToCreateLogger(121-121)ErrFailedToGetComponentFlag(122-122)ErrFailedToGetStackFlag(123-123)ErrComponentAndStackRequired(117-117)ErrFailedToGetLocalRepo(67-67)ErrFailedToGetRepoInfo(68-68)ErrFailedToCreateAPIClient(118-118)ErrFailedToLockStack(99-99)ErrFailedToUnlockStack(100-100)ErrFailedToUploadInstanceStatus(111-111)pkg/config/config.go (1)
InitCliConfig(25-62)pkg/logger/logger.go (2)
NewLoggerFromCliConfig(44-50)Logger(32-35)pkg/git/git.go (2)
GetRepoInfo(59-109)GitRepoInterface(112-116)pkg/pro/api_client.go (2)
NewAtmosProAPIClientFromEnv(98-139)AtmosProAPIClientInterface(74-77)pkg/schema/schema.go (1)
ConfigAndStacksInfo(434-511)pkg/pro/dtos/instances.go (1)
InstanceStatusUploadRequest(18-28)
internal/exec/pro_test.go (3)
pkg/pro/dtos/instances.go (2)
InstancesUploadRequest(9-15)InstanceStatusUploadRequest(18-28)pkg/git/git.go (2)
RepoInfo(49-57)GetRepoInfo(59-109)pkg/schema/schema.go (1)
ConfigAndStacksInfo(434-511)
⏰ 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). (5)
- GitHub Check: Build (macos-latest, macos)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Run pre-commit hooks
- GitHub Check: Analyze (go)
- GitHub Check: Lint (golangci)
🔇 Additional comments (3)
internal/exec/pro.go (2)
274-274: Lower log level to debug to reduce noise when Pro is disabled.Matches prior maintainer feedback.
- log.Warn("Pro is not enabled. Skipping upload of Terraform result.") + log.Debug("Pro is not enabled. Skipping upload of Terraform result.")
5-5: Remove direct os.Getenv for ATMOS_PRO_RUN_ID — inject or read from central configDirect env reads are forbidden. Preferred: add an atmosProRunID parameter to uploadStatus and pass it from callers; alternative: read it from the central viper-backed config (do not create a new viper instance).
Files to update:
- internal/exec/pro.go (remove os.Getenv usage; change uploadStatus signature)
- Call sites: internal/exec/terraform.go and internal/exec/pro_test.go (pass atmosProRunID)
- Note: pkg/config/const.go defines AtmosProRunIDEnvVarName.
Relevant diffs (unchanged from original suggestion):
-import ( - "fmt" - "os" +import ( + "fmt"-func uploadStatus(info *schema.ConfigAndStacksInfo, exitCode int, client pro.AtmosProAPIClientInterface, gitRepo git.GitRepoInterface) error { +func uploadStatus(info *schema.ConfigAndStacksInfo, exitCode int, client pro.AtmosProAPIClientInterface, gitRepo git.GitRepoInterface, atmosProRunID string) error {- // Get run ID from environment variables. - // Note: This is an exception to the general rule of using viper.BindEnv for environment variables. - // The run ID is always provided by the CI/CD environment and is not part of the stack configuration. - //nolint:forbidigo // Exception: Run ID is always from CI/CD environment, not config - atmosProRunID := os.Getenv("ATMOS_PRO_RUN_ID") + // atmosProRunID is provided by the caller (prefer ATMOS_PRO_RUN_ID; fallback to CI-specific ID upstream).Verified: ATMOS_PRO_RUN_ID is only read via os.Getenv in internal/exec/pro.go (pkg/config/const.go defines the env var). Update signature and tests accordingly.
pkg/git/git.go (1)
150-160: Don’t drop the underlying error.Return the sentinel wrapped with the cause.
func (d *DefaultGitRepo) GetCurrentCommitSHA() (string, error) { repo, err := GetLocalRepo() if err != nil { - return "", errUtils.ErrLocalRepoFetch + return "", fmt.Errorf(errUtils.ErrWrappingFormat, errUtils.ErrLocalRepoFetch, err) } ref, err := repo.Head() if err != nil { - return "", errUtils.ErrHeadLookup + return "", fmt.Errorf(errUtils.ErrWrappingFormat, errUtils.ErrHeadLookup, err) } return ref.Hash().String(), nil }
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
pkg/git/git.go (3)
134-148: Inconsistent error wrapping pattern.Line 145 uses a different error wrapping format (
%w: GetRepoInfo failed for repo %s: %w) instead of the standardized ErrWrappingFormat pattern used elsewhere in the file.Apply this diff to maintain consistency:
- return RepoInfo{}, fmt.Errorf("%w: GetRepoInfo failed for repo %s: %w", errUtils.ErrFailedToGetRepoInfo, repoPath, err) + cause := fmt.Errorf("GetRepoInfo failed for repo %s: %w", repoPath, err) + return RepoInfo{}, fmt.Errorf(errUtils.ErrWrappingFormat, errUtils.ErrFailedToGetRepoInfo, cause)
150-163: Error handling loses context information.The GetCurrentCommitSHA method returns static sentinel errors without preserving the underlying error details or adding context about what failed.
Apply this diff to preserve error context:
func (d *DefaultGitRepo) GetCurrentCommitSHA() (string, error) { repo, err := GetLocalRepo() if err != nil { - return "", errUtils.ErrLocalRepoFetch + return "", fmt.Errorf(errUtils.ErrWrappingFormat, errUtils.ErrLocalRepoFetch, fmt.Errorf("failed to get local repository: %w", err)) } ref, err := repo.Head() if err != nil { - return "", errUtils.ErrHeadLookup + return "", fmt.Errorf(errUtils.ErrWrappingFormat, errUtils.ErrHeadLookup, fmt.Errorf("failed to get HEAD reference: %w", err)) } return ref.Hash().String(), nil }
118-119: Add constructor function for consistency.Consider adding a NewDefaultGitRepo constructor function to follow Go conventions and make instantiation more explicit in consuming code.
Add this constructor:
// NewDefaultGitRepo creates a new instance of DefaultGitRepo. func NewDefaultGitRepo() GitRepoInterface { return &DefaultGitRepo{} }
📜 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 (1)
pkg/git/git.go(2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
pkg/**/*.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
Place business logic in pkg rather than in cmd
Group related functionality in pkg/ subpackages by domain
Files:
pkg/git/git.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 comments must end with periods - comments should be complete sentences (enforced by golangci-lint's godot linter)
All errors MUST be wrapped using static errors defined in errors/errors.go; never use dynamic errors directly
Always wrap errors with the static error first and add context-specific details after the static error; use %w verb to preserve error chain
ALWAYS use viper.BindEnv() for environment variable binding, and EVERY env var MUST have an ATMOS_ alternative
Distinguish between logging and UI output: only use logging for system/debug info, and use stderr/UI for prompts, progress, errors, and data for piping should go to stdout
Most text UI MUST go to stderr; only data/results go to stdout for proper piping; use utils.PrintfMessageToTUI() for UI messages
Atmos MUST work on Linux, macOS, and Windows—write portable implementations and prefer SDKs over calling binaries when available
ALWAYS compile after making changes; verify no compilation errors; run tests after successful compile; fix compilation errors immediately before proceeding
Write a test to reproduce the bug before fixing, run the test and confirm failure, fix the bug iteratively, and verify fix doesn't break existing tests
Files:
pkg/git/git.go
**/!(*_test).go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
Document all exported functions, types, and methods with Go doc comments
Files:
pkg/git/git.go
🧠 Learnings (3)
📚 Learning: 2025-09-23T03:44:59.100Z
Learnt from: CR
PR: cloudposse/atmos#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-23T03:44:59.100Z
Learning: Applies to **/*.go : Always wrap errors with the static error first and add context-specific details after the static error; use %w verb to preserve error chain
Applied to files:
pkg/git/git.go
📚 Learning: 2025-09-10T22:38:42.212Z
Learnt from: Benbentwo
PR: cloudposse/atmos#1475
File: pkg/auth/identities/aws/user.go:141-145
Timestamp: 2025-09-10T22:38:42.212Z
Learning: ErrWrappingFormat is correctly defined as "%w: %w" in the errors package and is used throughout the codebase to wrap two error types together. The usage fmt.Errorf(errUtils.ErrWrappingFormat, errUtils.ErrAuthAwsFileManagerFailed, err) is the correct pattern when both arguments are error types.
Applied to files:
pkg/git/git.go
📚 Learning: 2025-09-23T02:30:42.353Z
Learnt from: CR
PR: cloudposse/atmos#0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-09-23T02:30:42.353Z
Learning: Applies to **/*.go : Wrap errors with context using fmt.Errorf("context: %w", err)
Applied to files:
pkg/git/git.go
🧬 Code graph analysis (1)
pkg/git/git.go (1)
errors/errors.go (5)
ErrWrappingFormat(8-8)ErrFailedToGetLocalRepo(67-67)ErrFailedToGetRepoInfo(68-68)ErrLocalRepoFetch(69-69)ErrHeadLookup(70-70)
⏰ 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). (4)
- GitHub Check: Analyze (go)
- GitHub Check: Lint (golangci)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Run pre-commit hooks
🔇 Additional comments (2)
pkg/git/git.go (2)
111-116: LGTM on the interface design.The GitRepoInterface provides a clean abstraction for git operations needed by the Pro upload functionality. The method signatures are well-designed and the interface follows Go conventions.
121-132: Good error wrapping implementation.The error wrapping correctly uses the ErrWrappingFormat pattern and preserves the error chain. The implementation properly delegates to existing functions while adding the interface layer.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
what
New command for
atmos list instancesIncludes option for
--upload--upload, send the result to Atmos ProNew arg for
atmos terraform plan,--upload-statusOther changes:
This pull request introduces new functionality for uploading Terraform plan results and Atmos instances to the Pro API, improves error handling throughout the codebase, and refactors several areas for clarity and maintainability. The most notable changes are the addition of status upload logic for Terraform plans, new error definitions for Pro API interactions, and the creation of a new command for listing Atmos instances. Below are the key changes grouped by theme.
Pro API Integration and Terraform Plan Status Upload:
uploadStatusfunction and a flag (upload-status) for theplancommand incmd/terraform_commands.goandinternal/exec/pro.go. This enables reporting of plan status and drift detection to the Pro API. [1] [2]shouldUploadStatusfunction to determine when status should be uploaded, based on command type and component settings.Error Handling Improvements:
errors/errors.go, and refactored error wrapping to provide more context throughout the exec logic. [1] [2] [3] [4] [5] [6]Atmos Instances Listing Command:
list instancescommand incmd/list_instances.goto list Atmos instances and optionally upload them to the Pro API. This includes flag handling and integration with the existing list command structure.API Client Construction Refactoring:
NewAtmosProAPIClientFromEnvand related usage ininternal/exec/describe_affected.goandinternal/exec/pro.go. [1] [2] [3] [4]Minor Codebase Cleanups:
cmd/cmd_utils.go,cmd/terraform_commands.go, andinternal/exec/describe_affected.go. [1] [2] [3]These changes collectively improve the reliability, maintainability, and feature set of the Atmos CLI, especially around integration with the Pro API and error visibility.
why
We want to build drift detection into Atmos Pro. In order to do this, we need to upload an object similar to the describe affected object. This object needs specific component instances for specific stacks
With
atmos list instances --upload, we can find all component and stacks with drift detection and upload to Atmos ProWith
atmos terraform plan --upload-status, we can return the result of whether or not there is drift to APreferences
screenshots
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Tests