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
Open

fix terraform clean bugs #870

wants to merge 13 commits into from

Conversation

haitham911
Copy link
Collaborator

@haitham911 haitham911 commented Dec 19, 2024

what

Fix bugs related to:

  • remove --everything. --everything is the default behavior
  • add integration test for terraform apply and terraform clean
  • atmos terraform clean --everything. Outputs: component '' is not enabled and skipped
  • atmos terraform clean -s also just outputs the help.
  • atmos terraform clean reports that a stack is required
  • atmos terraform clean -s does work

references

Summary by CodeRabbit

Summary by CodeRabbit

  • New Features

    • Enhanced documentation for the atmos terraform clean command, clarifying its functionality and the removal of the --everything flag.
    • Introduced new flags for atmos terraform commands, including --skip-init, --deploy-run-init, and --from-plan.
    • Updated atmos terraform import command to set the AWS_REGION environment variable based on component settings.
  • Bug Fixes

    • Improved handling of the "clean" subcommand for stack processing.
    • Adjusted logging behavior for components not enabled during the "clean" command.
  • Documentation

    • Expanded help messages for Terraform commands to improve usability and clarity, including detailed descriptions of deletions performed by the atmos terraform clean command.
    • Updated the help documentation to reflect changes in command usage and clarify the impact of new flags.
  • Tests

    • Added a new test for the terraform clean command to ensure proper functionality and state file deletion.

@haitham911 haitham911 requested a review from a team as a code owner December 19, 2024 02:58
@haitham911 haitham911 requested a review from osterman December 19, 2024 02:58
@mergify mergify bot added the triage Needs triage label Dec 19, 2024
Copy link
Contributor

coderabbitai bot commented Dec 19, 2024

📝 Walkthrough
📝 Walkthrough

Walkthrough

The pull request modifies the ExecuteTerraform function in internal/exec/terraform.go to improve the handling of the Terraform "clean" subcommand. The changes adjust the conditional logic for processing stacks, removing previous flag-based checks and simplifying the condition for stack processing. The logging mechanism for skipped components has also been refined to align with the new command behavior.

Changes

File Change Summary
internal/exec/terraform.go Modified conditional logic for "clean" subcommand processing and adjusted logging for skipped components
internal/exec/help.go Updated help messages for clarity regarding Terraform commands and flags
internal/exec/utils.go Enhanced argument processing and error handling; added flexibility for command flags
internal/exec/terraform_clean.go Simplified function signature and logic for file clearing; removed unnecessary parameters
website/docs/cli/commands/terraform/terraform-clean.mdx Clarified documentation for the atmos terraform clean command regarding the --everything flag
website/docs/cli/commands/terraform/usage.mdx Updated usage documentation to include new flags and command behaviors
website/src/components/Screengrabs/atmos-terraform--help.html Enhanced documentation for atmos terraform clean command functionality and usage
website/src/components/Screengrabs/atmos-terraform-clean--help.html Modified help documentation for atmos terraform clean command to clarify deletion behavior
tests/cli_test.go Added a new test function TestCLITerraformClean to validate the clean command behavior

Assessment against linked issues

Objective Addressed Explanation
Clean command should work without explicit component/stack specification [#864, DEV-2859]
Support global clean operation for all components/stacks [#864, DEV-2859]

Possibly related PRs

Suggested reviewers

  • osterman
  • aknysh

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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR. (Beta)
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary or @auto-summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai or @auto-title anywhere in the PR title to generate the title automatically.

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 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 clean command. 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:

  1. atmos terraform clean --everything
  2. atmos terraform clean -s <stack>
  3. atmos terraform clean <component>
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between fe637fa and 6832ddd.

📒 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:

  1. clean --everything: No stack processing when component is empty
  2. clean -s <stack>: Stack check only when stack is provided
  3. clean <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 handles clean --everything
  • Stack validation (shouldCheckStack) only occurs when a stack is explicitly provided, fixing the clean -s <stack> case
  • The implementation in handleCleanSubCommand confirms 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

coderabbitai[bot]
coderabbitai bot previously approved these changes Dec 19, 2024
@mergify mergify bot removed the triage Needs triage label Dec 19, 2024
@aknysh aknysh added the patch A minor, backward compatible change label Dec 19, 2024
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 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 --everything flag'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:

  1. Removed duplicate "and" in "state files and and directories"
  2. Fixed em dash usage
  3. Removed redundant information about global operation
  4. Improved formatting for better readability
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 6832ddd and 0c90df5.

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

website/docs/cli/commands/terraform/terraform-clean.mdx Outdated Show resolved Hide resolved
internal/exec/utils.go Outdated Show resolved Hide resolved
coderabbitai[bot]
coderabbitai bot previously approved these changes Dec 19, 2024
coderabbitai[bot]
coderabbitai bot previously approved these changes Dec 19, 2024
@osterman
Copy link
Member

@coderabbitai can you add integration tests?

Copy link
Contributor

coderabbitai bot commented Dec 19, 2024

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 (
"fmt"
"os"
"os/exec"
"strings"
"testing"
)

func TestTerraformCleanEverything(t *testing.T) {
cmd := exec.Command("atmos", "terraform", "clean", "--everything")
output, err := cmd.CombinedOutput()
if err != nil {
t.Fatalf("Error running 'atmos terraform clean --everything': %v\nOutput: %s", err, string(output))
}
if !strings.Contains(string(output), "deleted") {
t.Errorf("Expected output to indicate successful deletion, got: %s", string(output))
}
}

func TestTerraformCleanSpecificStack(t *testing.T) {
// Replace “” with an actual valid stack name in your environment
stackName := ""
cmd := exec.Command("atmos", "terraform", "clean", "-s", stackName)
output, err := cmd.CombinedOutput()
if err != nil {
t.Fatalf("Error running 'atmos terraform clean -s %s': %v\nOutput: %s", stackName, err, string(output))
}
if !strings.Contains(string(output), "deleted") {
t.Errorf("Expected output to contain the word 'deleted', got: %s", string(output))
}
}

func TestTerraformCleanComponentNoStack(t *testing.T) {
// Replace “” with an actual valid component name in your environment
compName := ""
cmd := exec.Command("atmos", "terraform", "clean", compName)
output, err := cmd.CombinedOutput()
if err != nil {
t.Fatalf("Error running 'atmos terraform clean %s': %v\nOutput: %s", compName, err, string(output))
}
if !strings.Contains(string(output), "deleted") {
t.Errorf("Expected output to indicate a successful cleanup, got: %s", string(output))
}
}

func TestTerraformCleanComponentAndStack(t *testing.T) {
// Replace these placeholders with valid component and stack names in your environment
compName := ""
stackName := ""
cmd := exec.Command("atmos", "terraform", "clean", compName, "-s", stackName)
output, err := cmd.CombinedOutput()
if err != nil {
t.Fatalf("Error running 'atmos terraform clean %s -s %s': %v\nOutput: %s", compName, stackName, err, string(output))
}
if !strings.Contains(string(output), "deleted") {
t.Errorf("Expected output to indicate a successful cleanup, got: %s", string(output))
}
}


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 executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Let's search for existing Go test files in the repository to see how integration tests are structured.
fd _test.go

# Also, let's see if there's a directory named 'test' or 'integration' which might indicate integration tests.
fd test
fd integration

Length of output: 3303


Script:

#!/bin/bash
# Let's search for clues on how integration tests might be structured or referenced in the codebase
# by looking for mentions of "integration test" or relevant references in existing test files or docs.

rg -A 5 -B 5 "integration test"
rg -A 5 -B 5 "IntegrationTest"
rg -A 5 -B 5 "integration testing"
rg -A 5 -B 5 "end-to-end test"

Length of output: 2658

internal/exec/help.go Outdated Show resolved Hide resolved
Copy link
Member

@aknysh aknysh left a comment

Choose a reason for hiding this comment

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

@haitham911 thanks, please see comments

@haitham911 haitham911 requested a review from aknysh December 28, 2024 13:54
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (2)
website/src/components/Screengrabs/atmos-terraform-clean--help.html (1)

19-21: Add confirmation prompt information for safety.

Based on previous learnings, we should document that a confirmation prompt appears when using the command without the --force flag.

Add this line after the force flag description:

 Use '--force' to delete the files without confirmation.
+Note: Without '--force', you will be prompted for confirmation before deletion.
 Use '--skip-lock-file' flag to skip deleting the lock file.
internal/exec/terraform_clean.go (1)

457-457: Refine user message for clarity.

The new message generation logic is straightforward. However, consider explaining the scope of removal more explicitly ("affecting all components"), reinforcing the gravity of deleting all local Terraform state.

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 8d1292b and 1acd1d0.

📒 Files selected for processing (8)
  • internal/exec/help.go (2 hunks)
  • internal/exec/terraform.go (2 hunks)
  • internal/exec/terraform_clean.go (3 hunks)
  • internal/exec/utils.go (1 hunks)
  • website/docs/cli/commands/terraform/terraform-clean.mdx (2 hunks)
  • website/docs/cli/commands/terraform/usage.mdx (2 hunks)
  • website/src/components/Screengrabs/atmos-terraform--help.html (1 hunks)
  • website/src/components/Screengrabs/atmos-terraform-clean--help.html (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
  • website/docs/cli/commands/terraform/terraform-clean.mdx
  • internal/exec/terraform.go
  • internal/exec/help.go
  • website/docs/cli/commands/terraform/usage.mdx
🧰 Additional context used
📓 Learnings (1)
website/src/components/Screengrabs/atmos-terraform-clean--help.html (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 (7)
website/src/components/Screengrabs/atmos-terraform-clean--help.html (2)

Line range hint 11-17: LGTM! Clear and comprehensive list of affected files.

The documentation now provides a thorough breakdown of what the clean command affects. This clarity helps users understand the command's impact before execution.


Line range hint 19-24: Documentation aligns with PR objectives.

The updated help text correctly reflects the command's behavior:

  1. Removed confusing --everything flag
  2. Clearly shows the correct command structure with component and stack
  3. Added new --skip-lock-file option for flexibility

This addresses the reported issues in PR #870 where various command combinations were not working as expected.

internal/exec/utils.go (2)

668-670: Looks good for detecting "clean" subcommand.

This ensures the command "clean" is parsed correctly when provided alone. No immediate improvements are needed here.


672-673: Prevent accidental help trigger for single-arg commands.

Deferring help display to the newly handled "clean" case is consistent. This maintains clarity for one-argument commands while preserving "version" checks.

internal/exec/terraform_clean.go (2)

349-350: Simplified logic is well-received.

By removing the everything parameter and relying on info.ComponentFromArg, you reduce complexity and unify the conditions under which files are cleared. This approach seems consistent with the intended functionality. Ensure all relevant test cases reflect this simpler path.


409-409: Confirm multi-component usage.

initializeFilesToClear now depends solely on ComponentFromArg. If your workflow involves cleaning multiple components in a single invocation, verify that the function and associated logic capture every required scenario.

website/src/components/Screengrabs/atmos-terraform--help.html (1)

23-24: Documentation aligns well with code changes.

The updated description clarifies how the command deletes Terraform state files across components. This improvement mitigates confusion about the removal of .terraform and the lock file.

coderabbitai[bot]
coderabbitai bot previously approved these changes Jan 4, 2025
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Nitpick comments (1)
tests/cli_test.go (1)

304-305: Incomplete comment.

The comment // get command error sta is truncated. Consider removing or elaborating it to avoid confusion.

-	// get command error sta
+	// TODO: Handle command error status or remove this comment if unneeded.
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 1acd1d0 and 338d859.

📒 Files selected for processing (1)
  • tests/cli_test.go (1 hunks)

tests/cli_test.go Outdated Show resolved Hide resolved
coderabbitai[bot]
coderabbitai bot previously approved these changes Jan 5, 2025
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 3

♻️ Duplicate comments (1)
tests/cli_test.go (1)

297-306: ⚠️ Potential issue

Mismatch between command arguments and error message.

The error message doesn't match the command being executed.

Fix duplicate environment variable.

The ATMOS_COMPONENTS_TERRAFORM_APPLY_AUTO_APPROVE environment variable is being appended twice.

-    envVars = append(envVars, "ATMOS_COMPONENTS_TERRAFORM_APPLY_AUTO_APPROVE=true")
🧹 Nitpick comments (1)
tests/cli_test.go (1)

251-280: Consider extracting common test setup logic.

The setup code is duplicated from TestCLICommands. Consider extracting this into a helper function to improve maintainability.

+func setupTestEnvironment(t *testing.T) (*PathManager, string, string) {
+    startingDir, err := os.Getwd()
+    if err != nil {
+        t.Fatalf("Failed to get the current working directory: %v", err)
+    }
+
+    pathManager := NewPathManager()
+    pathManager.Prepend("../build", "..")
+    if err := pathManager.Apply(); err != nil {
+        t.Fatalf("Failed to apply updated PATH: %v", err)
+    }
+
+    binaryPath, err := exec.LookPath("atmos")
+    if err != nil {
+        t.Fatalf("Binary not found: %s. Current PATH: %s", "atmos", pathManager.GetPath())
+    }
+
+    return pathManager, startingDir, binaryPath
+}
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 338d859 and 6735525.

📒 Files selected for processing (1)
  • tests/cli_test.go (1 hunks)

Comment on lines +281 to +296
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
}
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.

Comment on lines +307 to +323
// 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
}
}
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
}
}

Comment on lines +324 to +348

// 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
}
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

Copy link

mergify bot commented Jan 8, 2025

💥 This pull request now has conflicts. Could you fix it @haitham911? 🙏

@mergify mergify bot added the conflict This PR has conflicts label Jan 8, 2025
@@ -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.


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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
conflict This PR has conflicts patch A minor, backward compatible change
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

atmos terraform clean does not work properly
3 participants