Skip to content

fix: normalize path separators in terraform clean output#1523

Merged
aknysh merged 2 commits intomainfrom
fix-clean-path-separators
Sep 26, 2025
Merged

fix: normalize path separators in terraform clean output#1523
aknysh merged 2 commits intomainfrom
fix-clean-path-separators

Conversation

@osterman
Copy link
Member

@osterman osterman commented Sep 25, 2025

what

  • Normalize path separators to forward slashes in terraform clean command output
  • Add static error for symbolic link deletion to satisfy linting requirements

why

  • Windows tests were failing due to path separator mismatches in output
  • The clean command was displaying paths with backslashes on Windows (e.g., basic/components\terraform\mock\.terraform/)
  • Test assertions expected forward slashes, causing failures on Windows CI
  • Using filepath.ToSlash() ensures consistent output across all platforms

references

  • Fixes Windows test failures in CI
  • Addresses path display inconsistency in terraform clean command

🤖 Generated with Claude Code

Summary by CodeRabbit

  • New Features
    • Shows a clear, user-facing error when attempting to delete a symbolic link, preventing unintended deletion.
  • Bug Fixes
    • Standardizes path formatting to forward slashes in delete operations for consistent output across platforms.
    • All success and error messages in delete operations now display normalized paths for clarity.

…atform consistency

Ensure paths in terraform clean command output use forward slashes on all platforms
by normalizing with filepath.ToSlash(). This fixes test failures on Windows where
backslashes in paths caused output mismatches.

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <noreply@anthropic.com>
@osterman osterman requested a review from a team as a code owner September 25, 2025 20:57
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Sep 25, 2025

📝 Walkthrough

Walkthrough

Adds a new exported error sentinel ErrRefuseDeleteSymbolicLink. Updates DeletePathTerraform to normalize paths to forward slashes for messages and to wrap symlink deletion refusals with the new sentinel error, ensuring consistent, user-facing output.

Changes

Cohort / File(s) Summary
Errors sentinel
errors/errors.go
Adds exported variable ErrRefuseDeleteSymbolicLink with message "refusing to delete symbolic link".
Terraform clean behavior
internal/exec/terraform_clean.go
Normalizes displayed paths to forward slashes; imports github.com/cloudposse/atmos/errors as errUtils; wraps symlink-delete refusal with errUtils.ErrRefuseDeleteSymbolicLink; uses normalized path in all messages.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant U as User
  participant D as DeletePathTerraform
  participant FS as Filesystem

  U->>D: Request delete path (objectName)
  rect rgb(240,240,255)
    note over D: Normalize objectName -> normalizedPath (forward slashes)
  end
  D->>FS: Inspect path
  alt Path is symlink
    D-->>U: error (wrapped with ErrRefuseDeleteSymbolicLink, includes normalizedPath)
  else Regular path
    D->>FS: Delete path
    FS-->>D: Result
    D-->>U: Success/error message using normalizedPath
  end
Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

Suggested reviewers

  • osterman

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The title clearly states the main change—normalizing path separators in the terraform clean output—and accurately reflects the core modification in the PR. It is concise, specific, and follows conventional style with a ‘fix:’ prefix. While it does not mention the new error sentinel for symbolic link deletion, PR titles need not enumerate every detail. A teammate scanning PR history would understand the primary purpose from this title.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
✨ Finishing touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix-clean-path-separators

Tip

👮 Agentic pre-merge checks are now available in preview!

Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.

  • Built-in checks – Quickly apply ready-made checks to enforce title conventions, require pull request descriptions that follow templates, validate linked issues for compliance, and more.
  • Custom agentic checks – Define your own rules using CodeRabbit’s advanced agentic capabilities to enforce organization-specific policies and workflows. For example, you can instruct CodeRabbit’s agent to verify that API documentation is updated whenever API schema files are modified in a PR. Note: Upto 5 custom checks are currently allowed during the preview period. Pricing for this feature will be announced in a few weeks.

Please see the documentation for more information.

Example:

reviews:
  pre_merge_checks:
    custom_checks:
      - name: "Undocumented Breaking Changes"
        mode: "warning"
        instructions: |
          Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).

Please share your feedback with us on this Discord post.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions github-actions bot added the size/s Small size PR label Sep 25, 2025
@osterman osterman added the patch A minor, backward compatible change label Sep 25, 2025
@codecov
Copy link

codecov bot commented Sep 25, 2025

Codecov Report

❌ Patch coverage is 85.71429% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 57.07%. Comparing base (e0d7cd7) to head (71e1ce5).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
internal/exec/terraform_clean.go 85.71% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1523      +/-   ##
==========================================
+ Coverage   56.94%   57.07%   +0.13%     
==========================================
  Files         286      286              
  Lines       30592    30595       +3     
==========================================
+ Hits        17420    17463      +43     
+ Misses      11334    11291      -43     
- Partials     1838     1841       +3     
Flag Coverage Δ
unittests 57.07% <85.71%> (+0.13%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

📜 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.

📥 Commits

Reviewing files that changed from the base of the PR and between e0d7cd7 and 71e1ce5.

📒 Files selected for processing (2)
  • errors/errors.go (1 hunks)
  • internal/exec/terraform_clean.go (2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.go

📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)

**/*.go: All code must pass golangci-lint checks
Follow Go error handling idioms and use meaningful error messages
Wrap errors with context using fmt.Errorf("context: %w", err)
Consider custom error types for domain-specific errors
Follow standard Go coding style; run gofmt and goimports
Use snake_case for environment variables
Document complex logic with inline comments

**/*.go: All Go comments must end with periods (enforced by golangci-lint's godot).
Wrap all errors with static errors defined in errors/errors.go; never return dynamic errors directly; use fmt.Errorf with %w and add details after the static error.
Always bind environment variables using viper.BindEnv and provide an ATMOS_ alternative for each external env var.
Use structured logging for system/debug events; logging must not affect execution and should use appropriate levels per docs/logging.md.
Prefer SDKs over shelling out to binaries for cross-platform compatibility; use filepath/os/runtime for portable paths and OS-specific logic.

Files:

  • errors/errors.go
  • internal/exec/terraform_clean.go
**/!(*_test).go

📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)

Document all exported functions, types, and methods with Go doc comments

Files:

  • errors/errors.go
  • internal/exec/terraform_clean.go
errors/errors.go

📄 CodeRabbit inference engine (CLAUDE.md)

Define all static errors centrally in errors/errors.go (e.g., ErrInvalidComponent, ErrInvalidStack, ErrInvalidConfig).

Files:

  • errors/errors.go
🧠 Learnings (6)
📚 Learning: 2025-04-26T15:54:10.506Z
Learnt from: haitham911
PR: cloudposse/atmos#1195
File: internal/exec/terraform_clean.go:99-99
Timestamp: 2025-04-26T15:54:10.506Z
Learning: The error variable `ErrRelPath` is defined in `internal/exec/terraform_clean_util.go` and is used across files in the `exec` package, including in `terraform_clean.go`. This is part of an approach to standardize error handling in the codebase.

Applied to files:

  • internal/exec/terraform_clean.go
📚 Learning: 2024-10-28T01:51:30.811Z
Learnt from: osterman
PR: cloudposse/atmos#727
File: internal/exec/terraform_clean.go:329-332
Timestamp: 2024-10-28T01:51:30.811Z
Learning: In the Atmos Go code, when deleting directories or handling file paths (e.g., in `terraform_clean.go`), always resolve the absolute path using `filepath.Abs` and use the logger `u.LogWarning` for logging messages instead of using `fmt.Printf`.

Applied to files:

  • internal/exec/terraform_clean.go
📚 Learning: 2024-10-27T04:41:49.199Z
Learnt from: haitham911
PR: cloudposse/atmos#727
File: internal/exec/terraform_clean.go:215-223
Timestamp: 2024-10-27T04:41:49.199Z
Learning: In `internal/exec/terraform_clean.go`, the function `determineCleanPath` is necessary and should not be removed.

Applied to files:

  • internal/exec/terraform_clean.go
📚 Learning: 2024-11-02T15:35:09.958Z
Learnt from: aknysh
PR: cloudposse/atmos#759
File: internal/exec/terraform.go:366-368
Timestamp: 2024-11-02T15:35:09.958Z
Learning: In `internal/exec/terraform.go`, the workspace cleaning code under both the general execution path and within the `case "init":` block is intentionally duplicated because the code execution paths are different. The `.terraform/environment` file should be deleted before executing `terraform init` in both scenarios to ensure a clean state.

Applied to files:

  • internal/exec/terraform_clean.go
📚 Learning: 2025-09-13T18:06:07.674Z
Learnt from: samtholiya
PR: cloudposse/atmos#1466
File: toolchain/list.go:39-42
Timestamp: 2025-09-13T18:06:07.674Z
Learning: In the cloudposse/atmos repository, for UI messages in the toolchain package, use utils.PrintfMessageToTUI instead of log.Error or fmt.Fprintln(os.Stderr, ...). Import pkg/utils with alias "u" to follow the established pattern.

Applied to files:

  • internal/exec/terraform_clean.go
📚 Learning: 2025-07-05T20:59:02.914Z
Learnt from: aknysh
PR: cloudposse/atmos#1363
File: internal/exec/template_utils.go:18-18
Timestamp: 2025-07-05T20:59:02.914Z
Learning: In the Atmos project, gomplate v4 is imported with a blank import (`_ "github.com/hairyhenderson/gomplate/v4"`) alongside v3 imports to resolve AWS SDK version conflicts. V3 uses older AWS SDK versions that conflict with newer AWS modules used by Atmos. A full migration to v4 requires extensive refactoring due to API changes and should be handled in a separate PR.

Applied to files:

  • internal/exec/terraform_clean.go
🧬 Code graph analysis (1)
internal/exec/terraform_clean.go (2)
pkg/ui/theme/colors.go (1)
  • Styles (37-69)
errors/errors.go (1)
  • ErrRefuseDeleteSymbolicLink (35-35)
⏰ 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 (windows-latest, windows)
  • GitHub Check: Run pre-commit hooks
  • GitHub Check: Lint (golangci)
  • GitHub Check: Analyze (go)
  • GitHub Check: Summary
🔇 Additional comments (1)
errors/errors.go (1)

35-35: Solid sentinel addition. Keeps symlink deletions guarded with a reusable, well-named static error—matches the rest of the catalogue neatly.

@aknysh aknysh added no-release Do not create a new release (wait for additional code changes) and removed patch A minor, backward compatible change labels Sep 26, 2025
@aknysh aknysh merged commit e8c1aed into main Sep 26, 2025
66 checks passed
@aknysh aknysh deleted the fix-clean-path-separators branch September 26, 2025 01:48
@github-actions
Copy link

These changes were released in v1.192.0.

@github-actions
Copy link

These changes were released in v1.193.0-test.1.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

no-release Do not create a new release (wait for additional code changes) size/s Small size PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants