feat: add pre-commit hooks and development workflow #1469
Conversation
📝 WalkthroughWalkthroughAdds developer tooling, CI, and test helpers: Atmos dev workflow and README in .atmos.d, pre-commit CI and configuration, docs and .gitignore updates, and test/runtime hooks to override Git repo root (TEST_GIT_ROOT) and skip loading .atmos.d in tests (TEST_EXCLUDE_ATMOS_D). Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev as Developer
participant GH as GitHub
participant GHA as GitHub Actions
participant PC as pre-commit
Note over GH,GHA: PR opened or updated
Dev->>GH: Push commits / open PR
GH-->>GHA: Trigger workflow (pull_request)
GHA->>GHA: Checkout + setup Go & Python
GHA->>GHA: Install gofumpt & golangci-lint
GHA->>PC: Run pre-commit (--from-ref base..HEAD)
PC-->>GHA: Hook results
GHA-->>GH: Post status (success/failure)
sequenceDiagram
autonumber
actor Dev as Developer
participant Atmos as atmos CLI
participant DevY as .atmos.d/dev.yaml
participant Tools as Local Tools
Note over Atmos,DevY: Atmos loads custom commands from .atmos.d when present
Dev->>Atmos: atmos dev <subcommand>
Atmos->>DevY: Resolve steps for subcommand
DevY->>Tools: Execute steps (install, format, lint, test, build, update-hooks)
Tools-->>DevY: Return exit status & output
DevY-->>Atmos: Command result
Atmos-->>Dev: Display status and logs
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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. Comment |
|
Important Cloud Posse Engineering Team Review RequiredThis pull request modifies files that require Cloud Posse's review. Please be patient, and a core maintainer will review your changes. To expedite this process, reach out to us on Slack in the |
osterman
added
the
no-release
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (7)
.gitignore (1)
45-49: Anchor .atmos.d ignore rules to repo root to avoid unintended matchesWithout a leading slash, these rules can match nested paths. Anchoring keeps the scope tight and intent clear.
- .atmos.d/* - !.atmos.d/dev.yaml - !.atmos.d/README.md + /.atmos.d/* + !/.atmos.d/dev.yaml + !/.atmos.d/README.md.pre-commit-config.yaml (1)
22-23: Clarify Go version in commentComment mentions “Go 1.25” incompatibility; project docs list Go 1.24+. Align the note to avoid confusion.
- # Note: Commented out due to Go 1.25 compatibility issue with building from source + # Note: Commented out due to current Go toolchain compatibility issues when building from sourcedocs/development.md (1)
142-149: Prereq version matches; consider pinning tools for reproducibilityGo 1.24+ is consistent with the repo; consider documenting minimum versions for golangci-lint and pre-commit for new contributors.
.github/workflows/pre-commit.yml (2)
16-21: Optional: add concurrency to cancel superseded runsSaves CI time on rapid pushes.
jobs: pre-commit: name: Run pre-commit hooks runs-on: ubuntu-latest timeout-minutes: 15 + concurrency: + group: ${{ github.workflow }}-${{ github.ref }} + cancel-in-progress: true
1-70: Fix YAML lint nits (trailing spaces, EOF newline)Strip trailing spaces on noted lines and add a terminal newline. Pre-commit’s trailing-whitespace and end-of-file-fixer will auto-correct, but committing a clean file avoids churn.
.atmos.d/dev.yaml (1)
1-96: YAML lint nits: trailing spaces and EOF newlineTrim trailing spaces on the flagged lines and add a final newline.
.atmos.d/README.md (1)
55-63: Relative links LGTM; minor enhancementConsider adding a short sentence that local custom files (e.g., local.yaml) are intentionally gitignored to encourage per-developer workflows.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
.atmos.d/README.md(1 hunks).atmos.d/dev.yaml(1 hunks).github/workflows/pre-commit.yml(1 hunks).gitignore(1 hunks).pre-commit-config.yaml(1 hunks)README.md(1 hunks)docs/development.md(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
README.md
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
Update README.md with new commands and features
Files:
README.md
🧠 Learnings (9)
📚 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:
.pre-commit-config.yamldocs/development.md
📚 Learning: 2025-01-25T03:51:57.689Z
Learnt from: Listener430
PR: cloudposse/atmos#934
File: tests/fixtures/scenarios/docs-generate/README.md.gotmpl:99-118
Timestamp: 2025-01-25T03:51:57.689Z
Learning: For the cloudposse/atmos repository, changes to template contents should be handled in dedicated PRs and are typically considered out of scope for PRs focused on other objectives.
Applied to files:
docs/development.md
📚 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:
docs/development.md
📚 Learning: 2024-11-25T17:17:15.703Z
Learnt from: RoseSecurity
PR: cloudposse/atmos#797
File: pkg/list/atmos.yaml:213-214
Timestamp: 2024-11-25T17:17:15.703Z
Learning: The file `pkg/list/atmos.yaml` is primarily intended for testing purposes.
Applied to files:
docs/development.md.gitignore.atmos.d/README.md
📚 Learning: 2025-01-08T19:01:32.938Z
Learnt from: osterman
PR: cloudposse/atmos#899
File: examples/tests/test-vendor/test-components/main.tf:1-7
Timestamp: 2025-01-08T19:01:32.938Z
Learning: In the examples/tests directory of the atmos project, code examples are intentionally kept minimal and simple to facilitate understanding. Avoid suggesting additional complexity or validations that might make the examples harder to follow.
Applied to files:
docs/development.md
📚 Learning: 2025-01-19T15:49:15.593Z
Learnt from: samtholiya
PR: cloudposse/atmos#955
File: tests/snapshots/TestCLICommands_atmos_validate_editorconfig_--help.stdout.golden:0-0
Timestamp: 2025-01-19T15:49:15.593Z
Learning: In future commits, the help text for Atmos CLI commands should be limited to only show component and stack parameters for commands that actually use them. This applies to the example usage section in command help text.
Applied to files:
docs/development.md
📚 Learning: 2024-12-11T18:40:12.808Z
Learnt from: Listener430
PR: cloudposse/atmos#844
File: cmd/helmfile.go:37-37
Timestamp: 2024-12-11T18:40:12.808Z
Learning: In the atmos project, `cliConfig` is initialized within the `cmd` package in `root.go` and can be used in other command files.
Applied to files:
docs/development.md
📚 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:
docs/development.md
📚 Learning: 2025-01-25T15:21:40.413Z
Learnt from: osterman
PR: cloudposse/atmos#808
File: examples/demo-atmos-cli-imports/atmos.yaml:8-8
Timestamp: 2025-01-25T15:21:40.413Z
Learning: In Atmos, when a directory is specified for configuration loading (e.g., in the `import` section of atmos.yaml), all files within that directory should be treated as Atmos configurations. Do not suggest restricting file extensions in directory-based glob patterns.
Applied to files:
.gitignore.atmos.d/README.md
🪛 YAMLlint (1.37.1)
.github/workflows/pre-commit.yml
[error] 21-21: trailing spaces
(trailing-spaces)
[error] 28-28: trailing spaces
(trailing-spaces)
[error] 34-34: trailing spaces
(trailing-spaces)
[error] 39-39: trailing spaces
(trailing-spaces)
[error] 42-42: trailing spaces
(trailing-spaces)
[error] 45-45: trailing spaces
(trailing-spaces)
[error] 50-50: trailing spaces
(trailing-spaces)
[error] 57-57: trailing spaces
(trailing-spaces)
[error] 64-64: trailing spaces
(trailing-spaces)
[error] 70-70: no new line character at the end of file
(new-line-at-end-of-file)
.atmos.d/dev.yaml
[error] 34-34: trailing spaces
(trailing-spaces)
[error] 40-40: trailing spaces
(trailing-spaces)
[error] 45-45: trailing spaces
(trailing-spaces)
[error] 55-55: trailing spaces
(trailing-spaces)
[error] 77-77: trailing spaces
(trailing-spaces)
[error] 89-89: trailing spaces
(trailing-spaces)
[error] 96-96: no new line character at the end of file
(new-line-at-end-of-file)
⏰ 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: Run pre-commit hooks
- GitHub Check: Analyze (go)
- GitHub Check: Lint (golangci)
- GitHub Check: Summary
🔇 Additional comments (2)
README.md (1)
157-173: Development section looks goodConcise quick-start, links to full guide, and reference to .atmos.d/dev.yaml are on point.
.atmos.d/README.md (1)
22-31: Nice clarity on git-tracked vs local-only filesScope and intent are clear; matches .gitignore rules.
- Add .pre-commit-config.yaml with Go-specific hooks and golangci-lint - Add GitHub Actions workflow to run pre-commit on PRs (changed files only) - Add .atmos.d/dev.yaml with intelligent tool installation (brew/apt/pip) - Use local golangci-lint hook to avoid Go 1.25 compatibility issues This ensures code quality checks run both locally and in CI, with golangci-lint preventing massive functions and files as requested.
osterman
force-pushed
the
precommit-hooks
branch
from
85d0bf4 to
735b433
Compare
- Explain development setup and workflow - Document pre-commit hooks and their purpose - Provide troubleshooting guidance - Emphasize dogfooding with atmos dev commands
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
.atmos.d/dev.yaml (1)
52-56: Make “check” operate on staged files only (aligns with docs).- name: check description: Run pre-commit hooks on staged files steps: - - pre-commit run + - | + set -eo pipefail + files="$(git diff --name-only --cached)" + if [ -z "$files" ]; then + echo "No staged files. Stage changes or use 'atmos dev check-all'." + exit 0 + fi + # shellcheck disable=SC2086 + pre-commit run --files $files
🧹 Nitpick comments (7)
.pre-commit-config.yaml (2)
18-29: Make golangci-lint base ref robust (forks, fresh clones).Using --new-from-rev=origin/main can fail/miss changes when origin/main is absent or renamed. Wrap with a fallback to merge-base or HEAD~ to keep lint useful.
- entry: golangci-lint run --new-from-rev=origin/main --config=.golangci.yml - language: system + entry: bash + args: + - -lc + - | + set -eo pipefail + base="${GOLANGCI_LINT_BASE:-origin/main}" + if ! git rev-parse --verify "$base" >/dev/null 2>&1; then + base="$(git merge-base --fork-point origin/HEAD HEAD || echo HEAD~)" + fi + golangci-lint run --new-from-rev="$base" --config=.golangci.yml + language: system types: [go] pass_filenames: false require_serial: true exclude: ^(vendor/|tests/test-cases/|tests/testdata/)
3-8: Pin to immutable revisions for reproducibility.Tags can move; prefer commit SHAs (for both repos) to avoid supply-chain drift. Keep a note to periodically rotate pins.
.atmos.d/dev.yaml (5)
29-36: Avoid curl | sh; install golangci-lint via go install with a pinned version.Safer and cacheable; let version be overridden via env.
- else - echo "Installing golangci-lint via curl..." - curl -sSfL https://raw.githubusercontent.com/golangci/golangci-lint/master/install.sh | sh -s -- -b $(go env GOPATH)/bin - fi + else + echo "Installing golangci-lint via go install..." + GOLANGCI_LINT_VERSION="${GOLANGCI_LINT_VERSION:-v1.61.0}" + go install github.com/golangci/golangci-lint/cmd/golangci-lint@${GOLANGCI_LINT_VERSION} + fi
7-12: Fail fast in setup scripts.Add strict bash flags to surface errors early.
- | - echo "🚀 Setting up Atmos development environment..." + set -euo pipefail + echo "🚀 Setting up Atmos development environment..."
44-46: Pin gofumpt version (override via env).Prevents accidental formatter drift across machines.
- go install mvdan.cc/gofumpt@latest + GOFUMPT_VERSION="${GOFUMPT_VERSION:-v0.6.0}" + go install mvdan.cc/gofumpt@${GOFUMPT_VERSION}
71-75: Set a lint timeout to avoid default 1m flakiness.- - golangci-lint run --config=.golangci.yml + - golangci-lint run --config=.golangci.yml --timeout=5m
100-124: Keep help text synced when hooks change.Minor: this drifts easily; consider generating from .pre-commit-config.yaml or keeping it shorter.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
.atmos.d/README.md(1 hunks).atmos.d/dev.yaml(1 hunks).github/workflows/pre-commit.yml(1 hunks).pre-commit-config.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- .atmos.d/README.md
- .github/workflows/pre-commit.yml
🧰 Additional context used
🧠 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: 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:
.pre-commit-config.yaml
📚 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: Run golangci-lint and fix any issues before submitting a pull request
Applied to files:
.pre-commit-config.yaml
⏰ 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 (windows-latest, windows)
- GitHub Check: Lint (golangci)
- GitHub Check: Analyze (go)
- GitHub Check: Summary
🔇 Additional comments (3)
.pre-commit-config.yaml (2)
31-44: LGTM: hygiene hooks and excludes look sane.
21-29: golangci-lint installation verified in CI
The pre-commit workflow (.github/workflows/pre-commit.ymllines 35–39) installsgolangci-lintand adds it to the PATH before running hooks, so the localsystemhook will run as expected..atmos.d/dev.yaml (1)
90-93: No update needed:make buildproduces./build/atmos.
Ensure only specific .atmos.d files are tracked (dev.yaml and README.md) while ignoring any other temporary or generated files in that directory.
- Add 'atmos dev check-pr' command to check PR changes specifically - Keep 'atmos dev check' for staged files (local development) - Update documentation to clarify when to use each check command - Handle both local and CI environments in check-pr command This ensures developers have the right tool for each context: - check: for pre-commit on staged files - check-pr: for reviewing PR changes - check-all: for comprehensive checks (use sparingly)
…cations BREAKING CHANGE: Renamed 'fix' command to 'format' for clarity - Check commands are now read-only (idempotent) - they only report issues - Format commands modify files - clearly separated and labeled - Added format-all with explicit DANGEROUS warning and 5-second delay - Protected golden snapshots in tests/snapshots/ from all hooks - Clear documentation explaining check vs format distinction This prevents developers from accidentally formatting the entire codebase when they just wanted to check for issues. Check commands are safe to run anytime, while format commands explicitly modify files.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
.atmos.d/dev.yaml (2)
98-105: Make “format” actually operate on staged files only.pre-commit run without args targets all files. Respect the command’s description and limit to staged paths; no-op when none.
- name: format description: Auto-format staged files steps: - | echo "🔧 Auto-formatting staged files..." - pre-commit run || true + files="$(git diff --name-only --cached)" + if [ -z "$files" ]; then + echo "No staged files. Stage changes or use 'atmos dev format-all'." + exit 0 + fi + # shellcheck disable=SC2086 + pre-commit run --files $files || true echo "✅ Formatting applied to staged files"
135-139: Add the promised “validate” command (build + lint + test).PR text advertises it; keep docs and implementation in sync.
- name: test description: Run tests steps: - go test ./... -v + + - name: validate + description: Build, lint, and test + steps: + - make build + - golangci-lint run --config=.golangci.yml --timeout="${GOLANGCI_LINT_TIMEOUT:-5m}" + - go test ./... -v
🧹 Nitpick comments (7)
.atmos.d/dev.yaml (7)
74-82: Ensure base ref exists locally for PR checks.Fetch the base branch before diffing; otherwise origin/$BASE_REF may be missing.
BASE_REF="${GITHUB_BASE_REF:-main}" + git fetch --quiet origin "$BASE_REF" || true # Get changed Go files CHANGED_GO_FILES=$(git diff origin/$BASE_REF...HEAD --name-only -- '*.go' | grep -v -E '^(vendor/|tests/test-cases/|tests/testdata/|tests/snapshots/)' || true) if [ -n "$CHANGED_GO_FILES" ]; then echo "Checking Go formatting..." echo "$CHANGED_GO_FILES" | xargs -I {} gofumpt -d {} 2>/dev/null | head -20 || true echo "Running golangci-lint..." - golangci-lint run --new-from-rev=origin/$BASE_REF --config=.golangci.yml || true + golangci-lint run --new-from-rev=origin/$BASE_REF --config=.golangci.yml --timeout="${GOLANGCI_LINT_TIMEOUT:-5m}" || true
60-66: Harden excludes and avoid top-level-only filtering.Current regex only excludes top-level vendor/tests. Exclude nested paths too; add a linter timeout.
- STAGED_GO_FILES=$(git diff --cached --name-only --diff-filter=ACM -- '*.go' | grep -v -E '^(vendor/|tests/test-cases/|tests/testdata/|tests/snapshots/)' || true) + STAGED_GO_FILES=$(git diff --cached --name-only --diff-filter=ACM -- '*.go' \ + | grep -Ev '(^|/)(vendor|tests/(test-cases|testdata|snapshots))/' || true) if [ -n "$STAGED_GO_FILES" ]; then echo "Checking Go formatting..." - echo "$STAGED_GO_FILES" | xargs -I {} gofumpt -d {} 2>/dev/null | head -20 || true + echo "$STAGED_GO_FILES" | xargs gofumpt -d 2>/dev/null | head -n "${HEAD_LIMIT:-20}" || true echo "Running golangci-lint..." - golangci-lint run --new-from-rev=origin/main --config=.golangci.yml $STAGED_GO_FILES || true + golangci-lint run --new-from-rev=origin/main --config=.golangci.yml --timeout="${GOLANGCI_LINT_TIMEOUT:-5m}" $STAGED_GO_FILES || true fi
93-95: Broaden find-path exclusions and make diff truncation controllable.- find . -name '*.go' -not -path './vendor/*' -not -path './tests/test-cases/*' -not -path './tests/testdata/*' -not -path './tests/snapshots/*' | xargs gofumpt -d 2>/dev/null | head -50 || true + find . -type f -name '*.go' \ + -not -path '*/vendor/*' \ + -not -path '*/tests/test-cases/*' \ + -not -path '*/tests/testdata/*' \ + -not -path '*/tests/snapshots/*' \ + | xargs gofumpt -d 2>/dev/null | head -n "${HEAD_LIMIT_ALL:-50}" || true - echo "Running comprehensive linting..." - golangci-lint run --config=.golangci.yml || true + echo "Running comprehensive linting..." + golangci-lint run --config=.golangci.yml --timeout="${GOLANGCI_LINT_TIMEOUT:-5m}" || true
35-39: Ensure golangci-lint is on PATH after install.The installer drops the binary in GOPATH/bin; export PATH for this session before invoking it.
- curl -sSfL https://raw.githubusercontent.com/golangci/golangci-lint/master/install.sh | sh -s -- -b $(go env GOPATH)/bin + export PATH="$(go env GOPATH)/bin:$PATH" + curl -sSfL https://raw.githubusercontent.com/golangci/golangci-lint/master/install.sh | sh -s -- -b "$(go env GOPATH)/bin"
63-63: Avoid per-file gofumpt invocations.Passing all files to a single gofumpt call is faster and simpler.
(Handled in the diff above with xargs gofumpt -d.)
184-187: Fix hook name typo in help.It’s “gofumpt”, not “go-fumpt”.
- echo " - go-fumpt: Go code formatting" + echo " - gofumpt: Go code formatting"
130-134: Set an explicit golangci-lint timeout.Avoid flakiness on slower machines.
- - golangci-lint run --config=.golangci.yml + - golangci-lint run --config=.golangci.yml --timeout="${GOLANGCI_LINT_TIMEOUT:-5m}"
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
.atmos.d/dev.yaml(1 hunks).gitignore(1 hunks).pre-commit-config.yaml(1 hunks)docs/development.md(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- .gitignore
- docs/development.md
🚧 Files skipped from review as they are similar to previous changes (1)
- .pre-commit-config.yaml
🧰 Additional context used
🧠 Learnings (1)
📓 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.
⏰ 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 (windows-latest, windows)
- GitHub Check: Analyze (go)
- GitHub Check: Lint (golangci)
- GitHub Check: Summary
🔇 Additional comments (3)
.atmos.d/dev.yaml (3)
1-192: Solid developer UX.Nice end-to-end workflow: setup, checks, format, lint, test, build, quick, and hook updates. Comments below focus on correctness and polish.
115-129: Exclusion patterns match help text
Verified that all formatting hooks (go-fumpt, go-build-mod, trailing-whitespace, end-of-file-fixer) in.pre-commit-config.yamlexcludevendor/,tests/test-cases/,tests/testdata/, andtests/snapshots/as promised; non-formatting hooks (e.g. check-yaml) aren’t covered by this guarantee.
130-190: Lint config validated. .golangci.yml is present at the repository root—no further changes required.
- Add ATMOS_TEST_EXCLUDE_DEFAULT_IMPORTS environment variable support to exclude specific paths from .atmos.d loading during tests - Modify test setup to automatically exclude repository root's .atmos.d directory - Add 'validate' command to .atmos.d/dev.yaml for comprehensive code validation (build + lint + test) - Preserve .atmos.d convention while giving tests control over config loading This fixes golden snapshot test failures caused by the repository's .atmos.d/dev.yaml being loaded during test execution, which added unexpected debug log messages to test output. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
.atmos.d/dev.yaml (6)
98-105: Make “format” operate only on staged files (consistent with description).Avoid scanning extra files; pass the staged list explicitly.
- - | - echo "🔧 Auto-formatting staged files..." - pre-commit run || true - echo "✅ Formatting applied to staged files" + - | + echo "🔧 Auto-formatting staged files..." + files="$(git diff --name-only --cached)" + if [ -z "$files" ]; then + echo "No staged files. Stage changes or use 'atmos dev format-all'." + exit 0 + fi + # shellcheck disable=SC2086 + pre-commit run --files $files || true + echo "✅ Formatting applied to staged files"
73-82: Ensure base ref exists locally for check-pr.Fetch the base ref to make origin/$BASE_REF diff robust on fresh clones.
BASE_REF="${GITHUB_BASE_REF:-main}" + git fetch --quiet origin "$BASE_REF" --depth=1 || true # Get changed Go files CHANGED_GO_FILES=$(git diff origin/$BASE_REF...HEAD --name-only -- '*.go' | grep -v -E '^(vendor/|tests/test-cases/|tests/testdata/|tests/snapshots/)' || true)
61-66: Optional: early-exit when nothing is staged.Small UX polish; avoid running tools needlessly.
- if [ -n "$STAGED_GO_FILES" ]; then + if [ -n "$STAGED_GO_FILES" ]; then echo "Checking Go formatting..." echo "$STAGED_GO_FILES" | xargs -I {} gofumpt -d {} 2>/dev/null | head -20 || true echo "Running golangci-lint..." golangci-lint run --new-from-rev=origin/main --config=.golangci.yml $STAGED_GO_FILES || true + else + echo "No staged Go files." fi
41-46: Pin gofumpt for reproducible formatting.Avoid @latest drift across machines/CI.
- go install mvdan.cc/gofumpt@latest + # Consider pinning to a known-good version to keep formatting stable across environments. + go install mvdan.cc/[email protected]
29-36: Pin golangci-lint installer to a version.Reduce nondeterminism and surprise lint diffs.
- else - echo "Installing golangci-lint via curl..." - curl -sSfL https://raw.githubusercontent.com/golangci/golangci-lint/master/install.sh | sh -s -- -b $(go env GOPATH)/bin + else + echo "Installing golangci-lint via curl (pinned)..." + GOLANGCI_LINT_VERSION="${GOLANGCI_LINT_VERSION:-v1.61.0}" + curl -sSfL https://raw.githubusercontent.com/golangci/golangci-lint/master/install.sh \ + | sh -s -- -b "$(go env GOPATH)/bin" "$GOLANGCI_LINT_VERSION"
203-211: Typo: “go-fumpt” → “gofumpt”.User-facing text polish.
- echo " - go-fumpt: Go code formatting" + echo " - gofumpt: Go code formatting"
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
.atmos.d/dev.yaml(1 hunks)pkg/config/load.go(1 hunks)tests/cli_test.go(2 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/config/load.gotests/cli_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: 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:
tests/cli_test.go
tests/**/*_test.go
📄 CodeRabbit inference engine (CLAUDE.md)
Integration tests reside under tests/ using fixtures in tests/test-cases/.
Files:
tests/cli_test.go
🧠 Learnings (9)
📓 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: 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:
pkg/config/load.go
📚 Learning: 2025-09-07T15:13:17.797Z
Learnt from: CR
PR: cloudposse/atmos#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-07T15:13:17.797Z
Learning: Applies to **/*.go : Always bind environment variables with viper.BindEnv() and provide ATMOS_ alternatives for each external var.
Applied to files:
pkg/config/load.go
📚 Learning: 2025-05-23T19:51:47.091Z
Learnt from: samtholiya
PR: cloudposse/atmos#1255
File: cmd/describe_affected_test.go:15-15
Timestamp: 2025-05-23T19:51:47.091Z
Learning: The atmos codebase has a custom extension to *testing.T that provides a Chdir method, allowing test functions to call t.Chdir() to change working directories during tests. This is used consistently across test files in the codebase.
Applied to files:
pkg/config/load.gotests/cli_test.go
📚 Learning: 2024-12-11T18:40:12.808Z
Learnt from: Listener430
PR: cloudposse/atmos#844
File: cmd/helmfile.go:37-37
Timestamp: 2024-12-11T18:40:12.808Z
Learning: In the atmos project, `cliConfig` is initialized within the `cmd` package in `root.go` and can be used in other command files.
Applied to files:
tests/cli_test.go
📚 Learning: 2025-05-23T19:51:47.091Z
Learnt from: samtholiya
PR: cloudposse/atmos#1255
File: cmd/describe_affected_test.go:15-15
Timestamp: 2025-05-23T19:51:47.091Z
Learning: In the atmos codebase, t.Chdir() is a valid method call on *testing.T objects and works correctly for changing directories in tests. This is implemented through custom testing framework extensions and is used consistently throughout the test suite.
Applied to files:
tests/cli_test.go
📚 Learning: 2025-05-23T19:51:47.091Z
Learnt from: samtholiya
PR: cloudposse/atmos#1255
File: cmd/describe_affected_test.go:15-15
Timestamp: 2025-05-23T19:51:47.091Z
Learning: In the atmos codebase, t.Chdir() is a valid method that can be called on *testing.T objects. This functionality is implemented through custom testing framework extensions and is used consistently throughout the test suite for changing working directories during tests.
Applied to files:
tests/cli_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 : Test both happy paths and error conditions
Applied to files:
tests/cli_test.go
📚 Learning: 2025-08-15T14:43:41.030Z
Learnt from: aknysh
PR: cloudposse/atmos#1352
File: pkg/store/artifactory_store_test.go:108-113
Timestamp: 2025-08-15T14:43:41.030Z
Learning: In test files for the atmos project, it's acceptable to ignore errors from os.Setenv/Unsetenv operations during test environment setup and teardown, as these are controlled test scenarios.
Applied to files:
tests/cli_test.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Build (macos-latest, macos)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Run pre-commit hooks
- GitHub Check: Lint (golangci)
- GitHub Check: Analyze (go)
- GitHub Check: Summary
🔇 Additional comments (3)
tests/cli_test.go (2)
44-44: LGTM: repoRoot at package scope.This simplifies reuse across tests and avoids recomputing.
605-608: Verify Windows path-list parsing and update comment punctuation
- Confirm that in pkg/config/load.go the value of ATMOS_TEST_EXCLUDE_DEFAULT_IMPORTS is split using filepath.SplitList (os.PathListSeparator) rather than a hard-coded “:”, so drive letters (e.g. C:\…) aren’t mis-parsed.
- In tests/cli_test.go (lines 605–608), add trailing periods to both comment lines:
// Exclude the repository root's .atmos.d from being loaded during tests. // This prevents test pollution from development-specific configurations..atmos.d/dev.yaml (1)
115-129: Verify “format-all” exclusions are honored by pre-commit config.The echoes list exclusions, but pre-commit run --all-files will still touch files unless hooks have matching excludes in .pre-commit-config.yaml.
Would you confirm those paths are excluded in the hook configs (e.g., files/exclude per hook)? If not, we should add them there to make this command safe.
- Fix path comparison to exclude repository root directory instead of .atmos.d subdirectory - This ensures test fixtures' .atmos.d directories still work while excluding the repo's .atmos.d 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
- Add TEST_GIT_ROOT environment variable check in ProcessTagGitRoot - Set TEST_GIT_ROOT to test scenario directory in CLI tests - Remove ATMOS_TEST_EXCLUDE_DEFAULT_IMPORTS exclusion logic - This ensures each test scenario acts as its own Git repository - Prevents repository root .atmos.d from polluting test outputs
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/utils/git.go (1)
14-15: Fix exported function doc: wrong name and missing period (will trip linters).Comment should start with the function name and end with a period.
Replace with:
// ProcessTagGitRoot returns the Git repository root (optionally honoring ATMOS_TEST_GIT_ROOT/TEST_GIT_ROOT) for Atmos tag evaluation.
🧹 Nitpick comments (2)
pkg/utils/git.go (2)
34-37: Add trailing periods to comments to satisfy godot.Minor, but pre-commit will flag these.
- Line 34: “// Open the repository starting from the given path.”
- Line 45: “// Get the worktree to extract the repository's root directory.”
- Line 54: “// Return the absolute path to the root directory.”
Also applies to: 45-47, 54-56
43-44: Adopt sentinel errors and wrap, per repo guidance.Current returns build dynamic errors. Define static sentinels and wrap.
At top of file:
var ( ErrOpenGitRepo = errors.New("open git repository") ErrGetWorktree = errors.New("get worktree") ErrResolveAbs = errors.New("resolve absolute path") ErrResolveTest = errors.New("resolve test git root") )Then, for example:
return "", fmt.Errorf("%w: %w", ErrOpenGitRepo, err) return "", fmt.Errorf("%w: %w", ErrGetWorktree, err) return "", fmt.Errorf("%w: %w", ErrResolveAbs, err)And in the new override block:
return "", fmt.Errorf("%w: %w", ErrResolveTest, err)Also applies to: 52-53, 61-62
📜 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/utils/git.go(1 hunks)tests/cli_test.go(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- pkg/config/load.go
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/cli_test.go
🧰 Additional context used
📓 Path-based instructions (1)
**/*.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/git.go
🧠 Learnings (1)
📓 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.
Learnt from: samtholiya
PR: cloudposse/atmos#1466
File: cmd/markdown/atmos_toolchain_aliases.md:2-4
Timestamp: 2025-09-13T16:39:20.007Z
Learning: In the cloudposse/atmos repository, CLI documentation files in cmd/markdown/ follow a specific format that uses " $ atmos command" (with leading space and dollar sign prompt) in code blocks. This is the established project convention and should not be changed to comply with standard markdownlint rules MD040 and MD014.
Learnt from: osterman
PR: cloudposse/atmos#1466
File: website/docs/cli/commands/toolchain/usage.mdx:117-121
Timestamp: 2025-09-08T01:25:44.958Z
Learning: The atmos toolchain XDG compliance implementation is complete with GetXDGCacheDir() and GetXDGTempCacheDir() functions in toolchain/xdg_cache.go, updated installer.go and toolchain_clean.go to use these helpers, and changed cache paths from ~/.cache/tools-cache to ${XDG_CACHE_HOME}/atmos-toolchain (or ~/.cache/atmos-toolchain when XDG_CACHE_HOME is not set).
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Build (macos-latest, macos)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Build (ubuntu-latest, linux)
- GitHub Check: Analyze (go)
- GitHub Check: Lint (golangci)
- GitHub Check: Run pre-commit hooks
- GitHub Check: Summary
🔇 Additional comments (1)
pkg/utils/git.go (1)
19-25: Use viper-bound env and return absolute path (keep TEST_GIT_ROOT compatibility)Bind ATMOS_TEST_GIT_ROOT and TEST_GIT_ROOT during config initialization (use the repo's bindEnv helper in pkg/config/load.go rather than viper.AutomaticEnv); in pkg/utils/git.go replace the os.Getenv read with strings.TrimSpace(viper.GetString("test_git_root")), resolve it with filepath.Abs and return the absolute path, and remove the nolint:forbidigo comment. Ensure the env binding runs before this code is invoked — tests set TEST_GIT_ROOT (tests/cli_test.go:610), so keep that name for compatibility.
- Add TEST_EXCLUDE_ATMOS_D environment variable to exclude repository .atmos.d - This fixes tests that use 'workdir: "../"' which change to the repo root - Silently skip loading .atmos.d from excluded directories - Fixes failing log level and log file priority tests in CI
- Fix TEST_EXCLUDE_ATMOS_D to only exclude repository root, not test fixtures - Update log level tests to use priority-log-check scenario instead of '../' - This ensures test fixtures can use their own .atmos.d configurations - Removes dependency on parent directory for tests
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/test-cases/log-level-validation.yaml (2)
25-36: Make this test hermetic too (avoid../).Using
workdir: "../"risks picking up repo-level config/.atmos.d, causing flakiness. Prefer a fixture or explicitly exclude autoload.Two viable fixes (pick one):
- workdir: "../" + workdir: "fixtures/scenarios/priority-log-check"or add the test guard:
env: + TEST_EXCLUDE_ATMOS_D: "true" ATMOS_LOGS_LEVEL: XTrace CI: "true"
103-103: Fix typos: “priortized” → “prioritized”.Minor polish for test names/descriptions.
- - name: "Valid log level in env should be priortized over config" + - name: "Valid log level in env should be prioritized over config" ... - - name: "Valid log level in flag should be priortized over env and config" + - name: "Valid log level in flag should be prioritized over env and config" ... - - name: "Valid log file in env should be priortized over config" + - name: "Valid log file in env should be prioritized over config" ... - - name: "Valid log file in flag should be priortized over env and config" + - name: "Valid log file in flag should be prioritized over env and config"Also applies to: 122-122, 144-144, 165-165
📜 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/config/load.go(1 hunks)tests/test-cases/log-level-validation.yaml(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/config/load.go
🧰 Additional context used
📓 Path-based instructions (2)
tests/test-cases/**
📄 CodeRabbit inference engine (CLAUDE.md)
Never modify golden snapshot fixtures under tests/test-cases/ unless explicitly instructed.
Files:
tests/test-cases/log-level-validation.yaml
**/*.{yaml,yml}
📄 CodeRabbit inference engine (CLAUDE.md)
YAML configs should support Go templating using provided template functions.
Files:
tests/test-cases/log-level-validation.yaml
🧠 Learnings (1)
📓 Common learnings
Learnt from: osterman
PR: cloudposse/atmos#1466
File: website/docs/cli/commands/toolchain/usage.mdx:117-121
Timestamp: 2025-09-08T01:25:44.958Z
Learning: The atmos toolchain XDG compliance implementation is complete with GetXDGCacheDir() and GetXDGTempCacheDir() functions in toolchain/xdg_cache.go, updated installer.go and toolchain_clean.go to use these helpers, and changed cache paths from ~/.cache/tools-cache to ${XDG_CACHE_HOME}/atmos-toolchain (or ~/.cache/atmos-toolchain when XDG_CACHE_HOME is not set).
Learnt from: osterman
PR: cloudposse/atmos#1466
File: website/docs/cli/commands/toolchain/usage.mdx:117-121
Timestamp: 2025-09-08T01:25:44.958Z
Learning: The atmos toolchain has been updated to follow XDG Base Directory Specification with helper functions GetXDGCacheDir() and GetXDGTempCacheDir() in toolchain/xdg_cache.go, using XDG_CACHE_HOME when set and falling back to ~/.cache/atmos-toolchain, making it consistent with atmos core's XDG compliance.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Build (macos-latest, macos)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Build (ubuntu-latest, linux)
- GitHub Check: Analyze (go)
- GitHub Check: Lint (golangci)
- GitHub Check: Run pre-commit hooks
- GitHub Check: Summary
🔇 Additional comments (1)
tests/test-cases/log-level-validation.yaml (1)
65-65: Good move: fixture-scoped workdirs harden test isolation.Pointing to fixtures/scenarios/priority-log-check prevents accidental bleed-in from repo-level config or .atmos.d autoload. Looks solid.
Confirm these fixtures include only what’s necessary for log-level precedence so future changes to global config can’t affect results.
Also applies to: 86-86, 107-107, 126-126, 147-147, 168-168
- Atmos already provides built-in help for custom commands - Custom commands from .atmos.d are automatically documented - Users can run 'atmos --help' to see all available commands
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1469 +/- ##
==========================================
+ Coverage 55.94% 55.99% +0.05%
==========================================
Files 274 274
Lines 28936 28976 +40
==========================================
+ Hits 16189 16226 +37
+ Misses 10955 10951 -4
- Partials 1792 1799 +7
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:
|
- Use os.PathListSeparator for splitting multiple paths - Add filepath.Clean for path canonicalization - Implement case-insensitive comparison on Windows - Add empty path check to skip invalid entries - Fix all comments to end with periods - Better handle path canonicalization errors
- Add tests for exact path matching behavior - Add tests for multiple exclude paths with OS path separator - Add tests for path canonicalization with relative paths - Add tests for Windows case-insensitive path comparison - Add tests for empty and invalid path handling - Increase test coverage to meet project requirements These tests ensure the TEST_EXCLUDE_ATMOS_D environment variable properly controls .atmos.d directory exclusion during test runs.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
pkg/config/load_test.go (2)
6-6: Prefer OS-specific test file; avoid runtime checks/import.Move the Windows-only test into a _windows_test.go file with //go:build windows. This drops the runtime import and removes the need to skip at runtime.
Outside this hunk, add a new file:
// pkg/config/load_windows_test.go //go:build windows package config import ( "strings" "testing" "github.com/spf13/viper" ) func TestMergeDefaultImports_WindowsCaseInsensitive(t *testing.T) { tempDir := t.TempDir() upper := strings.ToUpper(tempDir) lower := strings.ToLower(tempDir) t.Setenv("TEST_EXCLUDE_ATMOS_D", lower) v := viper.New() if err := mergeDefaultImports(upper, v); err != nil { t.Fatalf("Should match case-insensitively on Windows: %v", err) } }
349-391: Use t.Setenv for env isolation; drop manual Unsetenv.t.Setenv auto-restores and reduces boilerplate/leaks across cases.
@@ - if tt.excludePaths != "" { - os.Setenv("TEST_EXCLUDE_ATMOS_D", tt.excludePaths) - } - defer os.Unsetenv("TEST_EXCLUDE_ATMOS_D") + if tt.excludePaths != "" { + t.Setenv("TEST_EXCLUDE_ATMOS_D", tt.excludePaths) + }
📜 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/config/load_test.go(2 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/config/load_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: 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/config/load_test.go
pkg/**/*_test.go
📄 CodeRabbit inference engine (CLAUDE.md)
Unit tests reside under pkg/ alongside implementations.
Files:
pkg/config/load_test.go
🧠 Learnings (8)
📓 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.
Learnt from: osterman
PR: cloudposse/atmos#1466
File: website/docs/cli/commands/toolchain/usage.mdx:117-121
Timestamp: 2025-09-08T01:25:44.958Z
Learning: The atmos toolchain has been updated to follow XDG Base Directory Specification with helper functions GetXDGCacheDir() and GetXDGTempCacheDir() in toolchain/xdg_cache.go, using XDG_CACHE_HOME when set and falling back to ~/.cache/atmos-toolchain, making it consistent with atmos core's XDG compliance.
Learnt from: osterman
PR: cloudposse/atmos#1466
File: website/docs/cli/commands/toolchain/usage.mdx:117-121
Timestamp: 2025-09-08T01:25:44.958Z
Learning: The atmos toolchain XDG compliance implementation is complete with GetXDGCacheDir() and GetXDGTempCacheDir() functions in toolchain/xdg_cache.go, updated installer.go and toolchain_clean.go to use these helpers, and changed cache paths from ~/.cache/tools-cache to ${XDG_CACHE_HOME}/atmos-toolchain (or ~/.cache/atmos-toolchain when XDG_CACHE_HOME is not set).
📚 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/config/load_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 pkg/**/*_test.go : Unit tests reside under pkg/ alongside implementations.
Applied to files:
pkg/config/load_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 : Include integration tests for command flows
Applied to files:
pkg/config/load_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 **/*.go : Use Viper for managing configuration, environment variables, and flags
Applied to files:
pkg/config/load_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 **/*.go : Use Viper for configuration management
Applied to files:
pkg/config/load_test.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:
pkg/config/load_test.go
📚 Learning: 2025-05-23T19:51:47.091Z
Learnt from: samtholiya
PR: cloudposse/atmos#1255
File: cmd/describe_affected_test.go:15-15
Timestamp: 2025-05-23T19:51:47.091Z
Learning: The atmos codebase has a custom extension to *testing.T that provides a Chdir method, allowing test functions to call t.Chdir() to change working directories during tests. This is used consistently across test files in the codebase.
Applied to files:
pkg/config/load_test.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Build (ubuntu-latest, linux)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Lint (golangci)
- GitHub Check: Analyze (go)
- GitHub Check: Run pre-commit hooks
- GitHub Check: Summary
…oss-platform support
- Fix Windows-specific test to use t.Skipf with format string as per repo conventions
- Add strings import for ToUpper/ToLower path case variation on Windows
- Fix path canonicalization test to use table inputs directly (tt.dirPath and tt.excludePath)
- Add sentinel file testing to verify actual exclusion behavior
- Set viper.SetConfigType("yaml") in all tests to match production usage
- Create .atmos.d/sentinel.yaml files with unique keys to test import/skip behavior
- Assert viper.IsSet() to verify whether imports were actually processed or skipped
These changes ensure the tests properly validate the exclusion logic and work
correctly across all platforms.
- Update golangci-lint to exit 1 on failure instead of just printing warning - Update go vet fallback to also exit 1 on failure - Keep descriptive error messages but ensure validation fails on any lint/vet issues This ensures the validation pipeline fails fast when code quality issues are detected, preventing broken code from passing validation.
…ng function - Extract complex test exclusion logic from mergeDefaultImports into dedicated function - Add comprehensive test coverage for shouldExcludePathForTesting function - Test cases cover: empty env, single/multiple paths, path canonicalization, Windows case-insensitivity - Verify exact match behavior (subdirectories and parent directories not matched) - Handle edge cases like empty entries, relative paths, and path normalization This refactoring reduces function complexity and improves testability while maintaining the same behavior for test isolation during Atmos configuration loading.
The severity configuration was causing 'no default severity defined' error in CI. Removed the severity section and inline severity warnings from revive rules as they were using an incorrect format that's not compatible with the current golangci-lint version. This fixes the pre-commit hook failures in GitHub Actions.
The check-yaml hook rejects custom YAML tags like !repo-root, !terraform.output as a security measure to prevent arbitrary code execution from untrusted YAML. However, the --unsafe flag switches to syntax-only parsing without object construction. This is the correct solution because: 1. It validates YAML syntax without rejecting valid custom tags 2. It's actually safe since we're only parsing syntax, not executing code 3. It allows checking ALL files including tests and examples 4. Atmos custom tags are intentional features, not security risks The --unsafe name is misleading - it only means 'allows non-standard YAML constructs' but doesn't create security risks since it's just parsing, not executing.
…dev' The previous structure was incorrect - it was creating top-level commands instead of subcommands under 'atmos dev'. Fixed by: 1. Removing top-level 'name: dev' and 'description' fields 2. Moving to standard 'commands:' structure with nested subcommands 3. Fixing all indentation for steps content Now all dev commands properly appear under 'atmos dev' namespace: - atmos dev setup - atmos dev check - atmos dev format-pr - etc. This provides better organization and prevents cluttering the top-level command namespace.
|
💥 This pull request now has conflicts. Could you fix it @osterman? 🙏 |
what
.atmos.d/dev.yamlwhy
go-build-modhook ensures code compiles before allowing commitsgofumptand linting withgolangci-lintatmos devcommands for setup and validationChanges
Pre-commit Configuration
.pre-commit-config.yaml: Hooks for gofumpt, go-build-mod, golangci-lint, go-mod-tidy, and file hygieneDevelopment Workflow
.atmos.d/dev.yaml: Auto-loaded development commandsatmos dev setup- One-time setup for contributorsatmos dev check- Run pre-commit on staged filesatmos dev validate- Comprehensive validation (build, lint, test)atmos dev fix- Auto-fix formatting issuesCI Integration
.github/workflows/pre-commit.yml: GitHub Action usingcloudposse/[email protected]Documentation
docs/development.md: Comprehensive developer guide.atmos.d/README.md: Explains auto-loaded configurationsREADME.mdwith development sectionTesting
atmos devcommands work correctly.atmos.d/auto-loading functionalityreferences
github-action-pre-commitfor CI consistency.atmos.d/directory🤖 Generated with Claude Code
Summary by CodeRabbit
Documentation
Chores
Tests