Fixes and Improvements for terraform clean #870
Conversation
📝 WalkthroughWalkthroughThe pull request focuses on enhancing the functionality of the Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
✨ Finishing Touches
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: 0
🧹 Nitpick comments (1)
internal/exec/terraform.go (1)
Line range hint
78-98: Overall changes effectively address all reported bugs.The modifications to stack processing and component validation logic successfully fix all three reported issues with the
atmos terraform cleancommand. The changes are minimal, focused, and maintain the existing behavior for other commands.Consider adding integration tests to cover these specific scenarios to prevent regression:
atmos terraform clean --everythingatmos terraform clean -s <stack>atmos terraform clean <component>
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/exec/terraform.go(2 hunks)
🧰 Additional context used
📓 Learnings (1)
internal/exec/terraform.go (1)
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.
🔇 Additional comments (2)
internal/exec/terraform.go (2)
96-98: LGTM! Component enabled check now correctly handles clean command.
The condition info.SubCommand != "clean" ensures that disabled components can still be cleaned up, while maintaining the validation for other commands.
78-83: LGTM! Stack processing logic aligns with command requirements.
The changes correctly handle all three use cases:
clean --everything: No stack processing when component is emptyclean -s <stack>: Stack check only when stack is providedclean <component>: No stack requirement when only component is provided
Let's verify the command behavior:
✅ Verification successful
Stack processing logic changes are correctly implemented and verified!
The code changes align perfectly with the intended behavior:
- When no component is provided (
ComponentFromArg == ""), stack processing is skipped, which correctly handlesclean --everything - Stack validation (
shouldCheckStack) only occurs when a stack is explicitly provided, fixing theclean -s <stack>case - The implementation in
handleCleanSubCommandconfirms proper handling of component-only cleaning without requiring a stack
The changes are supported by:
- Clear error handling in
handleCleanSubCommand - Proper integration with the command help system
- Consistent behavior with the documented CLI usage at
atmos.tools/cli/commands/terraform/clean
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for tests covering these scenarios
# Test: Look for clean command test cases
rg -A 5 "TestExecuteTerraform.*Clean"
# Test: Search for clean command implementation details
ast-grep --pattern 'func $_(t *testing.T) {
$$$
clean
$$$
}'
Length of output: 108
Script:
#!/bin/bash
# Let's try a broader search for test files and clean command implementation
# Search for test files related to terraform execution
fd "terraform.*test.*" --type f --exec rg -l "clean"
# Look for clean command implementation in terraform.go
rg -B 5 -A 10 "SubCommand == \"clean\"" internal/exec/terraform.go
# Search for any test functions that might test clean functionality
rg -B 2 -A 5 "func Test.*" --type go | rg -B 2 -A 5 -i "clean"
Length of output: 1832
Script:
#!/bin/bash
# Let's examine the handleCleanSubCommand function implementation
# Search for handleCleanSubCommand function
ast-grep --pattern 'func handleCleanSubCommand($$$) {
$$$
}'
# Look for any related error handling or validation logic
rg -B 2 -A 5 "handleCleanSubCommand" --type go
# Check for any clean-related constants or types that might affect the behavior
rg "clean" -i --type go internal/exec/
Length of output: 4429
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (2)
website/docs/cli/commands/terraform/usage.mdx (1)
Line range hint
62-66: Ensure consistency with the clean command documentation.The description of the
--everythingflag's behavior should be consistent with the dedicated clean command documentation and clearly explain the confirmation prompt behavior.- Use the `--everything` flag is the default behavior to delete all the local Terraform state files and directories (including `terraform.tfstate.d`) for all components and stacks. - Use the `--force` flag to bypass the safety confirmation prompt and force the deletion (use with caution). - - :::warning - The `--everything` flag performs destructive operations that can lead to permanent state loss. Always ensure you have remote state configured in your components before proceeding. - ::: + The command now defaults to the behavior of the `--everything` flag, which means it will: + - Delete all local Terraform state files and directories (including `terraform.tfstate.d`) + - Apply to all components and stacks + - Show a confirmation prompt before proceeding + + Use the `--force` flag to bypass the safety confirmation prompt (use with extreme caution). + + :::danger Data Loss Risk + This command performs destructive operations that can lead to permanent state loss: + 1. Always ensure you have remote state configured in your components + 2. Verify your state backups are current + 3. Consider the impact on other team members + :::internal/exec/help.go (1)
Line range hint
82-87: Fix typos and improve clarity in help text.The help text contains typos and could be clearer about the default behavior.
- "Use '--everything' flag —everything is the default behavior to delete the Terraform state files and and directories with confirm message.\n\n" + - "Use --force to forcefully delete Terraform state files and directories for the component.\n\n" + - "- If no component is specified, the command will apply to all components and stacks.\n" + - "- If no stack is specified, the command will apply to all stacks for the specified component.\n" + - "Use '--skip-lock-file' flag to skip deleting the '.terraform.lock.hcl' file.\n\n" + - "If no component or stack is specified, the clean operation will apply globally to all components.\n\n" + + "IMPORTANT: By default, this command will attempt to delete all Terraform state files and directories.\n" + + "A confirmation prompt will be shown before deletion.\n\n" + + "Flags:\n" + + " --force Bypass the confirmation prompt (use with extreme caution)\n" + + " --skip-lock-file Skip deleting the '.terraform.lock.hcl' file\n\n" + + "Scope:\n" + + " - No component specified: Applies to all components and stacks\n" + + " - No stack specified: Applies to all stacks for the specified component\n\n" +Issues fixed:
- Removed duplicate "and" in "state files and and directories"
- Fixed em dash usage
- Removed redundant information about global operation
- Improved formatting for better readability
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
internal/exec/help.go(1 hunks)internal/exec/utils.go(1 hunks)website/docs/cli/commands/terraform/terraform-clean.mdx(1 hunks)website/docs/cli/commands/terraform/usage.mdx(1 hunks)
🧰 Additional context used
📓 Learnings (3)
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.
website/docs/cli/commands/terraform/usage.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.
internal/exec/help.go (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 (1)
internal/exec/utils.go (1)
660-664: LGTM! This change makes --everything the default behavior for the clean command.
The modification correctly addresses the issue where atmos terraform clean --everything was not working as expected. Now the --everything flag is automatically appended for the clean command if not already present.
|
@coderabbitai can you add integration tests? |
|
Tip For best results, initiate chat on the files or code changes. @osterman: Here's a draft of minimal integration tests for the “terraform clean” command. The example below shows how you might structure a new test file named “terraform_clean_integration_test.go” in a dedicated integration tests folder (for instance, create a “test/integration” directory if it doesn't exist). This code invokes the “atmos terraform clean” command with different flag combinations, checking for exit codes and capturing stdout/stderr as needed. Below is an illustrative snippet in Go: package integration_test import ( func TestTerraformCleanEverything(t *testing.T) { func TestTerraformCleanSpecificStack(t *testing.T) { func TestTerraformCleanComponentNoStack(t *testing.T) { func TestTerraformCleanComponentAndStack(t *testing.T) { You can adjust the checks (e.g., the success string “deleted”) to something more reliable or parse logs or state files directly. If needed, you can also run prior setup steps in each test—such as creating dummy Terraform resources—to validate the cleanup more comprehensively. Hope this helps you get started! 🔗 Analysis chain🏁 Scripts executedThe following scripts were executed for the analysis: Script: Length of output: 3303 Script: Length of output: 2658 |
There was a problem hiding this comment.
@haitham911 in the docs, we have the following examples
# Delete all Terraform-related files for all components (with confirmation)
atmos terraform clean
# Force delete all Terraform-related files for all components (no confirmation)
atmos terraform clean --force
atmos terraform clean top-level-component1 -s tenant1-ue2-dev
atmos terraform clean infra/vpc -s tenant1-ue2-staging
atmos terraform clean infra/vpc -s tenant1-ue2-staging --skip-lock-file
atmos terraform clean test/test-component -s tenant1-ue2-dev
atmos terraform clean test/test-component-override-2 -s tenant2-ue2-prod
atmos terraform clean test/test-component-override-3 -s tenant1-ue2-devwhen you specify a component and a stack, it works.
the command atmos terraform clean --force works too.
but atmos terraform clean does not, it throws the error
'stack' is required. Usage: atmos terraform <command> <component> -s <stack>please review it and make it behave according to the examples
Please also update the PT description in ##what - some items in there are not corerct, for example
atmos terraform clean -s also just outputs the help
atmos terraform clean -s does work
but it does not, it throws the error
flag needs an argument: 's' in -sPlease update the items in the description to reflect the actual behavior and what was done in the PR
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/cli_terraform_test.go (1)
77-92: Improve error handling in helper functions.The error handling could be more descriptive and provide better debugging information.
func runTerraformApply(t *testing.T, binaryPath, environment string) { cmd := exec.Command(binaryPath, "terraform", "apply", "station", "-s", environment) envVars := os.Environ() envVars = append(envVars, "ATMOS_COMPONENTS_TERRAFORM_APPLY_AUTO_APPROVE=true") cmd.Env = envVars var stdout, stderr bytes.Buffer cmd.Stdout = &stdout cmd.Stderr = &stderr err := cmd.Run() - t.Log(stdout.String()) + output := stdout.String() + t.Logf("Apply command output for environment %s:\n%s", environment, output) if err != nil { - t.Fatalf("Failed to run terraform apply station -s %s: %v", environment, stderr.String()) + t.Fatalf("Failed to run terraform apply for environment %s:\nCommand: %v\nError: %v\nStderr: %s\nStdout: %s", + environment, cmd.Args, err, stderr.String(), output) } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
internal/exec/terraform.go(2 hunks)tests/cli_terraform_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/exec/terraform.go
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Summary
🔇 Additional comments (2)
tests/cli_terraform_test.go (2)
28-33: Add cleanup in defer block to ensure test resources are cleaned.The defer block only restores the working directory. Add cleanup of test resources to prevent test interference.
defer func() { + // Clean up test resources + cmd := exec.Command(binaryPath, "terraform", "clean", "--force") + var stdout, stderr bytes.Buffer + cmd.Stdout = &stdout + cmd.Stderr = &stderr + if err := cmd.Run(); err != nil { + t.Logf("Warning: Cleanup failed: %v\n%s", err, stderr.String()) + } // Change back to the original working directory after the test if err := os.Chdir(startingDir); err != nil { t.Fatalf("Failed to change back to the starting directory: %v", err) } }()
47-50: Verify command outputs to ensure correct error messages.The test runs all required scenarios but doesn't verify the command outputs mentioned in PR objectives:
--everythingflag output- Stack-only command help message
- Component-only command error message
Add output verification:
runTerraformCleanForce(t, binaryPath) +// Verify outputs +func verifyCommandOutput(t *testing.T, cmd *exec.Cmd, expectedOutput string) { + var stdout bytes.Buffer + cmd.Stdout = &stdout + _ = cmd.Run() + if !strings.Contains(stdout.String(), expectedOutput) { + t.Errorf("Expected output to contain %q, got: %s", expectedOutput, stdout.String()) + } +}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
tests/cli_terraform_test.go (3)
28-33: Add cleanup in defer block to ensure test resources are cleaned.The defer block only changes back to the original directory. Add cleanup to ensure test resources are removed even if the test fails.
defer func() { + // Clean up test resources + cleanCmd := exec.Command(binaryPath, "terraform", "clean", "--force") + _ = cleanCmd.Run() // Change back to the original working directory after the test if err := os.Chdir(startingDir); err != nil { t.Fatalf("Failed to change back to the starting directory: %v", err) } }()
47-55: Organize clean command tests into subtests.Group related test cases using
t.Run()for better organization and independent execution.- // Force clean everything - runTerraformCleanCommand(t, binaryPath, "--force") - // Clean everything - runTerraformCleanCommand(t, binaryPath) - // Clean specific component - runTerraformCleanCommand(t, binaryPath, "station") - // Clean component with stack - runTerraformCleanCommand(t, binaryPath, "station", "-s", "dev") + testCases := []struct { + name string + args []string + }{ + {"ForceCleanEverything", []string{"--force"}}, + {"CleanEverything", nil}, + {"CleanSpecificComponent", []string{"station"}}, + {"CleanComponentWithStack", []string{"station", "-s", "dev"}}, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + runTerraformCleanCommand(t, binaryPath, tc.args...) + }) + }
171-174: Improve error messages in clean command execution.The error message could be more descriptive by including the command that failed.
if err != nil { - t.Fatalf("Failed to run terraform clean: %v", stderr.String()) + t.Fatalf("Failed to run 'atmos %s': %v", strings.Join(cmdArgs, " "), stderr.String()) }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/cli_terraform_test.go(1 hunks)
🧰 Additional context used
🪛 golangci-lint (1.62.2)
tests/cli_terraform_test.go
151-151: func runCLITerraformClean is unused
(unused)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Summary
🔇 Additional comments (1)
tests/cli_terraform_test.go (1)
111-150: 🛠️ Refactor suggestionConsolidate duplicate clean command functions.
The functions
runTerraformCleanandrunCLITerraformCleanComponentcan be replaced withrunTerraformCleanCommand.Remove these functions and update their callers to use
runTerraformCleanCommand:- runTerraformClean(t, binaryPath) + runTerraformCleanCommand(t, binaryPath, "--force") - runCLITerraformCleanComponent(t, binaryPath, environment) + runTerraformCleanCommand(t, binaryPath, "station", "-s", environment, "--force")Likely invalid or redundant comment.
haitham911
changed the title
terraform clean bugs
aknysh
added
minor
|
These changes were released in v1.157.0. |
Fixes and Improvements for terraform clean:
The --everything flag is redundant as it represents the default behavior. Remove it to simplify the command.
Add integration tests to verify the functionality of terraform apply and terraform clean.
Ensure the tests cover scenarios for applying and cleaning up resources.
Bug Fix: atmos terraform clean --everything:
Fix the issue where the output incorrectly states: component '' is not enabled and skipped.
Bug Fix: atmos terraform clean -s
<stack>Add tests for the following atmos terraform clean commands:
atmos terraform clean
atmos terraform clean
<component>atmos terraform clean
<component>-s<stack>Ensure the --force flag is supported in all cases to bypass confirmation prompts.
references
Summary by CodeRabbit
Release Notes: Atmos Terraform Clean Command Update
New Features
atmos terraform cleancommand behavior--everythingflag--forceflag to bypass confirmation--skip-initflag to skip automatic execution ofterraform init--from-planand--planfileflags for planfile usageDocumentation Updates
atmos terraformcommandsImprovements
Tests
atmos terraform cleancommand functionality