Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix terraform clean bugs #870

Open
wants to merge 13 commits into
base: main
Choose a base branch
from
7 changes: 2 additions & 5 deletions internal/exec/help.go
Original file line number Diff line number Diff line change
Expand Up @@ -32,9 +32,8 @@ func processHelp(
"the commands will use the planfile previously generated by 'atmos terraform plan' command instead of generating a new planfile")
u.PrintMessage(" - 'atmos terraform apply' and 'atmos terraform deploy' commands commands support '--planfile' flag to specify the path " +
"to a planfile. The '--planfile' flag should be used instead of the planfile argument in the native 'terraform apply <planfile>' command")
u.PrintMessage(" - 'atmos terraform clean' command deletes the '.terraform' folder, '.terraform.lock.hcl' lock file, " +
u.PrintMessage(" - 'atmos terraform clean' command delete the Terraform state files and directories for the component and stack., '.terraform.lock.hcl' lock file, " +
"and the previously generated 'planfile', 'varfile', and 'backend.tf.json' file for the specified component and stack. " +
"Use the --everything flag to also delete the Terraform state files and directories for the component. " +
"Note: State files store the local state of your infrastructure and should be handled with care, if not using a remote backend.\n\n" +
"Additional flags:\n" +
" --force Forcefully delete Terraform state files and directories without interaction\n" +
Expand Down Expand Up @@ -70,16 +69,14 @@ func processHelp(
"native arguments and flags for the 'helmfile' commands")
}
} else if componentType == "terraform" && command == "clean" {
u.PrintMessage("\n'atmos terraform clean' command deletes the following folders and files from the component's directory:\n\n" +
u.PrintMessage("\n'atmos terraform clean' command delete all the Terraform state files and directories for all components and stacks . deletes the following folders and files from the component's directory:\n\n" +
" - '.terraform' folder\n" +
" - folder that the 'TF_DATA_DIR' ENV var points to\n" +
" - '.terraform.lock.hcl' file\n" +
" - generated varfile for the component in the stack\n" +
" - generated planfile for the component in the stack\n" +
" - generated 'backend.tf.json' file\n" +
" - 'terraform.tfstate.d' folder (if '--everything' flag is used)\n\n" +
"Usage: atmos terraform clean <component> -s <stack> <flags>\n\n" +
"Use '--everything' flag to also 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" +
Expand Down
8 changes: 2 additions & 6 deletions internal/exec/terraform.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@ const (
outFlag = "-out"
varFileFlag = "-var-file"
skipTerraformLockFileFlag = "--skip-lock-file"
everythingFlag = "--everything"
forceFlag = "--force"
)

Expand Down Expand Up @@ -75,13 +74,10 @@ func ExecuteTerraform(info schema.ConfigAndStacksInfo) error {
shouldCheckStack := true
// Skip stack processing when cleaning with --everything or --force flags to allow
// cleaning without requiring stack configuration
if info.SubCommand == "clean" &&
(u.SliceContainsString(info.AdditionalArgsAndFlags, everythingFlag) ||
u.SliceContainsString(info.AdditionalArgsAndFlags, forceFlag)) {
if info.SubCommand == "clean" {
if info.ComponentFromArg == "" {
shouldProcessStacks = false
}

shouldCheckStack = info.Stack != ""

}
Expand All @@ -96,7 +92,7 @@ func ExecuteTerraform(info schema.ConfigAndStacksInfo) error {
}
}

if !info.ComponentIsEnabled {
if !info.ComponentIsEnabled && info.SubCommand != "clean" {
u.LogInfo(atmosConfig, fmt.Sprintf("component '%s' is not enabled and skipped", info.ComponentFromArg))
return nil
}
Expand Down
9 changes: 4 additions & 5 deletions internal/exec/terraform_clean.go
Original file line number Diff line number Diff line change
Expand Up @@ -346,8 +346,8 @@ func handleTFDataDir(componentPath string, relativePath string, atmosConfig sche
}

}
func initializeFilesToClear(info schema.ConfigAndStacksInfo, atmosConfig schema.AtmosConfiguration, everything bool) []string {
if everything && info.Stack == "" {
func initializeFilesToClear(info schema.ConfigAndStacksInfo, atmosConfig schema.AtmosConfiguration) []string {
if info.ComponentFromArg == "" {
return []string{".terraform", ".terraform.lock.hcl", "*.tfvar.json", "terraform.tfstate.d"}
}
varFile := constructTerraformComponentVarfileName(info)
Expand Down Expand Up @@ -406,8 +406,7 @@ func handleCleanSubCommand(info schema.ConfigAndStacksInfo, componentPath string
}

force := u.SliceContainsString(info.AdditionalArgsAndFlags, forceFlag)
everything := u.SliceContainsString(info.AdditionalArgsAndFlags, everythingFlag)
filesToClear := initializeFilesToClear(info, atmosConfig, everything)
filesToClear := initializeFilesToClear(info, atmosConfig)
folders, err := CollectDirectoryObjects(cleanPath, filesToClear)
if err != nil {
u.LogTrace(atmosConfig, fmt.Errorf("error collecting folders and files: %v", err).Error())
Expand Down Expand Up @@ -455,7 +454,7 @@ func handleCleanSubCommand(info schema.ConfigAndStacksInfo, componentPath string
u.PrintMessage(fmt.Sprintf("Do you want to delete the folder '%s'? ", tfDataDir))
}
var message string
if everything && info.ComponentFromArg == "" {
if info.ComponentFromArg == "" {
message = fmt.Sprintf("This will delete %v local terraform state files affecting all components", total)
} else if info.Component != "" && info.Stack != "" {
message = fmt.Sprintf("This will delete %v local terraform state files for component '%s' in stack '%s'", total, info.Component, info.Stack)
Expand Down
7 changes: 5 additions & 2 deletions internal/exec/utils.go
Original file line number Diff line number Diff line change
Expand Up @@ -665,9 +665,12 @@ func processArgsAndFlags(
var additionalArgsAndFlags []string
var globalOptions []string
var indexesToRemove []int
if len(inputArgsAndFlags) == 1 && inputArgsAndFlags[0] == "clean" {
info.SubCommand = inputArgsAndFlags[0]
}

// For commands like `atmos terraform clean` and `atmos terraform plan`, show the command help
if len(inputArgsAndFlags) == 1 && inputArgsAndFlags[0] != "version" {
// For commands like `atmos terraform plan`, show the command help
if len(inputArgsAndFlags) == 1 && inputArgsAndFlags[0] != "version" && info.SubCommand == "" {
info.SubCommand = inputArgsAndFlags[0]
info.NeedHelp = true
return info, nil
Expand Down
100 changes: 100 additions & 0 deletions tests/cli_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -248,3 +248,103 @@ func verifyFileContains(t *testing.T, filePatterns map[string][]string) bool {
}
return success
}
func TestCLITerraformClean(t *testing.T) {
// Capture the starting working directory
startingDir, err := os.Getwd()
if err != nil {
t.Fatalf("Failed to get the current working directory: %v", err)
}

// Initialize PathManager and update PATH
pathManager := NewPathManager()
pathManager.Prepend("../build", "..")
err = pathManager.Apply()
if err != nil {
t.Fatalf("Failed to apply updated PATH: %v", err)
}
fmt.Printf("Updated PATH: %s\n", pathManager.GetPath())
defer func() {
// 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)
}
}()
workDir := "../examples/quick-start-simple"
err = os.Chdir(workDir)
if err != nil {
t.Fatalf("Failed to change directory to %q: %v", workDir, err)
}
binaryPath, err := exec.LookPath("atmos")
if err != nil {
t.Fatalf("Binary not found: %s. Current PATH: %s", "atmos", pathManager.GetPath())
}
cmd := exec.Command(binaryPath, "terraform", "apply", "station", "-s", "dev")
var stdout, stderr bytes.Buffer
cmd.Stdout = &stdout
cmd.Stderr = &stderr
// ATMOS_COMPONENTS_TERRAFORM_APPLY_AUTO_APPROVE
envVars := os.Environ()
envVars = append(envVars, "ATMOS_COMPONENTS_TERRAFORM_APPLY_AUTO_APPROVE=true")
cmd.Env = envVars

// run terraform apply station -s dev and terraform apply station -s prod
err = cmd.Run()
if err != nil {
t.Log(stdout.String())
t.Fatalf("Failed to run terraform apply station -s dev: %v", stderr.String())
return
}
Comment on lines +281 to +296
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Improve command execution and error handling.

The command execution pattern could be improved in several ways:

  1. Reuse stdout/stderr buffers across commands
  2. Better error handling with more context
  3. Extract common command execution logic
+func executeAtmosCommand(t *testing.T, binaryPath string, args []string, env []string) (string, string, error) {
+    cmd := exec.Command(binaryPath, args...)
+    var stdout, stderr bytes.Buffer
+    cmd.Stdout = &stdout
+    cmd.Stderr = &stderr
+    cmd.Env = env
+    err := cmd.Run()
+    return stdout.String(), stderr.String(), err
+}

Committable suggestion skipped: line range outside the PR's diff.

cmd = exec.Command(binaryPath, "terraform", "apply", "station", "-s", "prod")
envVars = append(envVars, "ATMOS_COMPONENTS_TERRAFORM_APPLY_AUTO_APPROVE=true")
cmd.Env = envVars

err = cmd.Run()
if err != nil {
t.Log(stdout.String())
t.Fatalf("Failed to run terraform apply station -s prod: %v", stderr.String())
return
}
// get command error sta
// check if the state files and directories for the component and stack are exist
stateFiles := []string{
"./components/terraform/weather/.terraform",
"./components/terraform/weather/terraform.tfstate.d",
"./components/terraform/weather/.terraform.lock.hcl",
}
for _, file := range stateFiles {
fileAbs, err := filepath.Abs(file)
if err != nil {
t.Fatalf("Failed to resolve absolute path for %q: %v", file, err)
}
if _, err := os.Stat(fileAbs); errors.Is(err, os.ErrNotExist) {
t.Errorf("Reason: Expected file exist: %q", fileAbs)
return
}
}
Comment on lines +307 to +323
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Improve state file verification logic.

Several issues to address:

  1. Early returns could prevent proper test cleanup
  2. Hardcoded paths could be more flexible
  3. Incomplete comment on line 307
-    // get command error sta
+    // check terraform state files existence
     stateFiles := []string{
-        "./components/terraform/weather/.terraform",
-        "./components/terraform/weather/terraform.tfstate.d",
-        "./components/terraform/weather/.terraform.lock.hcl",
+        filepath.Join("components", "terraform", "weather", ".terraform"),
+        filepath.Join("components", "terraform", "weather", "terraform.tfstate.d"),
+        filepath.Join("components", "terraform", "weather", ".terraform.lock.hcl"),
     }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
// get command error sta
// check if the state files and directories for the component and stack are exist
stateFiles := []string{
"./components/terraform/weather/.terraform",
"./components/terraform/weather/terraform.tfstate.d",
"./components/terraform/weather/.terraform.lock.hcl",
}
for _, file := range stateFiles {
fileAbs, err := filepath.Abs(file)
if err != nil {
t.Fatalf("Failed to resolve absolute path for %q: %v", file, err)
}
if _, err := os.Stat(fileAbs); errors.Is(err, os.ErrNotExist) {
t.Errorf("Reason: Expected file exist: %q", fileAbs)
return
}
}
// check terraform state files existence
stateFiles := []string{
filepath.Join("components", "terraform", "weather", ".terraform"),
filepath.Join("components", "terraform", "weather", "terraform.tfstate.d"),
filepath.Join("components", "terraform", "weather", ".terraform.lock.hcl"),
}
for _, file := range stateFiles {
fileAbs, err := filepath.Abs(file)
if err != nil {
t.Fatalf("Failed to resolve absolute path for %q: %v", file, err)
}
if _, err := os.Stat(fileAbs); errors.Is(err, os.ErrNotExist) {
t.Errorf("Reason: Expected file exist: %q", fileAbs)
return
}
}


// run atmos terraform clean
cmd = exec.Command(binaryPath, "terraform", "clean", "--force")
err = cmd.Run()
if err != nil {
t.Log(stdout.String())
t.Fatalf("Failed to run atmos terraform clean: %v", stderr.String())
}
// check if the state files and directories for the component and stack are deleted
for _, file := range stateFiles {
fileAbs, err := filepath.Abs(file)
if err != nil {
t.Fatalf("Failed to resolve absolute path for %q: %v", file, err)
}
_, err = os.Stat(fileAbs)
if err != nil {
if errors.Is(err, os.ErrNotExist) {
continue
}
t.Errorf("Reason: error %q", fileAbs)
return

}
return
}
Comment on lines +324 to +348
Copy link
Contributor

Choose a reason for hiding this comment

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

💡 Codebase verification

⚠️ Potential issue

Improve clean command test verification logic

The review comment is correct. Looking at the implementation in internal/exec/terraform_clean.go and the test in tests/cli_test.go, the test needs improvement. Here's why:

  • The unconditional return statements will exit the test prematurely, potentially missing other undeleted files
  • The error message doesn't clearly indicate which files weren't cleaned up
  • The test should verify the clean operation's behavior as implemented in handleCleanSubCommand

Suggested improvements:

  • Replace unconditional returns with a failed flag as proposed
  • Enhance error message to clearly identify undeleted files
  • Consider adding verification of .terraform/environment file deletion since cleanTerraformWorkspace is part of the clean operation
🔗 Analysis chain

Fix clean command verification logic.

The verification has several issues:

  1. Unconditional return at line 347 could cause false positives
  2. Error message could be more descriptive
  3. Missing verification of command output
-        return
+        t.Errorf("Expected file %q to be deleted, but it still exists", fileAbs)
+        failed = true

     }
-    return
+    if failed {
+        t.Fail()
+    }

Also, consider adding verification of the command output to ensure it matches the expected behavior from the PR objectives.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the output of terraform clean command matches expected behavior

# Test: Check if the command output contains expected success message
cmd_output=$(atmos terraform clean --force 2>&1)
echo "$cmd_output" | grep -q "deleted" || echo "Command output doesn't indicate successful deletion"

Length of output: 382


Script:

#!/bin/bash
# Let's check the implementation of the clean command to understand expected output
ast-grep --pattern 'func $_($$$ *cli.Context) error {
  $$$
  clean($$$)
  $$$
}'

Length of output: 83


Script:

#!/bin/bash
# Let's check the test file more thoroughly
rg -A 5 "terraform clean" tests/cli_test.go

Length of output: 448


Script:

#!/bin/bash
# Check if there are any other tests for clean command
rg "TestClean" tests/

Length of output: 21


Script:

#!/bin/bash
# Let's check the clean command implementation with a different pattern
ast-grep --pattern 'func clean($$$) {
  $$$
}'

Length of output: 48


Script:

#!/bin/bash
# Let's look for any clean-related functions
rg -B 2 -A 5 "func.*[Cc]lean" .

Length of output: 1790


Script:

#!/bin/bash
# Check the test setup for stateFiles
rg -B 5 "stateFiles :=" tests/cli_test.go

Length of output: 270


}
7 changes: 3 additions & 4 deletions website/docs/cli/commands/terraform/terraform-clean.mdx
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ Execute the `terraform clean` command like this:
atmos terraform clean <component> -s <stack> [--skip-lock-file] [--everything] [--force]

:::warning
The `--everything` flag will delete all Terraform-related files including state files. The `--force` flag will bypass confirmation prompts.
The `clean` default behavior and will delete all Terraform-related files including state files, with a confirmation prompt before proceeding. The `--force` flag will bypass confirmation prompts.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
The `clean` default behavior and will delete all Terraform-related files including state files, with a confirmation prompt before proceeding. The `--force` flag will bypass confirmation prompts.
The `clean` command, by default, deletes all Terraform-related files, including local state files, but will prompt for confirmation before proceeding. Using the `--force` flag skips the confirmation prompt and executes the deletion immediately.

Use these flags with extreme caution as they can lead to irreversible data loss.
:::
```
Expand All @@ -36,10 +36,9 @@ Run `atmos terraform clean --help` to see all the available options

```shell
# Delete all Terraform-related files for all components (with confirmation)
atmos terraform clean --everything

atmos terraform clean
# Force delete all Terraform-related files for all components (no confirmation)
atmos terraform clean --everything --force
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
Expand Down
14 changes: 7 additions & 7 deletions website/docs/cli/commands/terraform/usage.mdx
Original file line number Diff line number Diff line change
Expand Up @@ -59,11 +59,11 @@ HCL-based domain-specific language and its interpreter. Atmos works with [OpenTo

- `atmos terraform clean` command deletes the `.terraform` folder, `.terraform.lock.hcl` lock file, and the previously generated `planfile`
and `varfile` for the specified component and stack. Use the `--skip-lock-file` flag to skip deleting the `.terraform.lock.hcl` file.
Use the `--everything` flag 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).
It deletes all local Terraform state files and directories (including `terraform.tfstate.d/`) for all components and stacks.
The `--force` flag bypasses the safety confirmation prompt and forces 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 `clean` performs destructive operations that can lead to permanent state loss. Always ensure you have remote state configured in your components before proceeding.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
The `clean` performs destructive operations that can lead to permanent state loss. Always ensure you have remote state configured in your components before proceeding.
The `clean` command performs destructive operations that can lead to permanent state loss, if not using remote backends. Always ensure you have remote state configured in your components before proceeding.

:::

- `atmos terraform workspace` command first runs `terraform init -reconfigure`, then `terraform workspace select`, and if the workspace was not
Expand Down Expand Up @@ -113,16 +113,16 @@ atmos terraform destroy test/test-component-override -s tenant1-ue2-dev --redire
atmos terraform init test/test-component-override-3 -s tenant1-ue2-dev

# Clean all components (with confirmation)
atmos terraform clean --everything
atmos terraform clean

# Clean a specific component
atmos terraform clean vpc --everything
atmos terraform clean vpc

# Clean a specific component in a stack
atmos terraform clean vpc --stack dev --everything
atmos terraform clean vpc --stack dev

# Clean without confirmation prompt
atmos terraform clean --everything --force
atmos terraform clean --force
atmos terraform clean test/test-component-override-3 -s tenant1-ue2-dev

atmos terraform workspace test/test-component-override-3 -s tenant1-ue2-dev
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,8 @@
- 'atmos terraform deploy' command supports '--deploy-run-init=true/false' flag to enable/disable running 'terraform init' before executing the command
- 'atmos terraform apply' and 'atmos terraform deploy' commands support '--from-plan' flag. If the flag is specified, the commands will use the planfile previously generated by 'atmos terraform plan' command instead of generating a new planfile
- 'atmos terraform apply' and 'atmos terraform deploy' commands commands support '--planfile' flag to specify the path to a planfile. The '--planfile' flag should be used instead of the planfile argument in the native 'terraform apply &lt;planfile&gt;' command
- 'atmos terraform clean' command deletes the '.terraform' folder, '.terraform.lock.hcl' lock file, and the previously generated 'planfile', 'varfile' and 'backend.tf.json' file for the specified component and stack. Use --skip-lock-file flag to skip deleting the lock file.
- 'atmos terraform clean' command supports '--everything' flag to delete all the Terraform state files and directories for all components and stacks . Use --force flag to skip the confirmation prompt.
- 'atmos terraform clean' command delete all the Terraform state files and directories for all components and stacks . deletes the '.terraform' folder, '.terraform.lock.hcl' lock file, and the previously generated 'planfile', 'varfile' and 'backend.tf.json' file for the specified component and stack. Use --skip-lock-file flag to skip deleting the lock file.
- 'atmos terraform clean' . Use --force flag to skip the confirmation prompt.
- 'atmos terraform workspace' command first runs 'terraform init -reconfigure', then 'terraform workspace select', and if the workspace was not created before, it then runs 'terraform workspace new'
- 'atmos terraform import' command searches for 'region' in the variables for the specified component and stack, and if it finds it, sets 'AWS_REGION=&lt;region&gt;' ENV var before executing the command
- 'atmos terraform generate backend' command generates a backend config file for an 'atmos' component in a stack
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@


'atmos terraform clean' command deletes the following folders and files from the component's directory:

- delete all the Terraform state files and directories for all components and stacks
- '.terraform' folder
- folder that the 'TF_DATA_DIR' ENV var points to
- '.terraform.lock.hcl' file
Expand All @@ -17,7 +17,7 @@
- generated 'backend.tf.json' file

Usage: atmos terraform clean &lt;component&gt; -s &lt;stack&gt; &lt;flags&gt;
Use '--everything' flag to delete all the files and folders mentioned above. '--force' to delete the files without confirmation.
Use '--force' to delete the files without confirmation.
Use '--skip-lock-file' flag to skip deleting the lock file.

For more details refer to https://atmos.tools/cli/commands/terraform/clean
Expand Down
Loading