Conversation
- Add ErrInvalidListMergeStrategy static error constant - Improve Merge function with better error handling using static errors - Replace if-else chain with switch statement for cleaner code - Add test coverage for nil configuration handling - Update CLAUDE.md with compilation requirements - Maintain backward compatibility for nil config (defaults to replace strategy) 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
📝 WalkthroughWalkthroughAdds a mandatory compilation/testing note to documentation, introduces two exported error sentinels, changes Merge to return wrapped errors (including when config is nil) and to validate/list-strategy-normalize via switch, and expands/renames unit tests to cover nil config, invalid strategy, and empty inputs. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller
participant MergePkg as pkg/merge.Merge
participant Errors as errors (sentinels)
Caller->>MergePkg: Merge(inputs..., cfg)
alt cfg == nil
MergePkg-->>Caller: error (wrap Errors.ErrMerge + Errors.ErrAtmosConfigIsNil)
else
MergePkg->>MergePkg: normalize strategy (empty -> replace)
MergePkg->>MergePkg: validate strategy
alt strategy invalid
MergePkg-->>Caller: error (wrap Errors.ErrMerge + Errors.ErrInvalidListMergeStrategy)
else
MergePkg->>MergePkg: set flags via switch (merge/append/replace)
MergePkg->>MergePkg: perform merge steps (to YAML, unmarshal, mergo)
alt failure in steps
MergePkg-->>Caller: error (wrap Errors.ErrMerge + underlying error)
else
MergePkg-->>Caller: return merged result, nil
end
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
CLAUDE.md (1)
631-640: Tighten wording and punctuation in new “Compilation Requirements” bullets.Consistent, imperative style and periods improve readability; also avoid
2>&1in examples.Apply this diff:
-### Compilation Requirements (MANDATORY) -- **ALWAYS compile after making changes** - Run `go build` after ANY code modification -- **Verify no compilation errors** before proceeding with further changes or commits -- **Run tests after successful compilation** - Execute `go test ./...` to ensure functionality -- **Never assume code changes work** without compilation verification -- **Use build-and-test pattern**: `go build -o binary . && go test ./... 2>&1` -- **Fix compilation errors immediately** - Do not proceed with additional changes until compilation succeeds -- **This prevents undefined function/variable errors** that waste time and create broken commits +### Compilation Requirements (MANDATORY) +- Always compile after making changes. Run `go build` after any code modification. +- Verify there are no compilation errors before committing. +- Run tests after a successful build. Execute `go test ./...` to ensure functionality. +- Do not assume changes work without compiling. +- Recommended build-and-test pattern: `go build ./... && go test ./...`. +- Fix compilation errors immediately; do not proceed until the build succeeds. +- Following this prevents undefined function or variable errors that cause broken commits.pkg/merge/merge_test.go (2)
168-168: End comment with a period to satisfy godot.Minor lint fix.
- // Nil config should default to replace strategy + // Nil config should default to replace strategy.
1-175: Add a test that asserts the sentinel error on invalid strategy.Verifies
errors.Is(err, ErrInvalidListMergeStrategy).Add this test:
func TestMerge_InvalidListMergeStrategy_ReturnsSentinel(t *testing.T) { atmosConfig := schema.AtmosConfiguration{ Settings: schema.AtmosSettings{ListMergeStrategy: "nope"}, } inputs := []map[string]any{{"k": "v"}} _, err := Merge(&atmosConfig, inputs) assert.Error(t, err) assert.True(t, errors.Is(err, errUtils.ErrInvalidListMergeStrategy)) }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
CLAUDE.md(1 hunks)errors/errors.go(1 hunks)pkg/merge/merge.go(4 hunks)pkg/merge/merge_test.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
**/*.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 Go comments must be complete sentences ending with periods (enforced by golangci-lint godot).
Wrap all returned errors with a static error first using fmt.Errorf("%w: details", errUtils.ErrStaticError, ...); never return dynamic errors directly.
Distinguish structured logging from UI output; never use logging for UI prompts or messages.
Use structured logging without message interpolation; follow logging levels per docs/logging.md (Error/Warn in prod; Debug/Trace disabled).
Always bind environment variables via viper.BindEnv().
Every environment variable must have an ATMOS_ alternative when binding (e.g., viper.BindEnv("ATMOS_GITHUB_TOKEN", "GITHUB_TOKEN")).
For non-standard execution paths, capture telemetry via telemetry.CaptureCmd or telemetry.CaptureCmdString without user data.
Ensure cross-platform compatibility: prefer SDKs to external binaries, use filepath.Join, os.PathSeparator/PathListSeparator, and runtime.GOOS only when necessary.Format Go code with
gofumpt -w .andgoimports -w .
Files:
errors/errors.gopkg/merge/merge_test.gopkg/merge/merge.go
errors/errors.go
📄 CodeRabbit inference engine (CLAUDE.md)
Define all static errors in errors/errors.go (e.g., ErrInvalidComponent, ErrInvalidStack, ErrInvalidConfig).
Files:
errors/errors.go
**/*
📄 CodeRabbit inference engine (CLAUDE.md)
Co-locate tests with implementation (Go _test.go files alongside .go files).
Files:
errors/errors.gopkg/merge/merge_test.goCLAUDE.mdpkg/merge/merge.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: Use table-driven tests for unit tests and prefer pure functions.
Test file naming should mirror implementation files (e.g., foo.go -> foo_test.go).
Files:
pkg/merge/merge_test.go
pkg/**/*_test.go
📄 CodeRabbit inference engine (CLAUDE.md)
Aim unit test coverage >80% for pkg/ and prioritize core logic coverage.
Files:
pkg/merge/merge_test.go
pkg/**
📄 CodeRabbit inference engine (CLAUDE.md)
Group related functionality into focused subpackages in pkg/ by domain (config, stack, component, utils, validate, workflow, hooks, telemetry).
Files:
pkg/merge/merge_test.gopkg/merge/merge.go
🧠 Learnings (7)
📚 Learning: 2025-09-02T14:43:39.586Z
Learnt from: CR
PR: cloudposse/atmos#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-02T14:43:39.586Z
Learning: Applies to **/*.go : Ensure cross-platform compatibility: prefer SDKs to external binaries, use filepath.Join, os.PathSeparator/PathListSeparator, and runtime.GOOS only when necessary.
Applied to files:
CLAUDE.md
📚 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: All code must pass golangci-lint checks
Applied to files:
CLAUDE.md
📚 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 **/*.go : Follow Go's documentation conventions
Applied to files:
CLAUDE.md
📚 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 **/*.go : Follow standard Go coding style
Applied to files:
CLAUDE.md
📚 Learning: 2025-09-02T14:43:39.586Z
Learnt from: CR
PR: cloudposse/atmos#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-02T14:43:39.586Z
Learning: Applies to pkg/ui/theme/colors.go : Use canonical Atmos theme colors from pkg/ui/theme/colors.go (ColorGreen, ColorCyan, ColorRed, ColorSelectedItem, ColorBorder).
Applied to files:
pkg/merge/merge.go
📚 Learning: 2025-09-02T14:43:39.586Z
Learnt from: CR
PR: cloudposse/atmos#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-02T14:43:39.586Z
Learning: Applies to {cmd,pkg/ui}/**/*.go : Use theme.Styles and theme.Colors for consistent TUI formatting.
Applied to files:
pkg/merge/merge.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/merge/merge.go
🧬 Code graph analysis (2)
pkg/merge/merge_test.go (1)
pkg/merge/merge.go (1)
Merge(78-108)
pkg/merge/merge.go (2)
pkg/schema/schema.go (1)
AtmosConfiguration(25-60)errors/errors.go (1)
ErrInvalidListMergeStrategy(67-67)
🪛 LanguageTool
CLAUDE.md
[grammar] ~633-~633: There might be a mistake here.
Context: ...### Compilation Requirements (MANDATORY) - ALWAYS compile after making changes - ...
(QB_NEW_EN)
[style] ~635-~635: Consider an alternative to strengthen your wording.
Context: ...ilation errors** before proceeding with further changes or commits - **Run tests after successf...
(CHANGES_ADJUSTMENTS)
[style] ~636-~636: Consider using the typographical ellipsis character here instead.
Context: ...fter successful compilation** - Execute go test ./... to ensure functionality - **Never assu...
(ELLIPSIS)
[grammar] ~638-~638: There might be a mistake here.
Context: ...** without compilation verification - Use build-and-test pattern: `go build -o ...
(QB_NEW_EN)
[grammar] ~638-~638: There might be a mistake here.
Context: ...ation - Use build-and-test pattern: go build -o binary . && go test ./... 2>&1 - Fix compilation errors immediately - D...
(QB_NEW_EN)
⏰ 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: Lint (golangci)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Build (macos-latest, macos)
- GitHub Check: Analyze (go)
- GitHub Check: Lint (golangci)
- GitHub Check: Summary
🔇 Additional comments (4)
errors/errors.go (1)
66-68: Good addition of static sentinel for list strategy errors.This aligns with the error-wrapping guideline and enables errors.Is checks.
pkg/merge/merge_test.go (1)
163-174: Nil-config test looks good; covers replace semantics.Assertions are clear and exercise the new defaulting behavior.
pkg/merge/merge.go (2)
91-95: Switch-based strategy selection and sentinel-wrapped error are solid.Cleaner flow, better errors.Is semantics.
Also applies to: 100-105
77-107: Remove ErrMerge wrapping suggestion: the codebase doesn’t define anErrMergesentinel—ignore this recommendation or first add that sentinel before wrapping errors.Likely an incorrect or invalid review comment.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1440 +/- ##
==========================================
+ Coverage 55.58% 55.68% +0.10%
==========================================
Files 273 273
Lines 28621 28626 +5
==========================================
+ Hits 15908 15941 +33
+ Misses 10930 10900 -30
- Partials 1783 1785 +2
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:
|
- Add TestMergeWithInvalidStrategy to test error handling for invalid merge strategies - Add TestMergeWithEmptyInputs to test edge cases with empty and nil inputs - Achieve 100% coverage for Merge function - Overall package coverage increased from 76.5% to 82.4% 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
pkg/merge/merge_test.go (3)
163-174: Prefer assert.NoError and fix comment punctuation.Small consistency and lint nits.
- // Nil config should default to replace strategy + // Nil config should default to replace strategy. result, err := Merge(nil, inputs) - assert.Nil(t, err) + assert.NoError(t, err)
195-222: Add a nil-slice case, use assert.NoError, and end comments with periods.Covers one more edge and keeps style/lints consistent.
// Test with empty inputs slice - inputs := []map[string]any{} + // Test with empty inputs slice. + inputs := []map[string]any{} result, err := Merge(&atmosConfig, inputs) - assert.Nil(t, err) + assert.NoError(t, err) assert.NotNil(t, result) assert.Empty(t, result) - // Test with nil maps in inputs + // Test with nil maps in inputs. inputs = []map[string]any{nil, nil} result, err = Merge(&atmosConfig, inputs) - assert.Nil(t, err) + assert.NoError(t, err) assert.NotNil(t, result) assert.Empty(t, result) + // Test with nil inputs slice (uninitialized). + var nilInputs []map[string]any + result, err = Merge(&atmosConfig, nilInputs) + assert.NoError(t, err) + assert.NotNil(t, result) + assert.Empty(t, result) + - // Test with mix of empty and non-empty maps + // Test with mix of empty and non-empty maps. inputs = []map[string]any{{}, {"foo": "bar"}, {}} result, err = Merge(&atmosConfig, inputs) - assert.Nil(t, err) + assert.NoError(t, err) assert.NotNil(t, result) assert.Equal(t, "bar", result["foo"])Optional: consider consolidating these related cases into a single table-driven test with t.Run for less duplication.
176-193: Use errors.Is for ErrInvalidListMergeStrategy
Replace the string‐based check withassert.ErrorIsagainst the sentinel error and retain the message assertions; fix the import path forerrUtils.func TestMergeWithInvalidStrategy(t *testing.T) { … result, err := Merge(&atmosConfig, inputs) assert.Nil(t, result) - assert.NotNil(t, err) - assert.Contains(t, err.Error(), "invalid list merge strategy") + assert.NotNil(t, err) + assert.ErrorIs(t, err, errUtils.ErrInvalidListMergeStrategy) + // Retain context in the error message. + assert.Contains(t, err.Error(), "invalid list merge strategy") assert.Contains(t, err.Error(), "invalid-strategy") assert.Contains(t, err.Error(), "replace, append, merge") }import ( "testing" "github.com/stretchr/testify/assert" "github.com/cloudposse/atmos/pkg/schema" - errUtils "github.com/cloudposse/atmos/pkg/errors" + errUtils "github.com/cloudposse/atmos/errors" u "github.com/cloudposse/atmos/pkg/utils" )
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
pkg/merge/merge_test.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.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 Go comments must be complete sentences ending with periods (enforced by golangci-lint godot).
Wrap all returned errors with a static error first using fmt.Errorf("%w: details", errUtils.ErrStaticError, ...); never return dynamic errors directly.
Distinguish structured logging from UI output; never use logging for UI prompts or messages.
Use structured logging without message interpolation; follow logging levels per docs/logging.md (Error/Warn in prod; Debug/Trace disabled).
Always bind environment variables via viper.BindEnv().
Every environment variable must have an ATMOS_ alternative when binding (e.g., viper.BindEnv("ATMOS_GITHUB_TOKEN", "GITHUB_TOKEN")).
For non-standard execution paths, capture telemetry via telemetry.CaptureCmd or telemetry.CaptureCmdString without user data.
Ensure cross-platform compatibility: prefer SDKs to external binaries, use filepath.Join, os.PathSeparator/PathListSeparator, and runtime.GOOS only when necessary.Format Go code with
gofumpt -w .andgoimports -w .
Files:
pkg/merge/merge_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 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: Use table-driven tests for unit tests and prefer pure functions.
Test file naming should mirror implementation files (e.g., foo.go -> foo_test.go).
Files:
pkg/merge/merge_test.go
pkg/**/*_test.go
📄 CodeRabbit inference engine (CLAUDE.md)
Aim unit test coverage >80% for pkg/ and prioritize core logic coverage.
Files:
pkg/merge/merge_test.go
pkg/**
📄 CodeRabbit inference engine (CLAUDE.md)
Group related functionality into focused subpackages in pkg/ by domain (config, stack, component, utils, validate, workflow, hooks, telemetry).
Files:
pkg/merge/merge_test.go
**/*
📄 CodeRabbit inference engine (CLAUDE.md)
Co-locate tests with implementation (Go _test.go files alongside .go files).
Files:
pkg/merge/merge_test.go
🧠 Learnings (1)
📚 Learning: 2025-09-02T14:43:39.586Z
Learnt from: CR
PR: cloudposse/atmos#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-02T14:43:39.586Z
Learning: Applies to internal/exec/**/*_test.go : Add comprehensive tests for new template functions under internal/exec/.
Applied to files:
pkg/merge/merge_test.go
🧬 Code graph analysis (1)
pkg/merge/merge_test.go (2)
pkg/merge/merge.go (2)
Merge(78-108)ListMergeStrategyReplace(16-16)pkg/schema/schema.go (3)
AtmosConfiguration(25-60)Settings(834-838)AtmosSettings(226-246)
⏰ 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: Analyze (go)
- GitHub Check: Lint (golangci)
- GitHub Check: Summary
- Add ErrMerge sentinel error to enable downstream error detection
- Wrap all merge-related errors with ErrMerge using fmt.Errorf("%w", ...)
- Update MergeWithOptions to wrap YAML conversion and mergo errors
- Update Merge to wrap invalid strategy errors with both ErrMerge and ErrInvalidListMergeStrategy
- Add tests to verify error wrapping works with errors.Is()
- Enables callers to detect merge failures using errors.Is(err, errors.ErrMerge)
This improves error handling consistency and allows calling code to properly
detect and handle merge-specific failures.
🤖 Generated with [Claude Code](https://claude.ai/code)
Co-Authored-By: Claude <noreply@anthropic.com>
- Add explicit nil check for atmosConfig to prevent runtime panic - Return proper error instead of panicking when atmosConfig is nil - Update tests to expect error on nil config instead of default behavior - Add TestMergePreventsNilConfigPanic to demonstrate panic prevention - Coverage increased to 83.3% This fixes intermittent panics that occur when Merge is called with nil atmosConfig, typically during component initialization or incomplete configuration scenarios. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
- Replace TestMergePreventsNilConfigPanic with TestMergeHandlesNilConfigWithoutPanic - This test would panic without the nil check (verified by temporarily removing it) - Panic occurs at line: if atmosConfig.Settings.ListMergeStrategy \!= "" - With the nil check in place, the test passes and returns a proper error This test proves that the nil check successfully prevents the runtime panic that was affecting the system. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
pkg/merge/merge.go (1)
45-46: Good: static-first error wrapping; consider underlying cause visibility.Using
%wfor the static sentinel aligns with guidelines. If you later need callers to match underlying causes (YAML/unmarshal/mergo), consider joining:errors.Join(fmt.Errorf("%w: context", errUtils.ErrMerge), err). Not required now; tests don’t assert on underlying causes.Also applies to: 51-52, 70-71
pkg/merge/merge_test.go (1)
167-181: Duplicate coverage with TestMerge_NilAtmosConfigReturnsError.These assertions overlap with the earlier test. Consider consolidating to reduce maintenance.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- 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)pkg/merge/merge.go(4 hunks)pkg/merge/merge_test.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- errors/errors.go
🧰 Additional context used
📓 Path-based instructions (5)
**/*.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 Go comments must be complete sentences ending with periods (enforced by golangci-lint godot).
Wrap all returned errors with a static error first using fmt.Errorf("%w: details", errUtils.ErrStaticError, ...); never return dynamic errors directly.
Distinguish structured logging from UI output; never use logging for UI prompts or messages.
Use structured logging without message interpolation; follow logging levels per docs/logging.md (Error/Warn in prod; Debug/Trace disabled).
Always bind environment variables via viper.BindEnv().
Every environment variable must have an ATMOS_ alternative when binding (e.g., viper.BindEnv("ATMOS_GITHUB_TOKEN", "GITHUB_TOKEN")).
For non-standard execution paths, capture telemetry via telemetry.CaptureCmd or telemetry.CaptureCmdString without user data.
Ensure cross-platform compatibility: prefer SDKs to external binaries, use filepath.Join, os.PathSeparator/PathListSeparator, and runtime.GOOS only when necessary.Format Go code with
gofumpt -w .andgoimports -w .
Files:
pkg/merge/merge_test.gopkg/merge/merge.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: Use table-driven tests for unit tests and prefer pure functions.
Test file naming should mirror implementation files (e.g., foo.go -> foo_test.go).
Files:
pkg/merge/merge_test.go
pkg/**/*_test.go
📄 CodeRabbit inference engine (CLAUDE.md)
Aim unit test coverage >80% for pkg/ and prioritize core logic coverage.
Files:
pkg/merge/merge_test.go
pkg/**
📄 CodeRabbit inference engine (CLAUDE.md)
Group related functionality into focused subpackages in pkg/ by domain (config, stack, component, utils, validate, workflow, hooks, telemetry).
Files:
pkg/merge/merge_test.gopkg/merge/merge.go
**/*
📄 CodeRabbit inference engine (CLAUDE.md)
Co-locate tests with implementation (Go _test.go files alongside .go files).
Files:
pkg/merge/merge_test.gopkg/merge/merge.go
🧠 Learnings (4)
📚 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/merge/merge_test.gopkg/merge/merge.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:
pkg/merge/merge.go
📚 Learning: 2025-09-02T14:43:39.586Z
Learnt from: CR
PR: cloudposse/atmos#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-02T14:43:39.586Z
Learning: Applies to pkg/ui/theme/colors.go : Use canonical Atmos theme colors from pkg/ui/theme/colors.go (ColorGreen, ColorCyan, ColorRed, ColorSelectedItem, ColorBorder).
Applied to files:
pkg/merge/merge.go
📚 Learning: 2025-09-02T14:43:39.586Z
Learnt from: CR
PR: cloudposse/atmos#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-02T14:43:39.586Z
Learning: Applies to {cmd,pkg/ui}/**/*.go : Use theme.Styles and theme.Colors for consistent TUI formatting.
Applied to files:
pkg/merge/merge.go
🧬 Code graph analysis (2)
pkg/merge/merge_test.go (3)
pkg/merge/merge.go (2)
Merge(78-114)ListMergeStrategyReplace(16-16)errors/errors.go (3)
ErrMerge(68-68)ErrAtmosConfigIsNil(66-66)ErrInvalidListMergeStrategy(67-67)pkg/schema/schema.go (3)
AtmosConfiguration(25-60)Settings(834-838)AtmosSettings(226-246)
pkg/merge/merge.go (2)
errors/errors.go (3)
ErrMerge(68-68)ErrAtmosConfigIsNil(66-66)ErrInvalidListMergeStrategy(67-67)pkg/schema/schema.go (2)
AtmosConfiguration(25-60)Settings(834-838)
🪛 GitHub Check: golangci-lint
pkg/merge/merge_test.go
[failure] 240-240:
TestMergePreventsNilConfigPanic$1 - inputs is unused
⏰ 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). (2)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Summary
🔇 Additional comments (8)
pkg/merge/merge.go (5)
9-9: Consistent use of shared error sentinels.Aliasing errors as
errUtilskeeps error handling consistent with the repo’s conventions.
82-85: Nil-config guard prevents panics.Early return with
ErrMerge+ErrAtmosConfigIsNilis correct and matches tests.
87-91: Confirm backward-compat default to “replace”.Defaulting to
replacewhen unset looks right; please confirm no callers relied on the previous implicit behavior differing from this.
93-101: Clear validation for invalid strategies.Returning
ErrMergewrapped withErrInvalidListMergeStrategyand listing supported values is solid UX.
106-111: Switch over strategy is clearer than if/else.Readable and easy to extend.
pkg/merge/merge_test.go (3)
28-42: Nil-config now returns wrapped error (not panic).Asserts cover both
ErrMergeandErrAtmosConfigIsNil. Good.
183-205: Invalid strategy path well covered.Checks for message content and both sentinels are on point.
206-234: Empty inputs behavior verified.Covers empty slice, nil maps, and mixed maps. Nice.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
pkg/merge/merge_test.go (5)
28-42: Fail fast on error and fix comment punctuation.Switch to require for the error/result checks to avoid continuing after a failed expectation; add periods to comments.
- // Nil atmosConfig should return an error to prevent panic + // Nil atmosConfig should return an error to prevent panic. @@ - assert.Nil(t, res) - assert.NotNil(t, err) + require.Nil(t, res) + require.NotNil(t, err) @@ - // Verify the error is properly wrapped + // Verify the error is properly wrapped.Add the import (outside this hunk):
import ( // ... "github.com/stretchr/testify/require" )
166-181: Deduplicate with TestMerge_NilAtmosConfigReturnsError.This test overlaps the earlier nil-config case. Consider merging into one table-driven test with subtests for message and wrapping checks, or remove one. Also add periods to comments.
- // Nil config should return an error + // Nil config should return an error. @@ - // Verify proper error wrapping + // Verify proper error wrapping.
183-204: Tighten assertions and punctuate comment.Use require for err before calling err.Error(); keep Contains checks. Add final period to the comment.
result, err := Merge(&atmosConfig, inputs) assert.Nil(t, result) - assert.NotNil(t, err) + require.NotNil(t, err) assert.Contains(t, err.Error(), "invalid list merge strategy") assert.Contains(t, err.Error(), "invalid-strategy") assert.Contains(t, err.Error(), "replace, append, merge") @@ - // Verify error wrapping - should be wrapped with both ErrMerge and ErrInvalidListMergeStrategy + // Verify error wrapping - should be wrapped with both ErrMerge and ErrInvalidListMergeStrategy.(Ensure require is imported once at top.)
206-233: Use NoError for clarity; consider subtests.Swap assert.Nil(err) with assert.NoError(err) and punctuate comments. Consider t.Run subtests for the three cases for clearer reporting.
- // Test with empty inputs slice + // Test with empty inputs slice. @@ - assert.Nil(t, err) + assert.NoError(t, err) @@ - // Test with nil maps in inputs + // Test with nil maps in inputs. @@ - assert.Nil(t, err) + assert.NoError(t, err) @@ - // Test with mix of empty and non-empty maps + // Test with mix of empty and non-empty maps. @@ - assert.Nil(t, err) + assert.NoError(t, err)
235-254: Explicitly assert no panic.Wrap the call in assert.NotPanics to make intent clear; punctuate multi-line comments.
- // This test verifies that Merge handles nil config gracefully - // Without the nil check in Merge, this test would panic when - // the function tries to access atmosConfig.Settings.ListMergeStrategy + // This test verifies that Merge handles nil config gracefully. + // Without the nil check in Merge, this test would panic when + // the function tries to access atmosConfig.Settings.ListMergeStrategy. @@ - // Call Merge with nil config - this would panic without our fix - // at the line: if atmosConfig.Settings.ListMergeStrategy != "" - result, err := Merge(nil, inputs) + // Call Merge with nil config - this would panic without our fix + // at the line: if atmosConfig.Settings.ListMergeStrategy != "". + var result map[string]any + var err error + assert.NotPanics(t, func() { + result, err = Merge(nil, inputs) + })
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
pkg/merge/merge_test.go(3 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.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 Go comments must be complete sentences ending with periods (enforced by golangci-lint godot).
Wrap all returned errors with a static error first using fmt.Errorf("%w: details", errUtils.ErrStaticError, ...); never return dynamic errors directly.
Distinguish structured logging from UI output; never use logging for UI prompts or messages.
Use structured logging without message interpolation; follow logging levels per docs/logging.md (Error/Warn in prod; Debug/Trace disabled).
Always bind environment variables via viper.BindEnv().
Every environment variable must have an ATMOS_ alternative when binding (e.g., viper.BindEnv("ATMOS_GITHUB_TOKEN", "GITHUB_TOKEN")).
For non-standard execution paths, capture telemetry via telemetry.CaptureCmd or telemetry.CaptureCmdString without user data.
Ensure cross-platform compatibility: prefer SDKs to external binaries, use filepath.Join, os.PathSeparator/PathListSeparator, and runtime.GOOS only when necessary.Format Go code with
gofumpt -w .andgoimports -w .
Files:
pkg/merge/merge_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 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: Use table-driven tests for unit tests and prefer pure functions.
Test file naming should mirror implementation files (e.g., foo.go -> foo_test.go).
Files:
pkg/merge/merge_test.go
pkg/**/*_test.go
📄 CodeRabbit inference engine (CLAUDE.md)
Aim unit test coverage >80% for pkg/ and prioritize core logic coverage.
Files:
pkg/merge/merge_test.go
pkg/**
📄 CodeRabbit inference engine (CLAUDE.md)
Group related functionality into focused subpackages in pkg/ by domain (config, stack, component, utils, validate, workflow, hooks, telemetry).
Files:
pkg/merge/merge_test.go
**/*
📄 CodeRabbit inference engine (CLAUDE.md)
Co-locate tests with implementation (Go _test.go files alongside .go files).
Files:
pkg/merge/merge_test.go
🧠 Learnings (3)
📓 Common learnings
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.
📚 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/merge/merge_test.go
📚 Learning: 2024-10-31T07:09:31.983Z
Learnt from: Cerebrovinny
PR: cloudposse/atmos#737
File: internal/exec/vendor_utils.go:181-182
Timestamp: 2024-10-31T07:09:31.983Z
Learning: In `internal/exec/vendor_utils.go`, the variables `mergedSources` and `mergedImports` are declared and used later in the code. Do not suggest removing them as unused variables.
Applied to files:
pkg/merge/merge_test.go
🧬 Code graph analysis (1)
pkg/merge/merge_test.go (3)
pkg/merge/merge.go (2)
Merge(78-114)ListMergeStrategyReplace(16-16)errors/errors.go (3)
ErrMerge(68-68)ErrAtmosConfigIsNil(66-66)ErrInvalidListMergeStrategy(67-67)pkg/schema/schema.go (3)
AtmosConfiguration(25-60)Settings(834-838)AtmosSettings(226-246)
⏰ 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). (2)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Summary
🔇 Additional comments (1)
pkg/merge/merge_test.go (1)
4-12: Imports look good.Adding errors and the errUtils alias is appropriate for wrapped-error assertions.
Resolve merge conflicts and integrate latest main branch changes: ✅ **errors/errors.go**: Added ErrMerge error for improved error handling ✅ **pkg/merge/merge.go**: Updated merge logic with better error wrapping and strategy handling ✅ **pkg/merge/merge_test.go**: Enhanced tests with proper error verification ✅ **cmd/describe_stacks.go**: Updated component types handling to support multi-values ✅ **cmd/describe_stacks_test.go**: Updated tests for new describe stacks functionality ✅ **website/docs/**: Fixed sample stack configuration All conflicts resolved in favor of main branch improvements while preserving our gotcha tool implementation and documentation changes. Merged commits from main: - 5e2f1c3 fix: improve error handling and code quality in merge package (#1440) - fd1399a [describe-stacks] Fix: switch --component-types to StringSlice (#1432) - c5c97fb Fix sample stacks/deploy/dev.yaml in documentation (#1434)
|
These changes were released in v1.189.0. |
* fix: improve error handling and code quality in merge package - Add ErrInvalidListMergeStrategy static error constant - Improve Merge function with better error handling using static errors - Replace if-else chain with switch statement for cleaner code - Add test coverage for nil configuration handling - Update CLAUDE.md with compilation requirements - Maintain backward compatibility for nil config (defaults to replace strategy) 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com> * test: increase merge package coverage to 82.4% - Add TestMergeWithInvalidStrategy to test error handling for invalid merge strategies - Add TestMergeWithEmptyInputs to test edge cases with empty and nil inputs - Achieve 100% coverage for Merge function - Overall package coverage increased from 76.5% to 82.4% 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com> * feat: add ErrMerge sentinel error for better error handling - Add ErrMerge sentinel error to enable downstream error detection - Wrap all merge-related errors with ErrMerge using fmt.Errorf("%w", ...) - Update MergeWithOptions to wrap YAML conversion and mergo errors - Update Merge to wrap invalid strategy errors with both ErrMerge and ErrInvalidListMergeStrategy - Add tests to verify error wrapping works with errors.Is() - Enables callers to detect merge failures using errors.Is(err, errors.ErrMerge) This improves error handling consistency and allows calling code to properly detect and handle merge-specific failures. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com> * [autofix.ci] apply automated fixes * fix: add nil check to prevent panic in merge package - Add explicit nil check for atmosConfig to prevent runtime panic - Return proper error instead of panicking when atmosConfig is nil - Update tests to expect error on nil config instead of default behavior - Add TestMergePreventsNilConfigPanic to demonstrate panic prevention - Coverage increased to 83.3% This fixes intermittent panics that occur when Merge is called with nil atmosConfig, typically during component initialization or incomplete configuration scenarios. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com> * [autofix.ci] apply automated fixes * test: add test that demonstrates panic prevention - Replace TestMergePreventsNilConfigPanic with TestMergeHandlesNilConfigWithoutPanic - This test would panic without the nil check (verified by temporarily removing it) - Panic occurs at line: if atmosConfig.Settings.ListMergeStrategy \!= "" - With the nil check in place, the test passes and returns a proper error This test proves that the nil check successfully prevents the runtime panic that was affecting the system. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com> * [autofix.ci] apply automated fixes --------- Co-authored-by: Claude <noreply@anthropic.com> Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
what
ErrInvalidListMergeStrategystatic error constant for consistent error handlingMergefunction with better error handling to prevent panicswhy
We've been experiencing intermittent panics in the merge package when the atmos configuration is unexpectedly nil or contains invalid merge strategies. This can occur during:
These defensive improvements ensure the merge functionality gracefully handles edge cases instead of panicking, improving overall stability and reliability of the Atmos CLI.
references
🤖 Generated with Claude Code
Summary by CodeRabbit
Documentation
Refactor
Tests