Fix atmos terraform clean command for Terraform components in sub-directories
#1195
Conversation
📝 WalkthroughWalkthroughThis update introduces standardized error handling and refactors the logic for cleaning Terraform component directories. Several new error variables are added for consistent error reporting. The deletion process now uses relative paths for more accurate file operations. The collection of directories and files to be cleaned is improved by leveraging stack descriptions and new utility functions. Additionally, comprehensive tests and supporting configuration files are included to validate the cleaning functionality in Terraform scenarios. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant Exec
participant Util
User->>CLI: Run 'terraform clean'
CLI->>Exec: handleCleanSubCommand()
Exec->>Exec: ExecuteDescribeStacks()
Exec->>Util: getAllStacksComponentsPaths()
Util-->>Exec: List of component paths
Exec->>Util: CollectComponentsDirectoryObjects()
Util-->>Exec: Directory objects with files
Exec->>Exec: deleteFolders() using relative paths
Exec->>CLI: Return result
CLI-->>User: Output success/failure
Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches
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:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1195 +/- ##
==========================================
+ Coverage 31.43% 32.60% +1.16%
==========================================
Files 208 209 +1
Lines 23189 23321 +132
==========================================
+ Hits 7290 7603 +313
+ Misses 14818 14574 -244
- Partials 1081 1144 +63
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:
|
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
internal/exec/terraform_clean_test.go (1)
13-24: 🛠️ Refactor suggestionConsider using viper.BindEnv.
This PR references a policy that forbids direct usage ofos.Getenvfor new environment variables. If feasible, switch toviper.BindEnvfor consistency.- if os.Getenv("ATMOS_CLI_CONFIG_PATH") != "" { + if viper.GetString("ATMOS_CLI_CONFIG_PATH") != "" { ...🧰 Tools
🪛 GitHub Check: golangci-lint
[failure] 13-13:
use ofos.Getenvforbidden because "Useviper.BindEnvfor new environment variables instead ofos.Getenv"
[failure] 19-19:
use ofos.Getenvforbidden because "Useviper.BindEnvfor new environment variables instead ofos.Getenv"
🧹 Nitpick comments (10)
tests/fixtures/scenarios/terraform-sub-components/stacks/deploy/staging.yaml (1)
9-9: Fix indentation.
Please ensure consistent YAML indentation to avoid potential parse errors.9- metadata: 9+ metadata:internal/exec/terraform_clean_test.go (2)
79-79: Run code formatter.
Lines 79 and 89 suggest formatting issues reported by linters. Consider runninggofmtorgofumptto ensure consistent formatting.Also applies to: 89-89
🧰 Tools
🪛 golangci-lint (1.64.8)
[error] 79-79: File is not properly formatted
(gofumpt)
81-81: Drop unused parameter.
t *testing.Tis not utilized inverifyFileDeleted. Consider removing it from the function signature.-func verifyFileDeleted(t *testing.T, files []string) (bool, string) { +func verifyFileDeleted(files []string) (bool, string) {🧰 Tools
🪛 GitHub Check: golangci-lint
[failure] 81-81:
verifyFileDeleted-tis unusedinternal/exec/terraform_clean.go (7)
310-314: Consider using wrapped static errors.
Line 312 usesfmt.Errorfdynamically. A static error or wrapping with%wmight be preferable for consistency.-log.Debug(fmt.Errorf("failed to get relative path for %s: %w", file.FullPath, err)) +log.Debug(fmt.Errorf("failed to get relative path for %s: %w", file.FullPath, err))(Consider capturing this error in a variable or static form if used repeatedly.)
433-433: Use wrapped or static error.
Avoid dynamically building error messages. Prefer using%wfor error wrapping or referencing a static error var.-return fmt.Errorf("error executing describe stacks: %v", err) +return fmt.Errorf("error executing describe stacks: %w", err)🧰 Tools
🪛 GitHub Check: golangci-lint
[failure] 433-433:
do not define dynamic errors, use wrapped static errors instead: "fmt.Errorf("error executing describe stacks: %v", err)"
555-567: Refine error handling at line 561.
Consider replacing the dynamic error with a wrapped error using%w.-u.LogTrace(fmt.Errorf("error collecting folders and files: %v", err).Error()) +u.LogTrace(fmt.Errorf("error collecting folders and files: %w", err).Error())🧰 Tools
🪛 GitHub Check: golangci-lint
[failure] 561-561:
do not define dynamic errors, use wrapped static errors instead: "fmt.Errorf("error collecting folders and files: %v", err)"
590-598: Avoid dynamic error strings.
At lines 592 and 595, prefer static or wrapped errors if needed.-return fmt.Errorf("path cannot be empty") +return fmt.Errorf("path cannot be empty: %w", ErrParseStacks)(or keep them distinct if each error stands on its own.)
🧰 Tools
🪛 GitHub Check: golangci-lint
[failure] 592-592:
do not define dynamic errors, use wrapped static errors instead: "fmt.Errorf("path cannot be empty")"
[failure] 595-595:
do not define dynamic errors, use wrapped static errors instead: "fmt.Errorf("path %s does not exist", path)"
600-612: Leverage wrapped error.
At line 603, the error could be wrapped with%wto preserve call stack context.-return nil, fmt.Errorf("error determining relative path for folder %s: %v", folderPath, err) +return nil, fmt.Errorf("error determining relative path for folder %s: %w", folderPath, err)🧰 Tools
🪛 GitHub Check: golangci-lint
[failure] 603-603:
do not define dynamic errors, use wrapped static errors instead: "fmt.Errorf("error determining relative path for folder %s: %v", folderPath, err)"
614-632: Switch to wrapped errors.
At line 618, replacing%vwith%wcan improve error detail retention.-return fmt.Errorf("error matching pattern %s in folder %s: %v", pat, folderPath, err) +return fmt.Errorf("error matching pattern %s in folder %s: %w", pat, folderPath, err)🧰 Tools
🪛 GitHub Check: golangci-lint
[failure] 618-618:
do not define dynamic errors, use wrapped static errors instead: "fmt.Errorf("error matching pattern %s in folder %s: %v", pat, folderPath, err)"
634-654: Use%wfor improved error wrapping.
Lines 637 and 645 rely on dynamically built error messages. Also note the file length limit warning. Consider refactoring into smaller files.-return nil, fmt.Errorf("error determining relative path for %s: %v", filePath, err) +return nil, fmt.Errorf("error determining relative path for %s: %w", filePath, err) -return nil, fmt.Errorf("error stating file %s: %v", filePath, err) +return nil, fmt.Errorf("error stating file %s: %w", filePath, err)🧰 Tools
🪛 GitHub Check: golangci-lint
[failure] 637-637:
do not define dynamic errors, use wrapped static errors instead: "fmt.Errorf("error determining relative path for %s: %v", filePath, err)"
[failure] 645-645:
do not define dynamic errors, use wrapped static errors instead: "fmt.Errorf("error stating file %s: %v", filePath, err)"
[failure] 654-654:
file-length-limit: file length is 554 lines, which exceeds the limit of 500
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
internal/exec/terraform_clean.go(5 hunks)internal/exec/terraform_clean_test.go(1 hunks)tests/cli_terraform_test.go(0 hunks)tests/fixtures/scenarios/terraform-sub-components/atmos.yaml(1 hunks)tests/fixtures/scenarios/terraform-sub-components/stacks/deploy/staging.yaml(1 hunks)
💤 Files with no reviewable changes (1)
- tests/cli_terraform_test.go
🧰 Additional context used
🧠 Learnings (2)
internal/exec/terraform_clean_test.go (1)
Learnt from: haitham911
PR: cloudposse/atmos#727
File: internal/exec/terraform_clean.go:215-223
Timestamp: 2025-04-05T19:57:40.635Z
Learning: In `internal/exec/terraform_clean.go`, the function `determineCleanPath` is necessary and should not be removed.
internal/exec/terraform_clean.go (2)
Learnt from: haitham911
PR: cloudposse/atmos#727
File: internal/exec/terraform_clean.go:407-416
Timestamp: 2025-04-05T19:57:35.387Z
Learning: In `internal/exec/terraform_clean.go`, when `getStackTerraformStateFolder` returns an error in the `handleCleanSubCommand` function, the error is logged, and the process continues without returning the error.
Learnt from: haitham911
PR: cloudposse/atmos#727
File: internal/exec/terraform_clean.go:314-319
Timestamp: 2025-04-05T19:57:35.386Z
Learning: When deleting empty folders in the `deleteFolders` function, handling errors from `os.Remove` are not required, as failures do not affect the process.
🧬 Code Graph Analysis (1)
internal/exec/terraform_clean_test.go (3)
tests/cli_terraform_test.go (1)
TestCLITerraformClean(14-38)pkg/schema/schema.go (1)
ConfigAndStacksInfo(363-431)internal/exec/terraform.go (1)
ExecuteTerraform(39-520)
🪛 golangci-lint (1.64.8)
internal/exec/terraform_clean_test.go
[error] 79-79: File is not properly formatted
(gofumpt)
[error] 89-89: File is not properly formatted
(gofumpt)
internal/exec/terraform_clean.go
[error] 520-520: File is not properly formatted
(gofumpt)
[error] 552-552: File is not properly formatted
(gofumpt)
🪛 GitHub Check: golangci-lint
internal/exec/terraform_clean_test.go
[failure] 13-13:
use of os.Getenv forbidden because "Use viper.BindEnv for new environment variables instead of os.Getenv"
[failure] 19-19:
use of os.Getenv forbidden because "Use viper.BindEnv for new environment variables instead of os.Getenv"
[failure] 81-81:
verifyFileDeleted - t is unused
internal/exec/terraform_clean.go
[failure] 433-433:
do not define dynamic errors, use wrapped static errors instead: "fmt.Errorf("error executing describe stacks: %v", err)"
[failure] 561-561:
do not define dynamic errors, use wrapped static errors instead: "fmt.Errorf("error collecting folders and files: %v", err)"
[failure] 592-592:
do not define dynamic errors, use wrapped static errors instead: "fmt.Errorf("path cannot be empty")"
[failure] 595-595:
do not define dynamic errors, use wrapped static errors instead: "fmt.Errorf("path %s does not exist", path)"
[failure] 603-603:
do not define dynamic errors, use wrapped static errors instead: "fmt.Errorf("error determining relative path for folder %s: %v", folderPath, err)"
[failure] 618-618:
do not define dynamic errors, use wrapped static errors instead: "fmt.Errorf("error matching pattern %s in folder %s: %v", pat, folderPath, err)"
[failure] 637-637:
do not define dynamic errors, use wrapped static errors instead: "fmt.Errorf("error determining relative path for %s: %v", filePath, err)"
[failure] 645-645:
do not define dynamic errors, use wrapped static errors instead: "fmt.Errorf("error stating file %s: %v", filePath, err)"
[failure] 654-654:
file-length-limit: file length is 554 lines, which exceeds the limit of 500
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Acceptance Tests (macos-latest, macos)
- GitHub Check: Acceptance Tests (windows-latest, windows)
- GitHub Check: Acceptance Tests (ubuntu-latest, linux)
- GitHub Check: Summary
🔇 Additional comments (9)
tests/fixtures/scenarios/terraform-sub-components/atmos.yaml (1)
1-16: Looks good.
The configuration keys appear properly defined, and there's no obvious issue with indentation or structure.internal/exec/terraform_clean.go (8)
14-17: Imports look fine.
These import statements appear straightforward and do not introduce issues.
19-23: Static error variables look good.
Defining these errors as static is helpful for clarity and maintainability.
316-317: No issues.
These calls toDeletePathTerraformappear correct and align with the intended cleanup logic.Also applies to: 320-321
424-432: FilterComponents logic looks sound.
UsingExecuteDescribeStackswith filtered components is a clear approach.
435-435: Straightforward usage.
Fetching component paths from the map is a clean approach.
510-520: Helper function is well-structured.
getAllStacksComponentsPathscorrectly iterates over stacks.🧰 Tools
🪛 golangci-lint (1.64.8)
[error] 520-520: File is not properly formatted
(gofumpt)
522-553: Parsing logic is clear.
Returning these named errors (e.g.,ErrParseStacks) is neat and consistent.🧰 Tools
🪛 golangci-lint (1.64.8)
[error] 552-552: File is not properly formatted
(gofumpt)
569-581: Component object collection logic is fine.
No apparent issues inCollectComponentObjects.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
internal/exec/terraform_clean_test.go (1)
13-23: 🛠️ Refactor suggestionUse of
os.Getenvis disallowed by policy.Lines 13-23 rely on
os.Getenvto check environment variables. Per internal guidelines, you should useviper.BindEnvinstead.Example approach:
-if os.Getenv("ATMOS_CLI_CONFIG_PATH") != "" { - err := os.Unsetenv("ATMOS_CLI_CONFIG_PATH") - ... -} +import "github.com/spf13/viper" +envValue := viper.GetString("ATMOS_CLI_CONFIG_PATH") +if envValue != "" { + ... +}🧰 Tools
🪛 GitHub Check: golangci-lint
[failure] 13-13:
use ofos.Getenvforbidden because "Useviper.BindEnvfor new environment variables instead ofos.Getenv"
[failure] 19-19:
use ofos.Getenvforbidden because "Useviper.BindEnvfor new environment variables instead ofos.Getenv"
🧹 Nitpick comments (4)
tests/fixtures/scenarios/terraform-sub-components/stacks/deploy/staging.yaml (1)
9-10: Consider fixing inconsistent indentation.Line 9 is indented by 5 spaces while line 6 is indented by 6 spaces. YAML is sensitive to indentation levels and it may cause parsing issues.
Proposed fix:
- component-2: - metadata: - component: component-1/component-2 # Point to the Terraform component + component-2: + metadata: + component: component-1/component-2 # Point to the Terraform componentinternal/exec/terraform_clean_test.go (2)
79-79: Format the file to align with gofumpt rules.A lint error indicates this file is not properly formatted. Consider running a gofumpt or equivalent formatter.
🧰 Tools
🪛 golangci-lint (1.64.8)
[error] 79-79: File is not properly formatted
(gofumpt)
81-89: Parameter 't' is unused.The function
verifyFileDeleteddoes not utilize thetesting.Targument. This could lead to confusion or code smell.Proposed fix:
-func verifyFileDeleted(t *testing.T, files []string) (bool, string) { +func verifyFileDeleted(files []string) (bool, string) {🧰 Tools
🪛 golangci-lint (1.64.8)
[error] 89-89: File is not properly formatted
(gofumpt)
🪛 GitHub Check: golangci-lint
[failure] 81-81:
verifyFileDeleted-tis unusedinternal/exec/terraform_clean.go (1)
654-654: File exceeds recommended line limit.This file is now 554 lines, which surpasses the 500-line limit cited by lint. Consider splitting parts (e.g., directory collection, path validation) into separate files for better maintainability.
🧰 Tools
🪛 GitHub Check: golangci-lint
[failure] 654-654:
file-length-limit: file length is 554 lines, which exceeds the limit of 500
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
internal/exec/terraform_clean.go(5 hunks)internal/exec/terraform_clean_test.go(1 hunks)tests/cli_terraform_test.go(0 hunks)tests/fixtures/scenarios/terraform-sub-components/atmos.yaml(1 hunks)tests/fixtures/scenarios/terraform-sub-components/stacks/deploy/staging.yaml(1 hunks)
💤 Files with no reviewable changes (1)
- tests/cli_terraform_test.go
🧰 Additional context used
🧠 Learnings (2)
internal/exec/terraform_clean_test.go (1)
Learnt from: haitham911
PR: cloudposse/atmos#727
File: internal/exec/terraform_clean.go:215-223
Timestamp: 2025-04-05T19:57:40.635Z
Learning: In `internal/exec/terraform_clean.go`, the function `determineCleanPath` is necessary and should not be removed.
internal/exec/terraform_clean.go (2)
Learnt from: haitham911
PR: cloudposse/atmos#727
File: internal/exec/terraform_clean.go:407-416
Timestamp: 2025-04-05T19:57:35.387Z
Learning: In `internal/exec/terraform_clean.go`, when `getStackTerraformStateFolder` returns an error in the `handleCleanSubCommand` function, the error is logged, and the process continues without returning the error.
Learnt from: haitham911
PR: cloudposse/atmos#727
File: internal/exec/terraform_clean.go:314-319
Timestamp: 2025-04-05T19:57:35.386Z
Learning: When deleting empty folders in the `deleteFolders` function, handling errors from `os.Remove` are not required, as failures do not affect the process.
🧬 Code Graph Analysis (1)
internal/exec/terraform_clean.go (1)
internal/exec/describe_stacks.go (1)
ExecuteDescribeStacks(148-689)
🪛 golangci-lint (1.64.8)
internal/exec/terraform_clean_test.go
[error] 79-79: File is not properly formatted
(gofumpt)
[error] 89-89: File is not properly formatted
(gofumpt)
internal/exec/terraform_clean.go
[error] 520-520: File is not properly formatted
(gofumpt)
[error] 552-552: File is not properly formatted
(gofumpt)
🪛 GitHub Check: golangci-lint
internal/exec/terraform_clean_test.go
[failure] 13-13:
use of os.Getenv forbidden because "Use viper.BindEnv for new environment variables instead of os.Getenv"
[failure] 19-19:
use of os.Getenv forbidden because "Use viper.BindEnv for new environment variables instead of os.Getenv"
[failure] 81-81:
verifyFileDeleted - t is unused
internal/exec/terraform_clean.go
[failure] 433-433:
do not define dynamic errors, use wrapped static errors instead: "fmt.Errorf("error executing describe stacks: %v", err)"
[failure] 561-561:
do not define dynamic errors, use wrapped static errors instead: "fmt.Errorf("error collecting folders and files: %v", err)"
[failure] 592-592:
do not define dynamic errors, use wrapped static errors instead: "fmt.Errorf("path cannot be empty")"
[failure] 595-595:
do not define dynamic errors, use wrapped static errors instead: "fmt.Errorf("path %s does not exist", path)"
[failure] 603-603:
do not define dynamic errors, use wrapped static errors instead: "fmt.Errorf("error determining relative path for folder %s: %v", folderPath, err)"
[failure] 618-618:
do not define dynamic errors, use wrapped static errors instead: "fmt.Errorf("error matching pattern %s in folder %s: %v", pat, folderPath, err)"
[failure] 637-637:
do not define dynamic errors, use wrapped static errors instead: "fmt.Errorf("error determining relative path for %s: %v", filePath, err)"
[failure] 645-645:
do not define dynamic errors, use wrapped static errors instead: "fmt.Errorf("error stating file %s: %v", filePath, err)"
[failure] 654-654:
file-length-limit: file length is 554 lines, which exceeds the limit of 500
🪛 GitHub Check: codecov/patch
internal/exec/terraform_clean.go
[warning] 312-314: internal/exec/terraform_clean.go#L312-L314
Added lines #L312 - L314 were not covered by tests
[warning] 317-317: internal/exec/terraform_clean.go#L317
Added line #L317 was not covered by tests
[warning] 320-321: internal/exec/terraform_clean.go#L320-L321
Added lines #L320 - L321 were not covered by tests
[warning] 426-427: internal/exec/terraform_clean.go#L426-L427
Added lines #L426 - L427 were not covered by tests
[warning] 433-434: internal/exec/terraform_clean.go#L433-L434
Added lines #L433 - L434 were not covered by tests
[warning] 515-515: internal/exec/terraform_clean.go#L515
Added line #L515 was not covered by tests
[warning] 525-526: internal/exec/terraform_clean.go#L525-L526
Added lines #L525 - L526 were not covered by tests
[warning] 530-531: internal/exec/terraform_clean.go#L530-L531
Added lines #L530 - L531 were not covered by tests
[warning] 535-536: internal/exec/terraform_clean.go#L535-L536
Added lines #L535 - L536 were not covered by tests
[warning] 542-543: internal/exec/terraform_clean.go#L542-L543
Added lines #L542 - L543 were not covered by tests
[warning] 547-548: internal/exec/terraform_clean.go#L547-L548
Added lines #L547 - L548 were not covered by tests
[warning] 561-563: internal/exec/terraform_clean.go#L561-L563
Added lines #L561 - L563 were not covered by tests
[warning] 571-572: internal/exec/terraform_clean.go#L571-L572
Added lines #L571 - L572 were not covered by tests
[warning] 576-577: internal/exec/terraform_clean.go#L576-L577
Added lines #L576 - L577 were not covered by tests
[warning] 580-581: internal/exec/terraform_clean.go#L580-L581
Added lines #L580 - L581 were not covered by tests
[warning] 592-593: internal/exec/terraform_clean.go#L592-L593
Added lines #L592 - L593 were not covered by tests
[warning] 595-596: internal/exec/terraform_clean.go#L595-L596
Added lines #L595 - L596 were not covered by tests
[warning] 603-604: internal/exec/terraform_clean.go#L603-L604
Added lines #L603 - L604 were not covered by tests
[warning] 618-619: internal/exec/terraform_clean.go#L618-L619
Added lines #L618 - L619 were not covered by tests
[warning] 624-625: internal/exec/terraform_clean.go#L624-L625
Added lines #L624 - L625 were not covered by tests
[warning] 637-638: internal/exec/terraform_clean.go#L637-L638
Added lines #L637 - L638 were not covered by tests
[warning] 642-643: internal/exec/terraform_clean.go#L642-L643
Added lines #L642 - L643 were not covered by tests
[warning] 645-646: internal/exec/terraform_clean.go#L645-L646
Added lines #L645 - L646 were not covered by tests
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Acceptance Tests (windows-latest, windows)
- GitHub Check: Summary
🔇 Additional comments (4)
tests/fixtures/scenarios/terraform-sub-components/atmos.yaml (1)
1-16: LGTM!No issues found with the new configuration. The structure and keys look consistent with the user’s objectives.
internal/exec/terraform_clean.go (3)
19-23: No issues with new error variables.These error constants enhance the clarity and maintainability of error handling.
310-314: Fallback logic might hide unexpected errors.If
getRelativePathfails, the fallback joins the oldrelativePathwithfile.Name. Ensure this behavior is desirable, or log it at a higher severity to highlight potential misconfiguration.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 312-314: internal/exec/terraform_clean.go#L312-L314
Added lines #L312 - L314 were not covered by tests
510-520: Skipping invalid components may mask underlying issues.
getAllStacksComponentsPathsignores errors and continues. This could cause partial cleaning of intended paths. Verify this behavior is correct or handle/log the error more explicitly.🧰 Tools
🪛 golangci-lint (1.64.8)
[error] 520-520: File is not properly formatted
(gofumpt)
🪛 GitHub Check: codecov/patch
[warning] 515-515: internal/exec/terraform_clean.go#L515
Added line #L515 was not covered by tests
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
internal/exec/terraform_clean.go (2)
662-662: File exceeds length limit.The file is 562 lines, exceeding the 500-line limit. Consider splitting it into multiple files.
You could move the new helper functions (lines 517-662) to a separate file like
terraform_clean_helpers.go.🧰 Tools
🪛 GitHub Check: golangci-lint
[failure] 662-662:
file-length-limit: file length is 562 lines, which exceeds the limit of 500
611-612:⚠️ Potential issueReplace dynamic errors with wrapped static errors.
Currently using dynamic error creation which the linter flags as an issue. Use wrapped static errors instead.
Apply this pattern to fix the issue:
-return nil, fmt.Errorf("error determining relative path for folder %s: %v", folderPath, err) +return nil, errors.Wrapf(ErrMatchPattern, "determining relative path for folder %s", folderPath)Also applies to: 626-627, 645-646, 653-654
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 611-612: internal/exec/terraform_clean.go#L611-L612
Added lines #L611 - L612 were not covered by tests🪛 GitHub Check: golangci-lint
[failure] 611-611:
do not define dynamic errors, use wrapped static errors instead: "fmt.Errorf("error determining relative path for folder %s: %v", folderPath, err)"
🧹 Nitpick comments (1)
internal/exec/terraform_clean_test.go (1)
79-88: Unused parameter in verifyFileDeleted function.The
t *testing.Tparameter is only used for logging but not for actual test assertions.Consider one of these options:
-func verifyFileDeleted(t *testing.T, files []string) (bool, string) { +func verifyFileDeleted(files []string) (bool, string) { for _, file := range files { fileAbs, err := os.Stat(file) if err == nil { - t.Logf("File %s exists", fileAbs.Name()) + // File exists return false, fileAbs.Name() } } return true, "" }Or fix the formatting and actually use the parameter more effectively:
func verifyFileDeleted(t *testing.T, files []string) (bool, string) { for _, file := range files { fileAbs, err := os.Stat(file) if err == nil { t.Logf("File %s exists when it should have been deleted", fileAbs.Name()) return false, fileAbs.Name() } } t.Log("All files successfully deleted") return true, "" }🧰 Tools
🪛 golangci-lint (1.64.8)
[error] 79-79: File is not properly formatted
(gofumpt)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
internal/exec/terraform_clean.go(11 hunks)internal/exec/terraform_clean_test.go(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
internal/exec/terraform_clean_test.go (1)
Learnt from: haitham911
PR: cloudposse/atmos#727
File: internal/exec/terraform_clean.go:215-223
Timestamp: 2025-04-05T19:57:40.635Z
Learning: In `internal/exec/terraform_clean.go`, the function `determineCleanPath` is necessary and should not be removed.
internal/exec/terraform_clean.go (1)
Learnt from: haitham911
PR: cloudposse/atmos#727
File: internal/exec/terraform_clean.go:407-416
Timestamp: 2025-04-05T19:57:35.387Z
Learning: In `internal/exec/terraform_clean.go`, when `getStackTerraformStateFolder` returns an error in the `handleCleanSubCommand` function, the error is logged, and the process continues without returning the error.
🪛 golangci-lint (1.64.8)
internal/exec/terraform_clean_test.go
[error] 79-79: File is not properly formatted
(gofumpt)
internal/exec/terraform_clean.go
[error] 520-520: File is not properly formatted
(gofumpt)
[error] 552-552: File is not properly formatted
(gofumpt)
🪛 GitHub Check: codecov/patch
internal/exec/terraform_clean.go
[warning] 58-58: internal/exec/terraform_clean.go#L58
Added line #L58 was not covered by tests
[warning] 92-92: internal/exec/terraform_clean.go#L92
Added line #L92 was not covered by tests
[warning] 106-106: internal/exec/terraform_clean.go#L106
Added line #L106 was not covered by tests
[warning] 137-137: internal/exec/terraform_clean.go#L137
Added line #L137 was not covered by tests
[warning] 205-205: internal/exec/terraform_clean.go#L205
Added line #L205 was not covered by tests
[warning] 216-216: internal/exec/terraform_clean.go#L216
Added line #L216 was not covered by tests
[warning] 320-322: internal/exec/terraform_clean.go#L320-L322
Added lines #L320 - L322 were not covered by tests
[warning] 325-325: internal/exec/terraform_clean.go#L325
Added line #L325 was not covered by tests
[warning] 328-329: internal/exec/terraform_clean.go#L328-L329
Added lines #L328 - L329 were not covered by tests
[warning] 434-435: internal/exec/terraform_clean.go#L434-L435
Added lines #L434 - L435 were not covered by tests
[warning] 441-442: internal/exec/terraform_clean.go#L441-L442
Added lines #L441 - L442 were not covered by tests
[warning] 446-446: internal/exec/terraform_clean.go#L446
Added line #L446 was not covered by tests
[warning] 523-523: internal/exec/terraform_clean.go#L523
Added line #L523 was not covered by tests
[warning] 533-534: internal/exec/terraform_clean.go#L533-L534
Added lines #L533 - L534 were not covered by tests
[warning] 538-539: internal/exec/terraform_clean.go#L538-L539
Added lines #L538 - L539 were not covered by tests
[warning] 543-544: int
6D40
ernal/exec/terraform_clean.go#L543-L544
Added lines #L543 - L544 were not covered by tests
[warning] 550-551: internal/exec/terraform_clean.go#L550-L551
Added lines #L550 - L551 were not covered by tests
[warning] 555-556: internal/exec/terraform_clean.go#L555-L556
Added lines #L555 - L556 were not covered by tests
[warning] 569-571: internal/exec/terraform_clean.go#L569-L571
Added lines #L569 - L571 were not covered by tests
[warning] 579-580: internal/exec/terraform_clean.go#L579-L580
Added lines #L579 - L580 were not covered by tests
[warning] 584-585: internal/exec/terraform_clean.go#L584-L585
Added lines #L584 - L585 were not covered by tests
[warning] 588-589: internal/exec/terraform_clean.go#L588-L589
Added lines #L588 - L589 were not covered by tests
[warning] 600-601: internal/exec/terraform_clean.go#L600-L601
Added lines #L600 - L601 were not covered by tests
[warning] 603-604: internal/exec/terraform_clean.go#L603-L604
Added lines #L603 - L604 were not covered by tests
[warning] 611-612: internal/exec/terraform_clean.go#L611-L612
Added lines #L611 - L612 were not covered by tests
[warning] 626-627: internal/exec/terraform_clean.go#L626-L627
Added lines #L626 - L627 were not covered by tests
[warning] 632-633: internal/exec/terraform_clean.go#L632-L633
Added lines #L632 - L633 were not covered by tests
[warning] 645-646: internal/exec/terraform_clean.go#L645-L646
Added lines #L645 - L646 were not covered by tests
[warning] 650-651: internal/exec/terraform_clean.go#L650-L651
Added lines #L650 - L651 were not covered by tests
[warning] 653-654: internal/exec/terraform_clean.go#L653-L654
Added lines #L653 - L654 were not covered by tests
🪛 GitHub Check: golangci-lint
internal/exec/terraform_clean.go
[failure] 611-611:
do not define dynamic errors, use wrapped static errors instead: "fmt.Errorf("error determining relative path for folder %s: %v", folderPath, err)"
[failure] 626-626:
do not define dynamic errors, use wrapped static errors instead: "fmt.Errorf("error matching pattern %s in folder %s: %v", pat, folderPath, err)"
[failure] 645-645:
do not define dynamic errors, use wrapped static errors instead: "fmt.Errorf("error determining relative path for %s: %v", filePath, err)"
[failure] 653-653:
do not define dynamic errors, use wrapped static errors instead: "fmt.Errorf("error stating file %s: %v", filePath, err)"
[failure] 662-662:
file-length-limit: file length is 562 lines, which exceeds the limit of 500
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Acceptance Tests (windows-latest, windows)
- GitHub Check: Summary
🔇 Additional comments (7)
internal/exec/terraform_clean.go (5)
19-32: Good addition of standardized error variables.The introduction of these error variables improves error handling consistency throughout the codebase. This supports better error categorization and follows Go best practices.
318-322: Good fallback mechanism for path resolution.The code now tries to get a more accurate relative path and falls back to the original method if that fails. This enhances reliability.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 320-322: internal/exec/terraform_clean.go#L320-L322
Added lines #L320 - L322 were not covered by tests
432-447: Core functionality change: Improved component path detection.This is the key change that addresses the PR objective. By using stack descriptions to retrieve component paths, the code can now properly handle components in subfolders.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 434-435: internal/exec/terraform_clean.go#L434-L435
Added lines #L434 - L435 were not covered by tests
[warning] 441-442: internal/exec/terraform_clean.go#L441-L442
Added lines #L441 - L442 were not covered by tests
[warning] 446-446: internal/exec/terraform_clean.go#L446
Added line #L446 was not covered by tests
517-561: Good extraction of component paths from stacks map.These helper functions provide clean separation of concerns and make the code more maintainable. The recursive traversal of the stack structure is well-implemented.
🧰 Tools
🪛 golangci-lint (1.64.8)
[error] 520-520: File is not properly formatted
(gofumpt)
[error] 552-552: File is not properly formatted
(gofumpt)
🪛 GitHub Check: codecov/patch
[warning] 523-523: internal/exec/terraform_clean.go#L523
Added line #L523 was not covered by tests
[warning] 533-534: internal/exec/terraform_clean.go#L533-L534
Added lines #L533 - L534 were not covered by tests
[warning] 538-539: internal/exec/terraform_clean.go#L538-L539
Added lines #L538 - L539 were not covered by tests
[warning] 543-544: internal/exec/terraform_clean.go#L543-L544
Added lines #L543 - L544 were not covered by tests
[warning] 550-551: internal/exec/terraform_clean.go#L550-L551
Added lines #L550 - L551 were not covered by tests
[warning] 555-556: internal/exec/terraform_clean.go#L555-L556
Added lines #L555 - L556 were not covered by tests
563-575: Well-structured collection of directory objects for components.This function effectively collects objects for multiple components, aggregating the results.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 569-571: internal/exec/terraform_clean.go#L569-L571
Added lines #L569 - L571 were not covered by testsinternal/exec/terraform_clean_test.go (2)
11-77: Thorough test coverage for the clean command.This test effectively validates the core functionality by:
- Setting up the environment
- Creating terraform components
- Verifying files exist
- Running the clean command
- Verifying files are deleted
Good work setting up and tearing down the test environment properly.
60-63:✅ Verification successful
Missing helper function declaration in test file.
The test calls
verifyFileExists()but this function isn't shown in the provided code.Make sure this helper function is implemented somewhere or add it to the test file.
🏁 Script executed:
#!/bin/bash # Check if verifyFileExists is defined in any file grep -r "func verifyFileExists" internal/exec/ || echo "verifyFileExists function not found"Length of output: 152
Helper Function Confirmed – No Action Required
I verified that the helper function
verifyFileExistsis implemented ininternal/exec/vendor_utils_test.goand is available for the test interraform_clean_test.go. If this shared implementation is intentional, no further modifications are needed. However, consider adding a comment interraform_clean_test.goto explicitly reference its origin for clarity.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
internal/exec/terraform_clean_util.go (3)
41-53: Consider ensuring uniqueness in component paths.Appending paths directly (lines 52–53) may add duplicates if multiple stacks contain the same
componentattribute. If that’s unintended, you might want to use a set or filter for unique paths before returning them.
77-84: Inform callers of partial collects.
CollectComponentObjectsstops and returns upon the first error (lines 78–84). If partial data is acceptable, consider logging the error instead of returning immediately. Alternatively, gather what you can and return aggregated errors for more robust handling.
117-135: Handle large patterns or nested matches.
collectFilesInFolder(lines 117–135) directly matches only at the top level of each folder. If patterns or subfolders get large or deeply nested, consider a more robust approach (e.g., filepath.Walk) for better coverage and performance.internal/exec/terraform_clean.go (3)
106-106: Clarify formatting in wrapped error.Line 106 has:
fmt.Errorf("%w,path %s error %v", ErrFileStat, filePath, err)There's a missing space after the comma, which may lead to confusing error messages. Consider revising as:
- return nil, fmt.Errorf("%w,path %s error %v", ErrFileStat, filePath, err) + return nil, fmt.Errorf("%w, path %s: %v", ErrFileStat, filePath, err)
205-205: Surface errors for debugging clarity.When failing to find stack folders, the returned error wraps
ErrFailedFoundStack. Consider logging a more descriptive message or providing call-site context to help diagnose which stack or path triggered this condition.
441-442: Add context to error stack logs.Here:
fmt.Errorf("%w: %s", ErrDescribeStack, err)This is good, but consider clarifying which stack or component triggered the error for improved debugging in multi-stack runs.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
internal/exec/terraform_clean.go(11 hunks)internal/exec/terraform_clean_util.go(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
internal/exec/terraform_clean_util.go (2)
Learnt from: haitham911
PR: cloudposse/atmos#727
File: internal/exec/terraform_clean.go:215-223
Timestamp: 2025-04-05T19:57:40.635Z
Learning: In `internal/exec/terraform_clean.go`, the function `determineCleanPath` is necessary and should not be removed.
Learnt from: haitham911
PR: cloudposse/atmos#727
File: internal/exec/terraform_clean.go:407-416
Timestamp: 2025-04-05T19:57:35.387Z
Learning: In `internal/exec/terraform_clean.go`, when `getStackTerraformStateFolder` returns an error in the `handleCleanSubCommand` function, the error is logged, and the process continues without returning the error.
internal/exec/terraform_clean.go (1)
Learnt from: haitham911
PR: cloudposse/atmos#727
File: internal/exec/terraform_clean.go:407-416
Timestamp: 2025-04-05T19:57:35.387Z
Learning: In `internal/exec/terraform_clean.go`, when `getStackTerraformStateFolder` returns an error in the `handleCleanSubCommand` function, the error is logged, and the process continues without returning the error.
🪛 GitHub Check: golangci-lint
internal/exec/terraform_clean_util.go
[failure] 140-140:
do not define dynamic errors, use wrapped static errors instead: "fmt.Errorf("error determining relative path for %s: %v", filePath, err)"
⏰ Context from checks skipped due to timeout of 90000ms (20)
- GitHub Check: [mock-macos] tests/fixtures/scenarios/complete
- GitHub Check: [mock-macos] examples/demo-vendoring
- GitHub Check: [mock-macos] examples/demo-context
- GitHub Check: [mock-macos] examples/demo-component-versions
- GitHub Check: [mock-macos] examples/demo-atlantis
- GitHub Check: [mock-windows] examples/demo-context
- GitHub Check: [mock-windows] examples/demo-atlantis
- GitHub Check: [mock-linux] tests/fixtures/scenarios/complete
- GitHub Check: [mock-linux] examples/demo-vendoring
- GitHub Check: Acceptance Tests (macos-latest, macos)
- GitHub Check: [mock-linux] examples/demo-context
- GitHub Check: Acceptance Tests (windows-latest, windows)
- GitHub Check: [mock-linux] examples/demo-component-versions
- GitHub Check: Acceptance Tests (ubuntu-latest, linux)
- GitHub Check: [lint] quick-start-advanced
- GitHub Check: [k3s] demo-helmfile
- GitHub Check: [lint] demo-context
- GitHub Check: [lint] Dockerfile
- GitHub Check: [localstack] demo-localstack
- GitHub Check: Summary
🔇 Additional comments (8)
internal/exec/terraform_clean_util.go (2)
58-70: Validate subfolder collection strategy.
CollectComponentsDirectoryObjectscurrently collects files for each relative path (line 62) but doesn’t walk subdirectories beyondCollectComponentObjects. Confirm this meets your needs for multi-level Terraform components.
137-157: Check concurrency-safety if usage expands.
createFileInfousesos.Statand returns file metadata; currently it's safe in single-thread usage. If you later parallelize collection, ensure no concurrency hazards (e.g., repeated writes to shared structures).🧰 Tools
🪛 GitHub Check: golangci-lint
[failure] 140-140:
do not define dynamic errors, use wrapped static errors instead: "fmt.Errorf("error determining relative path for %s: %v", filePath, err)"internal/exec/terraform_clean.go (6)
58-59: Use wrapped error variable for uniformity.Here, you wrap an error with:
fmt.Errorf("%w path %s: error %v", ErrReadDir, root, err)This is good, but ensure all references to directory read errors consistently use
ErrReadDirthroughout the file. This consistency improves grep-based troubleshooting.
92-92: Prevent dynamic error definitions.Line 92 uses:
fmt.Errorf("%w %s", ErrPathNotExist, basePath)This looks correct. Just confirm other code segments also rely on the
ErrPathNotExistvariable instead of new dynamic errors, to keep your error handling coherent.
137-138: Prefer wrapped static error for match pattern failures.On line 137, you already wrap with
ErrMatchPattern. This is great—make sure all pattern failures use this variable consistently across the code. This consistency helps track pattern-matching errors efficiently.
318-321: Debug logging for fallback path usage.This block logs an error then falls back to a default path. This approach is valid, but ensure logs have enough detail (like
file.FullPathand the exact cause) for debugging the fallback scenario.
432-435: Filter components usage check.Lines 432–435 define
FilterComponents. Validate downstream logic to confirm these filtered components are effectively applied when describing stacks. Otherwise, irrelevant components might appear in your final list of folders.
449-458: Partial error handling on state folder retrieval.You catch errors from
getStackTerraformStateFolder(line 451) but continue without failing. This might hide crucial state if the folder can’t be retrieved. Verify if partial success is expected, or handle the error more strictly if needed.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
internal/exec/terraform_clean.go (1)
446-447: Debug logging could be improved.While you're correctly logging the error at debug level, consider enhancing the error returned to the caller with additional context or using one of your new standard error variables.
if err != nil { log.Debug("error collecting folders and files", "error", err) - return err + return errors.Wrap(ErrCollectFiles, err.Error()) }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/exec/terraform_clean.go(10 hunks)
🧰 Additional context used
🧠 Learnings (1)
internal/exec/terraform_clean.go (1)
Learnt from: haitham911
PR: cloudposse/atmos#727
File: internal/exec/terraform_clean.go:407-416
Timestamp: 2025-04-05T19:57:35.387Z
Learning: In `internal/exec/terraform_clean.go`, when `getStackTerraformStateFolder` returns an error in the `handleCleanSubCommand` function, the error is logged, and the process continues without returning the error.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Summary
🔇 Additional comments (5)
internal/exec/terraform_clean.go (5)
18-31: Great standardization of error variables.Adding these static error variables improves error handling consistency across the codebase and addresses the static analysis issues flagged in previous reviews.
58-58: Error handling improvements align with best practices.You've consistently updated error handling to use the new static error variables throughout the code. This is exactly what was needed according to previous review comments.
Also applies to: 92-92, 106-106, 137-137, 205-205, 216-216
318-322: Nice resilience improvement in path handling.Adding fallback logic when relative path retrieval fails improves code robustness. The debug log provides visibility without disrupting execution.
441-441: Good use of static error wrapping.Using the new
ErrDescribeStackerror variable improves error clarity while maintaining stack trace information.
432-445:✅ Verification successful
Core fix implementation for sub-component handling looks solid.
The changes properly implement support for components in subfolders by:
- Adding component filtering
- Using
ExecuteDescribeStacksto collect component information- Implementing a more robust path collection approach
This addresses the main issue described in the PR objective.
🏁 Script executed:
#!/bin/bash # Verify the implementation of the referenced functions echo "Checking implementation of getAllStacksComponentsPaths and CollectComponentsDirectoryObjects" rg -A 10 "func getAllStacksComponentsPaths" internal/exec/ rg -A 10 "func CollectComponentsDirectoryObjects" internal/exec/Length of output: 1980
Sub-Component Handling Fix Verified and Approved
The verification confirms that the functions
getAllStacksComponentsPathsandCollectComponentsDirectoryObjectsare correctly implemented. The code properly handles:
- Component filtering using
info.ComponentFromArg.- Collection of stack component paths from the provided
stacksMap.- Robust path collection through directory object gathering.
Everything checks out and aligns with the intended fix for supporting components in subfolders.
|
@haitham911 please review/address the |
atmos terraform clean command for Terraform components in sub-directories
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
internal/exec/terraform_clean.go (2)
466-468: Inconsistent error handling style.Similar to the previous comment, error handling here could be updated to use structured logging with
log.Debug.- u.LogTrace(fmt.Errorf("error collecting folder of ENV TF_DATA_DIR: %v", err).Error()) + log.Debug("error collecting folder of ENV TF_DATA_DIR", "error", err)
120-120:⚠️ Potential issueFix undefined error variable
ErrRelPath.The same undefined
ErrRelPatherror variable is used here.-return nil, fmt.Errorf("%w %s: %v", ErrRelPath, folderPath, err) +return nil, fmt.Errorf("error determining relative path for folder %s: %v", folderPath, err)
🧹 Nitpick comments (1)
internal/exec/terraform_clean.go (1)
451-456: Inconsistent error handling style.Unlike other error handling in this file, errors here aren't wrapped with standard error variables. Also, consider using structured logging with
log.Debuginstead ofu.LogTrace.- errMsg := fmt.Errorf("error getting stack terraform state folders: %v", err) - u.LogTrace(errMsg.Error()) + log.Debug("error getting stack terraform state folders", "error", err)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
internal/exec/terraform_clean.go(10 hunks)tests/fixtures/scenarios/terraform-sub-components/atmos.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/fixtures/scenarios/terraform-sub-components/atmos.yaml
🧰 Additional context used
🧠 Learnings (1)
internal/exec/terraform_clean.go (2)
Learnt from: haitham911
PR: cloudposse/atmos#727
File: internal/exec/terraform_clean.go:407-416
Timestamp: 2024-11-24T19:13:10.287Z
Learning: In `internal/exec/terraform_clean.go`, when `getStackTerraformStateFolder` returns an error in the `handleCleanSubCommand` function, the error is logged, and the process continues without returning the error.
Learnt from: Cerebrovinny
PR: cloudposse/atmos#764
File: internal/exec/describe_stacks.go:289-295
Timestamp: 2024-11-13T21:37:07.852Z
Learning: In the `internal/exec/describe_stacks.go` file of the `atmos` project written in Go, avoid extracting the stack name handling logic into a helper function within the `ExecuteDescribeStacks` method, even if the logic appears duplicated.
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Summary
🔇 Additional comments (3)
internal/exec/terraform_clean.go (3)
317-321: Good enhancement to path handling.Using
getRelativePathadds robustness to the path handling logic with an appropriate fallback mechanism when the relative path calculation fails.
440-440: Use consistent error wrapping.Error wrapping looks good here with
ErrDescribeStack, which helps with standardized error handling.
445-445: Use debug level for non-critical errors.Good change - using debug level for collection errors allows operation to continue without alarming users unnecessarily.
…sts for folder operations
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (2)
internal/exec/terraform_clean.go (2)
55-57: 🛠️ Refactor suggestionReuse the standard
ErrEmptyPathvariableYou already declared
ErrEmptyPath; wrap it here for consistency and to satisfy lint rules.- if root == "" { - return nil, fmt.Errorf("root path cannot be empty") + if root == "" { + return nil, ErrEmptyPath }
91-93: 🛠️ Refactor suggestionConsistent error handling for empty paths
Same issue as above—prefer the predefined
ErrEmptyPath.- if basePath == "" { - return nil, fmt.Errorf("path cannot be empty") + if basePath == "" { + return nil, ErrEmptyPath }
🧹 Nitpick comments (3)
internal/exec/terraform_clean_test.go (1)
77-86: Mark helper and return full path for clearer failures
verifyFileDeletedcan be a test helper and should return the full path to ease debugging.-func verifyFileDeleted(t *testing.T, files []string) (bool, string) { +func verifyFileDeleted(t *testing.T, files []string) (bool, string) { + t.Helper() for _, file := range files { fileAbs, err := os.Stat(file) if err == nil { - t.Errorf("Reason: File still exists: %q", file) - return false, fileAbs.Name() + t.Errorf("Reason: File still exists: %q", file) + return false, file } } return true, "" }internal/exec/terraform_clean.go (2)
435-438: Follow Go naming conventionsLocal slice should be
filterComponents(camel-case, lower first letter).-var FilterComponents []string -if info.ComponentFromArg != "" { - FilterComponents = append(FilterComponents, info.ComponentFromArg) -} +var filterComponents []string +if info.ComponentFromArg != "" { + filterComponents = append(filterComponents, info.ComponentFromArg) +}
339-341: Provide structured context in debug logsPassing just an
errordrops useful context. Supply a short message.- for _, err := range errors { - log.Debug(err) + for _, err := range errors { + log.Debug("delete error", "error", err) }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
internal/exec/terraform_clean.go(15 hunks)internal/exec/terraform_clean_test.go(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
internal/exec/terraform_clean_test.go (3)
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.
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.
Learnt from: Cerebrovinny
PR: cloudposse/atmos#737
File: internal/exec/vendor_utils.go:131-141
Timestamp: 2024-10-22T23:00:20.627Z
Learning: In the `ReadAndProcessVendorConfigFile` function in `internal/exec/vendor_utils.go`, the existence of the vendor config file is already checked, so additional file existence checks may be unnecessary.
internal/exec/terraform_clean.go (4)
Learnt from: haitham911
PR: cloudposse/atmos#1195
File: internal/exec/terraform_clean.go:99-99
Timestamp: 2025-04-26T15:54:10.485Z
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.
Learnt from: haitham911
PR: cloudposse/atmos#1195
File: internal/exec/terraform_clean.go:99-99
Timestamp: 2025-04-26T15:54:10.486Z
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.
Learnt from: haitham911
PR: cloudposse/atmos#727
File: internal/exec/terraform_clean.go:407-416
Timestamp: 2024-11-24T19:13:10.287Z
Learning: In `internal/exec/terraform_clean.go`, when `getStackTerraformStateFolder` returns an error in the `handleCleanSubCommand` function, the error is logged, and the process continues without returning the error.
Learnt from: Cerebrovinny
PR: cloudposse/atmos#764
File: internal/exec/describe_stacks.go:289-295
Timestamp: 2024-11-13T21:37:07.852Z
Learning: In the `internal/exec/describe_stacks.go` file of the `atmos` project written in Go, avoid extracting the stack name handling logic into a helper function within the `ExecuteDescribeStacks` method, even if the logic appears duplicated.
🪛 GitHub Check: golangci-lint
internal/exec/terraform_clean.go
[failure] 444-444:
add-constant: string literal "%w: %s" appears, at least, 4 times, create a named constant for it
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Summary
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (2)
internal/exec/terraform_clean.go (2)
175-175: 🛠️ Refactor suggestionStandard error variable should be used here.
This error should use
ErrReadDirfor consistency.-return nil, fmt.Errorf("error reading the base path %s: %v", basePath, err) +return nil, fmt.Errorf("%w path %s: %v", ErrReadDir, basePath, err)
427-428: 🛠️ Refactor suggestionUse standard error variable.
You should use one of your standard error variables here.
-return fmt.Errorf("could not find the component '%s'", info.ComponentFromArg) +return fmt.Errorf("%w: '%s'", ErrParseComponents, info.ComponentFromArg)
♻️ Duplicate comments (3)
internal/exec/terraform_clean.go (3)
399-399: 🛠️ Refactor suggestionFix error wrapping to follow linting rules.
The linter flagged this dynamic error.
-return fmt.Errorf(ErroFormat, ErrResolveEnvDir, err) +return errors.Wrap(ErrResolveEnvDir, err.Error())🧰 Tools
🪛 GitHub Check: golangci-lint
[failure] 399-399:
do not define dynamic errors, use wrapped static errors instead: "fmt.Errorf(ErroFormat, ErrResolveEnvDir, err)"
454-454: 🛠️ Refactor suggestionFix error wrapping to follow linting rules.
The linter flagged this dynamic error.
-return fmt.Errorf(ErroFormat, ErrDescribeStack, err) +return errors.Wrap(ErrDescribeStack, err.Error())🧰 Tools
🪛 GitHub Check: golangci-lint
[failure] 454-454:
do not define dynamic errors, use wrapped static errors instead: "fmt.Errorf(ErroFormat, ErrDescribeStack, err)"
413-413: 🛠️ Refactor suggestionFix error wrapping to follow linting rules.
The linter flagged this dynamic error.
-return fmt.Errorf(ErroFormat, ErrRefusingToDelete, "..") +return errors.Wrapf(ErrRefusingToDelete, "%s", "..")🧰 Tools
🪛 GitHub Check: golangci-lint
[failure] 413-413:
do not define dynamic errors, use wrapped static errors instead: "fmt.Errorf(ErroFormat, ErrRefusingToDelete, "..")"
🧹 Nitpick comments (1)
internal/exec/terraform_clean.go (1)
111-111: Minor formatting inconsistency in error wrapping.There's an inconsistency in the error format - a comma is used instead of a space after the wrapped error.
-return nil, fmt.Errorf("%w,path %s error %v", ErrFileStat, filePath, err) +return nil, fmt.Errorf("%w path %s error %v", ErrFileStat, filePath, err)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
internal/exec/terraform_clean.go(15 hunks)internal/exec/terraform_clean_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/exec/terraform_clean_test.go
🧰 Additional context used
🧠 Learnings (1)
internal/exec/terraform_clean.go (4)
Learnt from: haitham911
PR: cloudposse/atmos#1195
File: internal/exec/terraform_clean.go:99-99
Timestamp: 2025-04-26T15:54:10.486Z
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.
Learnt from: haitham911
PR: cloudposse/atmos#1195
File: internal/exec/terraform_clean.go:99-99
Timestamp: 2025-04-26T15:54:10.485Z
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.
Learnt from: haitham911
PR: cloudposse/atmos#727
File: internal/exec/terraform_clean.go:407-416
Timestamp: 2024-11-24T19:13:10.287Z
Learning: In `internal/exec/terraform_clean.go`, when `getStackTerraformStateFolder` returns an error in the `handleCleanSubCommand` function, the error is logged, and the process continues without returning the error.
Learnt from: Cerebrovinny
PR: cloudposse/atmos#764
File: internal/exec/describe_stacks.go:289-295
Timestamp: 2024-11-13T21:37:07.852Z
Learning: In the `internal/exec/describe_stacks.go` file of the `atmos` project written in Go, avoid extracting the stack name handling logic into a helper function within the `ExecuteDescribeStacks` method, even if the logic appears duplicated.
🧬 Code Graph Analysis (1)
internal/exec/terraform_clean.go (3)
internal/exec/terraform_clean_util.go (2)
ErrRelPath(13-13)CollectComponentsDirectoryObjects(62-74)cmd/cmd_utils.go (1)
Contains(780-787)internal/exec/describe_stacks.go (1)
ExecuteDescribeStacks(148-689)
🪛 GitHub Check: golangci-lint
internal/exec/terraform_clean.go
[failure] 399-399:
do not define dynamic errors, use wrapped static errors instead: "fmt.Errorf(ErroFormat, ErrResolveEnvDir, err)"
[failure] 404-404:
do not define dynamic errors, use wrapped static errors instead: "fmt.Errorf(ErroFormat, ErrRefusingToDeleteDir, absTFDataDir)"
[failure] 409-409:
do not define dynamic errors, use wrapped static errors instead: "fmt.Errorf(ErroFormat, ErrRefusingToDeleteDir, absTFDataDir)"
[failure] 413-413:
do not define dynamic errors, use wrapped static errors instead: "fmt.Errorf(ErroFormat, ErrRefusingToDelete, "..")"
[failure] 454-454:
do not define dynamic errors, use wrapped static errors instead: "fmt.Errorf(ErroFormat, ErrDescribeStack, err)"
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Summary
🔇 Additional comments (21)
internal/exec/terraform_clean.go (21)
11-16: Good improvement with structured logging.Switching to
github.com/charmbracelet/logfor structured logging is a solid improvement that aligns with modern Go practices and will make debugging easier.
18-37: Strong error standardization approach.Defining these standardized error variables is excellent. It improves error handling consistency, makes errors more identifiable, and follows Go best practices.
However, there's a typo in
ErroFormat(should beErrorFormat), but since it's internal, it's not critical to change.
58-58: Good usage of standardized error.Using
ErrRootPathinstead of a dynamic error string follows best practices.
63-63: Improved error wrapping.Properly using the error wrapping pattern with the standardized error variable.
77-77: Good structured logging.Using structured logging with fields for directory and error provides better context for debugging.
97-97: Good error wrapping pattern.Properly wrapping the standard error with additional context.
105-105: Correct use of error variable from utility file.Using
ErrRelPathdefined interraform_clean_util.gois the right approach here.
142-142: Good error detail for pattern matching.The error provides clear context about which pattern failed in which folder.
210-210: Good error wrapping pattern.Using the standard error format with
ErrFailedFoundStack.
221-221: Improved error specificity.The error clearly indicates what failed and where.
236-253: Good utility function addition.Adding
getRelativePathcentralizes path handling logic and improves code maintainability.
323-327: Robust error handling with fallback.Good approach to handling path resolution failures by providing a fallback and logging the error.
329-330: Improved error propagation in file deletion.Errors during deletion are now captured and include the relative path for better context.
341-341: Changed logging level.Changed from error logging to debug logging. Make sure this matches your logging strategy.
Is debug the appropriate level for deletion errors, or should they be warnings or errors since they represent operation failures?
349-349: Consistent structured logging.Good use of structured logging with relevant fields.
395-395: Standard error usage.Using the standard error variable directly is cleaner than wrapping it.
402-410: Improved Windows path validation.Good addition of Windows-specific root path detection to prevent accidental deletion of root directories.
🧰 Tools
🪛 GitHub Check: golangci-lint
[failure] 404-404:
do not define dynamic errors, use wrapped static errors instead: "fmt.Errorf(ErroFormat, ErrRefusingToDeleteDir, absTFDataDir)"
[failure] 409-409:
do not define dynamic errors, use wrapped static errors instead: "fmt.Errorf(ErroFormat, ErrRefusingToDeleteDir, absTFDataDir)"
446-457: Core improvement for handling subfolders.This is the key change that fixes the issue with components in subdirectories. Using
ExecuteDescribeStacksandgetAllStacksComponentsPathsto discover all component paths is a solid approach.🧰 Tools
🪛 GitHub Check: golangci-lint
[failure] 454-454:
do not define dynamic errors, use wrapped static errors instead: "fmt.Errorf(ErroFormat, ErrDescribeStack, err)"
457-460: Good integration with utilities.Using
CollectComponentsDirectoryObjectsto collect all component directories is a clean approach that supports the subdirectory handling.
475-481: Consistent error handling.The error handling for TF_DATA_DIR validation and collection follows the same pattern used elsewhere in the file.
465-465:✅ Verification successful
Appropriate error handling.
Good use of debug logging instead of returning the error, which allows the process to continue even if one component has an issue.
🏁 Script executed:
#!/bin/bash # Check if the structured logging approach is consistent with other error handling in the codebase grep -r "log.Debug(\"error" --include="*.go" internal/exec/ | wc -lLength of output: 70
Consistent debug‐level error handling
Verified that debug‐level error logs (e.g., internal/exec/terraform_clean.go:465) appear 6 times in internal/exec/, matching the existing pattern for nonblocking errors. No changes needed.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/exec/terraform_clean.go (1)
36-36: Consider using a constant for the error format string.The
ErroFormatvariable is used in multiple places, which is good. However, it would be better to follow Go naming conventions and name it asErrFormat(without the 'o') for consistency with the other error variable names.-var ErroFormat = "%w: %s" +var ErrFormat = "%w: %s"
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
internal/exec/terraform_clean.go(15 hunks)internal/exec/terraform_clean_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/exec/terraform_clean_test.go
🧰 Additional context used
🧠 Learnings (1)
internal/exec/terraform_clean.go (4)
Learnt from: haitham911
PR: cloudposse/atmos#1195
File: internal/exec/terraform_clean.go:99-99
Timestamp: 2025-04-26T15:54:10.486Z
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.
Learnt from: haitham911
PR: cloudposse/atmos#1195
File: internal/exec/terraform_clean.go:99-99
Timestamp: 2025-04-26T15:54:10.485Z
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.
Learnt from: haitham911
PR: cloudposse/atmos#727
File: internal/exec/terraform_clean.go:407-416
Timestamp: 2024-11-24T19:13:10.287Z
Learning: In `internal/exec/terraform_clean.go`, when `getStackTerraformStateFolder` returns an error in the `handleCleanSubCommand` function, the error is logged, and the process continues without returning the error.
Learnt from: Cerebrovinny
PR: cloudposse/atmos#764
File: internal/exec/describe_stacks.go:289-295
Timestamp: 2024-11-13T21:37:07.852Z
Learning: In the `internal/exec/describe_stacks.go` file of the `atmos` project written in Go, avoid extracting the stack name handling logic into a helper function within the `ExecuteDescribeStacks` method, even if the logic appears duplicated.
🪛 GitHub Check: golangci-lint
internal/exec/terraform_clean.go
[failure] 399-399:
do not define dynamic errors, use wrapped static errors instead: "fmt.Errorf(ErroFormat, ErrResolveEnvDir, err)"
[failure] 404-404:
do not define dynamic errors, use wrapped static errors instead: "fmt.Errorf(ErroFormat, ErrRefusingToDeleteDir, absTFDataDir)"
[failure] 409-409:
do not define dynamic errors, use wrapped static errors instead: "fmt.Errorf(ErroFormat, ErrRefusingToDeleteDir, absTFDataDir)"
[failure] 413-413:
do not define dynamic errors, use wrapped static errors instead: "fmt.Errorf(ErroFormat, ErrRefusingToDelete, "..")"
[failure] 454-454:
do not define dynamic errors, use wrapped static errors instead: "fmt.Errorf(ErroFormat, ErrDescribeStack, err)"
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Summary
🔇 Additional comments (8)
internal/exec/terraform_clean.go (8)
18-37: Good addition of standardized error variables.The addition of standardized error variables improves code consistency and makes error handling more robust. These variables will make the codebase more maintainable by centralizing error definitions and standardizing error messages.
11-11: Good switch to structured logging.Using the Charmbracelet log package for structured logging aligns with modern logging practices. This change improves the diagnostic capabilities of the code.
236-253: Well-implemented relative path utility function.The new
getRelativePathfunction is a good addition that standardizes path handling logic. This will be particularly useful for handling components in subdirectories, addressing the core issue this PR aims to fix.
323-327: Good fallback mechanism for relative path resolution.The code now attempts to use the new
getRelativePathfunction, but falls back gracefully to the previous method if it fails. This ensures backward compatibility while adding the new functionality.
402-410: Good addition of Windows-specific path checks.The addition of Windows-specific root path checks improves cross-platform compatibility. This prevents accidental deletion of root directories on Windows systems.
🧰 Tools
🪛 GitHub Check: golangci-lint
[failure] 404-404:
do not define dynamic errors, use wrapped static errors instead: "fmt.Errorf(ErroFormat, ErrRefusingToDeleteDir, absTFDataDir)"
[failure] 409-409:
do not define dynamic errors, use wrapped static errors instead: "fmt.Errorf(ErroFormat, ErrRefusingToDeleteDir, absTFDataDir)"
445-457: Key implementation for fixing components in subdirectories.This section implements the core functionality needed to fix the issue with Terraform components in sub-directories. By using
ExecuteDescribeStacksand extracting component paths from the stack description, the code can now locate all component paths including those in subdirectories.🧰 Tools
🪛 GitHub Check: golangci-lint
[failure] 454-454:
do not define dynamic errors, use wrapped static errors instead: "fmt.Errorf(ErroFormat, ErrDescribeStack, err)"
458-460: Good structured error logging.Using structured logging with debug level is appropriate here. It provides useful diagnostic information without cluttering normal output.
105-105: Correct usage of ErrRelPath error variable.The error variable
ErrRelPathis defined ininternal/exec/terraform_clean_util.go. Since both files are in the same package, this variable is properly accessible here.Also applies to: 126-126
…ectoryObjects function
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
internal/exec/terraform_clean.go (2)
62-63: Consider using errors.Wrapf instead of fmt.Errorf for wrapping errors.While using fmt.Errorf with %w correctly wraps the static error, the linter may still flag it. Using errors.Wrapf is the preferred approach in this codebase.
-return nil, fmt.Errorf("%w path %s: error %v", ErrReadDir, root, err) +return nil, errors.Wrapf(ErrReadDir, "path %s: error %v", root, err)
104-105: ErrRelPath is used but defined in terraform_clean_util.go.The error variable ErrRelPath is defined in terraform_clean_util.go and used here. This is valid since both files are in the same package, but a comment explaining this would improve code clarity.
relativePath, err := filepath.Rel(basePath, filePath) if err != nil { + // ErrRelPath is defined in terraform_clean_util.go return nil, fmt.Errorf("%w %s: %v", ErrRelPath, filePath, err) }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/exec/terraform_clean.go(15 hunks)
🧰 Additional context used
🧠 Learnings (1)
internal/exec/terraform_clean.go (4)
Learnt from: haitham911
PR: cloudposse/atmos#1195
File: internal/exec/terraform_clean.go:99-99
Timestamp: 2025-04-26T15:54:10.486Z
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.
Learnt from: haitham911
PR: cloudposse/atmos#1195
File: internal/exec/terraform_clean.go:99-99
Timestamp: 2025-04-26T15:54:10.485Z
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.
Learnt from: haitham911
PR: cloudposse/atmos#727
File: internal/exec/terraform_clean.go:407-416
Timestamp: 2024-11-24T19:13:10.287Z
Learning: In `internal/exec/terraform_clean.go`, when `getStackTerraformStateFolder` returns an error in the `handleCleanSubCommand` function, the error is logged, and the process continues without returning the error.
Learnt from: Cerebrovinny
PR: cloudposse/atmos#764
File: internal/exec/describe_stacks.go:289-295
Timestamp: 2024-11-13T21:37:07.852Z
Learning: In the `internal/exec/describe_stacks.go` file of the `atmos` project written in Go, avoid extracting the stack name handling logic into a helper function within the `ExecuteDescribeStacks` method, even if the logic appears duplicated.
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Summary
🔇 Additional comments (7)
internal/exec/terraform_clean.go (7)
18-36: Good approach with standardized error variables.Creating a comprehensive set of static error variables improves error handling consistency across the codebase. This addresses the linter warnings from previous reviews and makes the code more maintainable.
235-252: Good addition of getRelativePath function.This helper function improves path handling and provides a more user-friendly way to represent file paths relative to the base directory.
322-326: Good error handling with fallback.Using getRelativePath with a fallback mechanism ensures the code still works even if relative path calculation fails. This makes the code more robust.
401-413: Improved cross-platform validation in IsValidDataDir.Adding specific checks for both Unix and Windows root paths enhances the safety of the deletion process across different operating systems.
453-454: Standardized error wrapping for DescribeStack errors.Using the ErrDescribeStack error variable provides consistent error handling for stack description failures.
76-76: Good transition to structured logging.Replacing u.LogWarning with structured logging using log.Debug and log.Warn improves the consistency and readability of logs. Including field names like "directory", "path", and "error" makes the logs more searchable and informative.
Also applies to: 324-325, 340-341, 348-349, 361-362, 365-366, 369-370, 464-465, 475-476, 479-480
444-459:✅ Verification successful
Key improvement for handling components in subdirectories.
This refactoring effectively addresses the main PR objective by supporting Terraform components in subdirectories. Using ExecuteDescribeStacks to get all component paths is a solid approach.
🏁 Script executed:
#!/bin/bash # Verify if the code can successfully find components in subdirectories # Check if there are test cases for components in subdirectories echo "Checking test coverage for components in subdirectories..." grep -r "subfolder" internal/exec/terraform_clean_test.go || echo "No tests found specifically for subfolder components" # Check the implementation of getAllStacksComponentsPaths function echo "Checking implementation of getAllStacksComponentsPaths..." grep -A 20 "getAllStacksComponentsPaths" internal/exec/terraform_clean_util.go || echo "Function not found in terraform_clean_util.go"Length of output: 1224
✅ Subdirectory component handling verified
- The test “Components with nested subfolders” exists and covers nested component paths.
getAllStacksComponentsPathscorrectly aggregates all component paths, including those in subdirectories.No further changes needed—nice work!
|
These changes were released in v1.174.0. |
what
why
Summary by CodeRabbit
New Features
Tests
Chores