add terraform clean --everything and terraform clean --force to delete terraform.tfstate.d folder
#727
Conversation
…thing` flag is provided.
📝 Walkthrough📝 WalkthroughWalkthroughThis pull request introduces several changes to the CLI functionality of the Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Warning Rate limit exceeded@haitham911 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 3 minutes and 3 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (1)
- internal/exec/terraform.go (3 hunks)
🧰 Additional context used
🔇 Additional comments (2)
internal/exec/terraform.go (2)
24-24: LGTM: New constant for the--everythingflag.The new constant
everythingis well-named and consistent with other constant declarations in the file. It correctly represents the--everythingflag mentioned in the PR objectives.
24-24: Summary: Successfully implemented the--everythingflag for Terraform state cleaning.The changes in this file successfully implement the new
--everythingflag for theatmos terraform cleancommand as described in the PR objectives. The implementation is well-integrated into the existing code structure and follows the established patterns in the codebase.Key points:
- A new constant
everythingis added to represent the flag.- The
cleansubcommand logic is extended to handle the new flag.- When the flag is present, the code correctly targets and deletes the specific Terraform state folder for the component.
These changes enhance the Atmos tool's functionality by providing users with more control over cleaning Terraform state files, which can be beneficial in testing scenarios as mentioned in the PR description.
Also applies to: 107-114
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (1)
- internal/exec/terraform.go (2 hunks)
🧰 Additional context used
🔇 Additional comments (2)
internal/exec/terraform.go (2)
24-24: LGTM: New constant for the--everythingflag.The addition of the
everythingconstant with the value"--everything"is appropriate and aligns with the PR objectives.
Line range hint
1-669: Overall: Implementation meets PR objectives.The changes successfully implement the
--everythingflag functionality for thecleansubcommand as described in the PR objectives. The new constant and the modifications to theExecuteTerraformfunction are correct and well-integrated into the existing code structure. With the minor suggestion for adding a comment addressed, this implementation is ready for merging.
|
Here's what we should aim to replicate with https://github.com/cloudposse/atmos/blob/main/demo/screengrabs/scripts/demo-stacks/.demo.rc#L17-L22 Of this, I think we're missing also the Also, if no component is passed, can we then process all components? This would clean all components: and this would clean the vpc component. Note, many stacks can share the same component. Also, please make sure to update the related docs for |
aknysh
added
the
no-release
The docs need to be updted. @haitham911 let me know if you need help on updating the docs |
… a prefix and deleting files and folders recursively
There was a problem hiding this comment.
Actionable comments posted: 11
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (3)
- internal/exec/path_utils.go (2 hunks)
- internal/exec/terraform.go (3 hunks)
- internal/exec/utils.go (2 hunks)
🧰 Additional context used
🔇 Additional comments (5)
internal/exec/terraform.go (4)
24-24: LGTM: Addition ofeverythingFlagconstantThe new constant
everythingFlagis correctly defined and follows the existing naming convention for flags in this file. This addition improves code readability and maintainability by centralizing the flag definition.
76-83: LGTM: UpdatedProcessStackscall and error handlingThe changes to the
ProcessStacksfunction call and the subsequent error handling are appropriate. The introduction of theCheckStackparameter allows for more flexible behavior when processing stacks, which aligns well with the new--everythingflag functionality.
Line range hint
1-644: Overall assessment: Implementation aligns with PR objectivesThe changes to
internal/exec/terraform.gosuccessfully implement the--everythingflag functionality for thecleansubcommand as described in the PR objectives. The code is well-structured and covers the required scenarios for cleaning Terraform state files.Key points:
- The
everythingFlagconstant is correctly defined.- The logic for handling the
--everythingflag is properly integrated into theExecuteTerraformfunction.- The implementation covers scenarios for cleaning all components, a specific component, or a specific component and stack.
Minor improvements suggested:
- Remove a debug print statement.
- Consider enhancing error handling in some cases.
- Verify the implementation of new helper functions used in the changes.
Overall, the changes appear to meet the requirements and improve the functionality of the Atmos tool for Terraform state file management.
110-144: LGTM: Implementation of--everythingflag forcleansubcommandThe implementation of the
--everythingflag for thecleansubcommand is comprehensive and aligns well with the PR objectives. It correctly handles different scenarios such as cleaning all components, a specific component, or a specific component and stack.A few points to consider:
The functions
deleteFilesAndFoldersRecursiveandfindFoldersNamesWithPrefixare used but not defined in this file. Could you provide information about where these functions are defined and their exact behavior?The current error handling only logs warnings without stopping execution. Consider whether some errors should halt the process instead of continuing.
To verify the implementation of the new functions, please run the following script:
This will help ensure that these functions are properly defined and available for use in this context.
✅ Verification successful
LGTM: Implementation of
--everythingflag forcleansubcommandThe implementation of the
--everythingflag for thecleansubcommand is comprehensive and aligns well with the PR objectives. It correctly handles different scenarios such as cleaning all components, a specific component, or a specific component and stack.A few points to consider:
- The functions
deleteFilesAndFoldersRecursiveandfindFoldersNamesWithPrefixare defined ininternal/exec/path_utils.go, ensuring their availability and correctness.- The current error handling only logs warnings without stopping execution. Consider whether some errors should halt the process instead of continuing to enhance reliability.
To further improve, you might want to evaluate the error handling strategy to determine if certain failures should prevent the continuation of the cleanup process.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for the definitions of the new functions echo "Searching for deleteFilesAndFoldersRecursive function:" rg --type go "func deleteFilesAndFoldersRecursive" echo "Searching for findFoldersNamesWithPrefix function:" rg --type go "func findFoldersNamesWithPrefix"Length of output: 537
internal/exec/path_utils.go (1)
139-175: Avoid usingfilepath.Walkdue to potential performance issues.Using
filepath.Walkcan be inefficient for large directory structures. Since we only need to check immediate subdirectories, consider usingos.ReadDirfor better performance.[performance]
Apply this change to improve efficiency:
subDirs, err := os.ReadDir(basePath) if err != nil { return fmt.Errorf("error reading base path %s: %w", basePath, err) } for _, subDir := range subDirs { if subDir.IsDir() { subDirPath := filepath.Join(basePath, subDir.Name()) for _, item := range items { fullPath := filepath.Join(subDirPath, item) // Proceed with deletion logic } } }This avoids recursively walking through the entire directory structure.
There was a problem hiding this comment.
Actionable comments posted: 7
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (2)
- internal/exec/help.go (2 hunks)
- internal/exec/path_utils.go (2 hunks)
🧰 Additional context used
🔇 Additional comments (3)
internal/exec/help.go (2)
77-78: Approved: Added information about deleting state filesThe addition of information about deleting the 'terraform.tfstate.d' folder when using the
--everythingflag is consistent with the PR objectives and provides crucial information about the expanded functionality of thecleancommand.
Line range hint
33-84: Overall assessment: Significant improvement in help messagesThe changes to the
processHelpfunction ininternal/exec/help.gohave significantly enhanced the clarity and comprehensiveness of the help messages for theatmos terraform cleancommand. These improvements include:
- Detailed explanation of the
--everythingflag's functionality- Introduction of the
--skip-lock-fileflag- Clear description of the command's behavior when no component or stack is specified
- Addition of a link to the documentation for further details
These enhancements align perfectly with the PR objectives and will greatly improve the user experience by providing more accurate and detailed information about the command's capabilities and usage.
internal/exec/path_utils.go (1)
92-126:findFoldersNamesWithPrefixfunction implementation looks goodThe function correctly finds folders with the specified prefix in both the root directory and its immediate subdirectories. Error handling and directory traversal are appropriately implemented.
There was a problem hiding this comment.
Actionable comments posted: 7
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (3)
- internal/exec/path_utils.go (2 hunks)
- internal/exec/terraform.go (3 hunks)
- internal/exec/utils.go (2 hunks)
🧰 Additional context used
🔇 Additional comments (5)
internal/exec/terraform.go (3)
24-24: LGTM: Addition ofeverythingFlagconstantThe new constant
everythingFlagis correctly defined and aligns with the PR objectives to introduce a comprehensive cleaning option.
Line range hint
144-193: LGTM: Preservation of existing cleaning logicThe existing cleaning logic for
.terraformfolder,.terraform.lock.hclfile, and other artifacts is preserved. This maintains backwards compatibility while the new--everythingflag logic complements the existing process.
Line range hint
1-1000: Overall LGTM: Well-implemented--everythingflag for Terraform cleanThe implementation of the
--everythingflag for the Terraformcleansubcommand is well-done and aligns closely with the PR objectives. The code is structured logically, follows existing patterns, and maintains backwards compatibility. Error handling and logging are appropriately implemented.Minor suggestions for improvement:
- Use camelCase for local variables (
needProcessStacks,checkStack).- Rename
listOfCleartofilesToClearfor better clarity.These small changes would enhance readability and consistency with Go naming conventions.
internal/exec/utils.go (2)
394-397: LGTM!The added condition correctly allows the function to proceed without error when
checkStackis false and no stacks are found. This enhances the flexibility for operations that don't require a stack.
1003-1009: LGTM!The updated argument parsing logic now properly differentiates between options and component names. This improves the robustness and clarity of the function.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
website/docs/cli/commands/terraform/terraform-clean.mdx (2)
Line range hint
67-67: Document the new flags in the Flags section.Warriors, the new flags need to be documented in the battlefield manual! Add the following entries to the Flags table:
| Flag | Description | Alias | Required | | :----------------- | :-------------------------------- | :---- | :------- | | `--stack` | Atmos stack | `-s` | yes | | `--dry-run` | Dry run | | no | | `--skip-lock-file` | Skip deleting .terraform.lock.hcl | | no | +| `--everything` | Delete all Terraform-related files including state files | | no | +| `--force` | Skip confirmation prompts for destructive operations | | no |
25-28: Enhance the warning message with specific file details.While the current warning is good, let's make it even more specific about what files will be deleted:
:::warning -The `--everything` flag will delete all Terraform-related files including state files. The `--force` flag will bypass confirmation prompts. +The `--everything` flag will delete all Terraform-related files including: +- `backend.tf.json` +- `.terraform` directory +- `terraform.tfstate.d` directory +- `.terraform.lock.hcl` + +The `--force` flag will bypass confirmation prompts. Use these flags with extreme caution as they can lead to irreversible data loss. :::website/docs/cli/commands/terraform/usage.mdx (2)
61-63: Enhance flag documentation with specific file details.For completeness, let's specify all files that will be deleted with the
--everythingflag:Use the `--everything` flag to delete all the local Terraform state files and directories (including `terraform.tfstate.d`) for all components and stacks. + The following files and directories will be deleted: + - `backend.tf.json` + - `.terraform` + - `terraform.tfstate.d` + - `.terraform.lock.hcl`
Line range hint
1-1: Document component name parsing improvement.The PR includes a bug fix related to component name parsing from command arguments. Consider adding a note about this improvement in the documentation to help users understand the enhanced behavior.
🧰 Tools
🪛 LanguageTool
[grammar] ~58-~58: Please add a punctuation mark at the end of paragraph.
Context: ...E>to apply the previously generated planfile -atmos terraform clean` command dele...(PUNCTUATION_PARAGRAPH_END)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
website/docs/cli/commands/terraform/terraform-clean.mdx(2 hunks)website/docs/cli/commands/terraform/usage.mdx(2 hunks)
🧰 Additional context used
📓 Learnings (1)
website/docs/cli/commands/terraform/terraform-clean.mdx (1)
Learnt from: osterman
PR: cloudposse/atmos#727
File: internal/exec/terraform.go:114-118
Timestamp: 2024-11-12T05:52:05.088Z
Learning: When `atmos terraform clean --everything` is used without specifying a component and without the `--force` flag, prompt the user for confirmation before deleting all components. Use the `--force` flag to skip the confirmation prompt.
🔇 Additional comments (3)
website/docs/cli/commands/terraform/terraform-clean.mdx (2)
23-28: LGTM! Clear usage and warning message.
The warning message effectively communicates the risks of using these destructive flags. This aligns well with the spartan principle of clear communication in battle!
38-42: Well-documented examples with clear impact descriptions!
The examples now include helpful comments explaining what each command does. This improvement addresses the previous review feedback.
website/docs/cli/commands/terraform/usage.mdx (1)
65-67: LGTM! Clear and effective warning message.
The warning effectively communicates the risks and provides the crucial prerequisite of having remote state configured.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
internal/exec/terraform_clean.go (2)
363-365: Remove redundant path checkThe check for
".."is unnecessary afterfilepath.Absas the path is already resolved.-if strings.Contains(absTFDataDir, "..") { - return fmt.Errorf("refusing to delete directory containing '..'") -}
454-457: Improve error handling for user cancellationWhen the user cancels the operation, return a specific error to differentiate from other errors.
if confirm, err := confirmDeletion(cliConfig); err != nil || !confirm { - return err + if err != nil { + return err + } + return fmt.Errorf("operation cancelled by user") }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
internal/exec/terraform_clean.go(1 hunks)
🧰 Additional context used
📓 Learnings (1)
internal/exec/terraform_clean.go (2)
Learnt from: osterman
PR: cloudposse/atmos#727
File: internal/exec/terraform_clean.go:233-235
Timestamp: 2024-11-12T05:52:05.088Z
Learning: When specifying color values in functions like `confirmDeleteTerraformLocal` in `internal/exec/terraform_clean.go`, avoid hardcoding color values. Instead, use predefined color constants or allow customization through configuration settings to improve accessibility and user experience across different terminals and themes.
Learnt from: haitham911
PR: cloudposse/atmos#727
File: internal/exec/terraform_clean.go:183-187
Timestamp: 2024-11-12T05:52:05.088Z
Learning: In the `getStackTerraformStateFolder` function, it's acceptable and not an error if no Terraform state folders are found for a stack.
🔇 Additional comments (2)
internal/exec/terraform_clean.go (2)
53-54:
Pass proper CliConfig for consistent logging
The function uses an empty schema.CliConfiguration{} which may lead to inconsistent logging behavior.
-u.LogWarning(schema.CliConfiguration{}, fmt.Sprintf("Error reading subdirectory %s: %v", level2Path, err))
+u.LogWarning(cliConfig, fmt.Sprintf("Error reading subdirectory %s: %v", level2Path, err))Likely invalid or redundant comment.
325-329:
Improve path handling for TF_DATA_DIR
When tfDataDir is an absolute path, filepath.Join will ignore componentPath.
-if _, err := os.Stat(filepath.Join(componentPath, tfDataDir)); os.IsNotExist(err) {
+tfDataDirPath := tfDataDir
+if !filepath.IsAbs(tfDataDir) {
+ tfDataDirPath = filepath.Join(componentPath, tfDataDir)
+}
+if _, err := os.Stat(tfDataDirPath); os.IsNotExist(err) {
u.LogWarning(cliConfig, fmt.Sprintf("TF_DATA_DIR '%s' does not exist", tfDataDir))
return
}Likely invalid or redundant comment.
There was a problem hiding this comment.
@haitham911 please remove all pretty print formatting from src/components/Screengrabs/* as these are very sensitive to whitespace changes
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
internal/exec/terraform_clean.go (4)
354-369: Simplify path validation inIsValidDataDirThe check for
".."is redundant sincefilepath.Absalready resolves any parent directory references in the path.func IsValidDataDir(tfDataDir string) error { if tfDataDir == "" { return fmt.Errorf("ENV TF_DATA_DIR is empty") } absTFDataDir, err := filepath.Abs(tfDataDir) if err != nil { return fmt.Errorf("error resolving TF_DATA_DIR path: %v", err) } if absTFDataDir == "/" || absTFDataDir == filepath.Clean("/") { return fmt.Errorf("refusing to delete root directory '/'") } - if strings.Contains(absTFDataDir, "..") { - return fmt.Errorf("refusing to delete directory containing '..'") - } return nil }
233-239: Consider using theme-aware colorsHardcoded color values may not render well across different terminal themes. Consider using predefined color constants or allowing customization through configuration.
256-276: Use consistent logging approachThe function uses direct
fmt.Printfandfmt.Printlncalls instead of the logging utilities used elsewhere in the codebase.func DeletePathTerraform(fullPath string, objectName string) error { if _, err := os.Stat(fullPath); os.IsNotExist(err) { xMark := lipgloss.NewStyle().Foreground(lipgloss.Color("9")).SetString("x") - fmt.Printf("%s Cannot delete %s: path does not exist", xMark, objectName) - fmt.Println() + u.LogWarning(cliConfig, fmt.Sprintf("%s Cannot delete %s: path does not exist", xMark, objectName)) return err } err := os.RemoveAll(fullPath) if err != nil { xMark := lipgloss.NewStyle().Foreground(lipgloss.Color("9")).SetString("x") - fmt.Printf("%s Error deleting %s", xMark, objectName) - fmt.Println() + u.LogWarning(cliConfig, fmt.Sprintf("%s Error deleting %s", xMark, objectName)) return err } checkMark := lipgloss.NewStyle().Foreground(lipgloss.Color("42")).SetString("✓") - fmt.Printf("%s deleted %s", checkMark, objectName) - fmt.Println() + u.LogInfo(cliConfig, fmt.Sprintf("%s deleted %s", checkMark, objectName)) return nil }
371-470: Add unit tests for critical functionalityThe
handleCleanSubCommandfunction contains complex logic for path handling and file operations. Consider adding unit tests to verify its behavior under different scenarios.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
internal/exec/terraform_clean.go(1 hunks)
🧰 Additional context used
📓 Learnings (1)
internal/exec/terraform_clean.go (2)
Learnt from: osterman
PR: cloudposse/atmos#727
File: internal/exec/terraform_clean.go:233-235
Timestamp: 2024-11-12T05:52:05.088Z
Learning: When specifying color values in functions like `confirmDeleteTerraformLocal` in `internal/exec/terraform_clean.go`, avoid hardcoding color values. Instead, use predefined color constants or allow customization through configuration settings to improve accessibility and user experience across different terminals and themes.
Learnt from: haitham911
PR: cloudposse/atmos#727
File: internal/exec/terraform_clean.go:183-187
Timestamp: 2024-11-12T05:52:05.088Z
Learning: In the `getStackTerraformStateFolder` function, it's acceptable and not an error if no Terraform state folders are found for a stack.
…rmStateFolder functions
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (6)
internal/exec/terraform_clean.go (6)
29-66: Consider enhancing error handling for subdirectory operations.While the function handles root directory errors well, the subdirectory error handling could be improved. Currently, errors from reading subdirectories are logged and skipped, which might hide important issues.
Consider collecting and returning all errors:
func findFoldersNamesWithPrefix(root, prefix string, cliConfig schema.CliConfiguration) ([]string, error) { + var errors []error // ... existing code ... if err != nil { - u.LogWarning(cliConfig, fmt.Sprintf("Error reading subdirectory %s: %v", level2Path, err)) - continue + errors = append(errors, fmt.Errorf("error reading subdirectory %s: %w", level2Path, err)) } // ... rest of the code ... - return folderNames, nil + if len(errors) > 0 { + return folderNames, fmt.Errorf("encountered errors while searching directories: %v", errors) + } + return folderNames, nil
68-179: LGTM! Well-structured directory collection logic.The function is well-organized with clear helper functions and comprehensive error handling. The path validation and error handling are thorough.
Consider making error messages more consistent by using the same format:
- return nil, fmt.Errorf("error stating file %s: %v", filePath, err) + return nil, fmt.Errorf("error reading file status for %s: %w", filePath, err)
181-212: Consider using consistent path handling.The function uses platform-specific path separators, which could cause issues across different platforms.
Consider using
filepath.ToSlashfor consistent path handling:- directories[i].Files[j].Name = folderName + "/" + directories[i].Files[j].Name + directories[i].Files[j].Name = filepath.ToSlash(filepath.Join(folderName, directories[i].Files[j].Name))
214-231: Enhance error messages in path resolution.The error messages could be more descriptive to help with debugging.
Consider adding more context to error messages:
func getRelativePath(basePath, componentPath string) (string, error) { absBasePath, err := filepath.Abs(basePath) if err != nil { - return "", err + return "", fmt.Errorf("failed to resolve absolute path for base path %s: %w", basePath, err) } absComponentPath, err := filepath.Abs(componentPath) if err != nil { - return "", err + return "", fmt.Errorf("failed to resolve absolute path for component path %s: %w", componentPath, err) }
336-369: Consider additional validation for file patterns.The initialization and validation are thorough, but could benefit from additional checks.
Consider validating file patterns before using them:
func initializeFilesToClear(info schema.ConfigAndStacksInfo, cliConfig schema.CliConfiguration, everything bool) []string { + validatePattern := func(pattern string) bool { + _, err := filepath.Match(pattern, "test") + return err == nil + } if everything && info.Stack == "" { - return []string{".terraform", ".terraform.lock.hcl", "*.tfvar.json", "terraform.tfstate.d"} + patterns := []string{".terraform", ".terraform.lock.hcl", "*.tfvar.json", "terraform.tfstate.d"} + for _, pattern := range patterns { + if !validatePattern(pattern) { + u.LogWarning(cliConfig, fmt.Sprintf("Invalid pattern: %s", pattern)) + } + } + return patterns }
371-470: Consider improving error handling in the main command.The command handling is thorough, but error handling could be more consistent.
Consider wrapping errors with more context:
if err != nil { - u.LogTrace(cliConfig, fmt.Errorf("error collecting folders and files: %v", err).Error()) - return err + return fmt.Errorf("failed to collect folders and files for cleaning: %w", err) }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
internal/exec/terraform_clean.go(1 hunks)
🧰 Additional context used
📓 Learnings (1)
internal/exec/terraform_clean.go (1)
Learnt from: osterman
PR: cloudposse/atmos#727
File: internal/exec/terraform_clean.go:233-235
Timestamp: 2024-11-12T05:52:05.088Z
Learning: When specifying color values in functions like `confirmDeleteTerraformLocal` in `internal/exec/terraform_clean.go`, avoid hardcoding color values. Instead, use predefined color constants or allow customization through configuration settings to improve accessibility and user experience across different terminals and themes.
🔇 Additional comments (1)
internal/exec/terraform_clean.go (1)
1-28: LGTM! Well-structured data types and imports.
The ObjectInfo and Directory structs are well-designed with clear field names, and the imports are properly organized.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
internal/exec/terraform_clean.go (3)
68-74: Enhance path validation in CollectDirectoryObjectsThe path validation is good, but consider adding checks for path traversal attacks and absolute paths.
if basePath == "" { return nil, fmt.Errorf("path cannot be empty") } +// Clean and validate the path +basePath = filepath.Clean(basePath) +if filepath.IsAbs(basePath) && (basePath == "/" || basePath == "\\") { + return nil, fmt.Errorf("refusing to process root directory") +} if _, err := os.Stat(basePath); os.IsNotExist(err) { return nil, fmt.Errorf("path %s does not exist", basePath) }
235-239: Consider using theme-aware colorsThe hardcoded colors might not be accessible in all terminal themes.
Consider using adaptive colors or theme configurations:
-cream := lipgloss.AdaptiveColor{Light: "#FFFDF5", Dark: "#FFFDF5"} -purple := lipgloss.AdaptiveColor{Light: "#5B00FF", Dark: "#5B00FF"} +cream := lipgloss.AdaptiveColor{Light: t.DefaultTheme.SelectedBackground, Dark: t.DefaultTheme.SelectedBackground} +purple := lipgloss.AdaptiveColor{Light: t.DefaultTheme.PrimaryText, Dark: t.DefaultTheme.PrimaryText}
357-372: Strengthen path validation in IsValidDataDirWhile the current validation is good, it could be enhanced to handle more edge cases.
func IsValidDataDir(tfDataDir string) error { if tfDataDir == "" { return fmt.Errorf("ENV TF_DATA_DIR is empty") } + // Clean the path before validation + tfDataDir = filepath.Clean(tfDataDir) absTFDataDir, err := filepath.Abs(tfDataDir) if err != nil { return fmt.Errorf("error resolving TF_DATA_DIR path: %v", err) } - if absTFDataDir == "/" || absTFDataDir == filepath.Clean("/") { + // Check for root and system directories + if absTFDataDir == "/" || absTFDataDir == filepath.Clean("/") || + strings.HasPrefix(absTFDataDir, "/etc") || + strings.HasPrefix(absTFDataDir, "/usr") { return fmt.Errorf("refusing to delete root directory '/'") } if strings.Contains(absTFDataDir, "..") { return fmt.Errorf("refusing to delete directory containing '..'") } return nil }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
internal/exec/terraform_clean.go(1 hunks)
🧰 Additional context used
📓 Learnings (1)
internal/exec/terraform_clean.go (1)
Learnt from: osterman
PR: cloudposse/atmos#727
File: internal/exec/terraform_clean.go:233-235
Timestamp: 2024-11-12T05:52:05.088Z
Learning: When specifying color values in functions like `confirmDeleteTerraformLocal` in `internal/exec/terraform_clean.go`, avoid hardcoding color values. Instead, use predefined color constants or allow customization through configuration settings to improve accessibility and user experience across different terminals and themes.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
internal/exec/terraform_clean.go (3)
285-291: Standardize error messages in confirmation handlingThe error message "Mission aborted." is used here, while elsewhere we use more formal messages. Consider standardizing the message style across the codebase.
-u.LogWarning(cliConfig, "Mission aborted.") +u.LogWarning(cliConfig, "Operation cancelled by user.")
453-474: Enhance user confirmation message clarityThe confirmation message could be more explicit about the destructive nature of the operation, especially when using the
--everythingflag.Consider adding a warning about the irreversible nature of the operation:
-message = fmt.Sprintf("This will delete %v local terraform state files affecting all components", total) +message = fmt.Sprintf("WARNING: This will permanently delete %v local terraform state files affecting all components", total)
369-384: Strengthen path validation in IsValidDataDirWhile the function checks for root directory and parent directory traversal, it could benefit from additional safety checks.
Consider adding checks for:
- Absolute path resolution
- System-critical directories
- Symbolic link detection
func IsValidDataDir(tfDataDir string) error { if tfDataDir == "" { return fmt.Errorf("ENV TF_DATA_DIR is empty") } absTFDataDir, err := filepath.Abs(tfDataDir) if err != nil { return fmt.Errorf("error resolving TF_DATA_DIR path: %v", err) } + // Check if path is a symlink + if info, err := os.Lstat(absTFDataDir); err == nil && info.Mode()&os.ModeSymlink != 0 { + return fmt.Errorf("refusing to process symbolic link: %s", tfDataDir) + } if absTFDataDir == "/" || absTFDataDir == filepath.Clean("/") { return fmt.Errorf("refusing to delete root directory '/'") } if strings.Contains(absTFDataDir, "..") { return fmt.Errorf("refusing to delete directory containing '..'") } + // Add checks for system-critical directories + criticalPaths := []string{"/etc", "/usr", "/var", "/bin"} + for _, path := range criticalPaths { + if strings.HasPrefix(absTFDataDir, path) { + return fmt.Errorf("refusing to delete system directory: %s", tfDataDir) + } + } return nil }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
internal/exec/terraform_clean.go(1 hunks)
🧰 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: osterman
PR: cloudposse/atmos#727
File: internal/exec/terraform_clean.go:233-235
Timestamp: 2024-11-12T05:52:05.088Z
Learning: When specifying color values in functions like `confirmDeleteTerraformLocal` in `internal/exec/terraform_clean.go`, avoid hardcoding color values. Instead, use predefined color constants or allow customization through configuration settings to improve accessibility and user experience across different terminals and themes.
🔇 Additional comments (2)
internal/exec/terraform_clean.go (2)
15-28: Well-structured data types for file system operations!
The ObjectInfo and Directory structs are well-designed with clear field names and appropriate types for representing file system objects.
342-346: 🛠️ Refactor suggestion
Enhance path handling for TF_DATA_DIR
When handling TF_DATA_DIR, the code should handle both absolute and relative paths correctly.
Consider this approach:
-if _, err := os.Stat(filepath.Join(componentPath, tfDataDir)); os.IsNotExist(err) {
+tfDataDirPath := tfDataDir
+if !filepath.IsAbs(tfDataDir) {
+ tfDataDirPath = filepath.Join(componentPath, tfDataDir)
+}
+if _, err := os.Stat(tfDataDirPath); os.IsNotExist(err) {
u.LogWarning(cliConfig, fmt.Sprintf("TF_DATA_DIR '%s' does not exist", tfDataDir))
return
}Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
internal/exec/terraform_clean.go (4)
15-28: Add documentation for type definitionsThe
ObjectInfoandDirectorystructs would benefit from documentation explaining their purpose and field usage. Consider adding comments in the standard Go documentation format.+// ObjectInfo represents metadata about a file or directory in the filesystem type ObjectInfo struct { + // FullPath is the absolute path to the file FullPath string + // RelativePath is the path relative to the base directory RelativePath string + // Name is the base name of the file Name string + // IsDir indicates whether this is a directory IsDir bool } +// Directory represents a directory containing files to be processed type Directory struct { + // Name is the base name of the directory Name string + // FullPath is the absolute path to the directory FullPath string + // RelativePath is the path relative to the base directory RelativePath string + // Files contains the files and subdirectories within this directory Files []ObjectInfo }
31-66: Consider additional validation in findFoldersNamesWithPrefixWhile the function handles basic error cases, consider adding:
- Maximum depth validation to prevent excessive traversal
- Path sanitization to prevent path traversal attacks
- Size limits for the returned slice to prevent memory issues
func findFoldersNamesWithPrefix(root, prefix string, cliConfig schema.CliConfiguration) ([]string, error) { var folderNames []string + const maxResults = 1000 // Prevent unbounded results if root == "" { return nil, fmt.Errorf("root path cannot be empty") } + // Sanitize and validate the root path + cleanRoot := filepath.Clean(root) + if strings.Contains(cleanRoot, "..") { + return nil, fmt.Errorf("invalid root path: contains parent directory reference") + }
256-279: Use consistent logging approachThe function uses direct
fmt.Printfcalls instead of the logging system used elsewhere in the codebase. Consider using the unified logging functions for consistency.- fmt.Printf("%s Cannot delete %s: path does not exist", xMark, objectName) - fmt.Println() + u.LogWarning(cliConfig, fmt.Sprintf("%s Cannot delete %s: path does not exist", xMark, objectName)) - fmt.Printf("%s Error deleting %s", xMark, objectName) - fmt.Println() + u.LogError(cliConfig, fmt.Sprintf("%s Error deleting %s", xMark, objectName)) - fmt.Printf("%s Deleted %s", checkMark, objectName) - fmt.Println() + u.LogInfo(cliConfig, fmt.Sprintf("%s Deleted %s", checkMark, objectName))
384-483: Enhance error handling in handleCleanSubCommandConsider improving error handling in the following areas:
- Error from
getStackTerraformStateFolderis logged but not propagated- Errors during
tfDataDirFolderscollection are only loggedif err != nil { errMsg := fmt.Errorf("error getting stack terraform state folders: %v", err) u.LogTrace(cliConfig, errMsg.Error()) + if !force { + return errMsg + } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
internal/exec/terraform_clean.go(1 hunks)
🧰 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: osterman
PR: cloudposse/atmos#727
File: internal/exec/terraform_clean.go:233-235
Timestamp: 2024-11-12T05:52:05.088Z
Learning: When specifying color values in functions like `confirmDeleteTerraformLocal` in `internal/exec/terraform_clean.go`, avoid hardcoding color values. Instead, use predefined color constants or allow customization through configuration settings to improve accessibility and user experience across different terminals and themes.
🔇 Additional comments (1)
internal/exec/terraform_clean.go (1)
329-348: 🛠️ Refactor suggestion
Improve path handling for TF_DATA_DIR
When handling TF_DATA_DIR, the function should consider whether the path is absolute or relative before joining it with componentPath.
- if _, err := os.Stat(filepath.Join(componentPath, tfDataDir)); os.IsNotExist(err) {
+ tfDataDirPath := tfDataDir
+ if !filepath.IsAbs(tfDataDir) {
+ tfDataDirPath = filepath.Join(componentPath, tfDataDir)
+ }
+ if _, err := os.Stat(tfDataDirPath); os.IsNotExist(err) {
u.LogWarning(cliConfig, fmt.Sprintf("TF_DATA_DIR '%s' does not exist", tfDataDir))
return
}Likely invalid or redundant comment.
What
terraform cleancalled--everythingandterraform cleancalled--forceto clean state files. This option deletes the Terraform-related folder and files, including:
backend.tf.json.terraformterraform.tfstate.d.terraform.lock.hclThe following scenarios are covered:
If no component is specified:
cmd atmos terraform clean --everything delete with confim message yes/no
cmd atmos terraform clean --force .delete force
Cleans the state files for all components.
If a specific component is specified:
Cleans the state files for the specified component.
If both a component and a stack are specified:
Cleans the state files for the specified component and stack.
terraform clean --everything, the component name was mistakenly set to--everything).Why
Cleaning state files is useful when running tests, but it should not be the default behavior to avoid unintended data loss.
References
https://linear.app/cloudposse/issue/DEV-346/atmos-terraform-clean-everything-should-clean-statefiles
Summary by CodeRabbit
New Features
atmos terraform cleancommand, detailing the new--everythingand--forceflags.atmos terraformcommands to improve usability, including--skip-init,--from-plan, and--planfile.Bug Fixes
Chores