fix: Handle TF_DATA_DIR and Error Logging for !terraform.output #1037
Conversation
mergify
bot
added
triage
milldr
changed the title
Co-authored-by: Erik Osterman (CEO @ Cloud Posse) <[email protected]>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
📝 WalkthroughWalkthroughThis pull request updates the logging and error handling mechanisms in the internal Terraform helper functions. The Changes
Suggested labels
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
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
internal/exec/terraform_utils.go (1)
45-69: Consider handling file permission errors explicitly.While the error handling is good, it would be helpful to distinguish between permission errors and other types of errors when checking and deleting files.
Here's a suggested improvement:
if _, err := os.Stat(filePath); err == nil { l.Debug("Terraform environment file found. Proceeding with deletion.", "file", filePath, ) if err := os.Remove(filePath); err != nil { + if os.IsPermission(err) { + l.Debug("Permission denied when deleting Terraform environment file.", + "file", filePath, + "error", err, + ) + } else { l.Debug("Failed to delete Terraform environment file.", "file", filePath, "error", err, ) + } } else { l.Debug("Successfully deleted Terraform environment file.", "file", filePath, ) } } else if os.IsNotExist(err) { l.Debug("Terraform environment file not found. No action needed.", "file", filePath, ) + } else if os.IsPermission(err) { + l.Debug("Permission denied when checking Terraform environment file.", + "file", filePath, + "error", err, + ) } else { l.Debug("Error checking Terraform environment file.", "file", filePath, "error", err, ) }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/exec/terraform_utils.go(4 hunks)
🧰 Additional context used
📓 Learnings (1)
internal/exec/terraform_utils.go (1)
Learnt from: aknysh
PR: cloudposse/atmos#759
File: internal/exec/terraform.go:366-368
Timestamp: 2024-11-12T05:52:05.088Z
Learning: In `internal/exec/terraform.go`, the workspace cleaning code under both the general execution path and within the `case "init":` block is intentionally duplicated because the code execution paths are different. The `.terraform/environment` file should be deleted before executing `terraform init` in both scenarios to ensure a clean state.
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Summary
🔇 Additional comments (3)
internal/exec/terraform_utils.go (3)
8-8: LGTM! Good addition of semantic logging.The import of
charmbracelet/logaligns with the team's direction to use semantic logging for better observability.
28-44: LGTM! Robust handling of TF_DATA_DIR and paths.The changes properly handle the TF_DATA_DIR environment variable and ensure correct path resolution, which aligns with the PR objectives.
92-94: LGTM! Consistent semantic logging.The logging changes consistently use semantic logging with appropriate context across functions, which improves debugging capabilities.
Also applies to: 117-119
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
internal/exec/terraform_outputs.go (2)
83-83: Consider adding more context to debug logs.While the logging statements are clear, they could benefit from additional context fields.
Consider enhancing the debug logs with more structured fields:
-l.Debug("Writing the backend config to file:", "file", backendFileName) +l.Debug("Writing the backend config to file:", "file", backendFileName, "component", component, "stack", stack) -l.Debug("Wrote the backend config to file:", "file", backendFileName) +l.Debug("Wrote the backend config to file:", "file", backendFileName, "component", component, "stack", stack) -l.Debug("Writing the provider overrides to file:", "file", providerOverrideFileName) +l.Debug("Writing the provider overrides to file:", "file", providerOverrideFileName, "component", component, "stack", stack) -l.Debug("Wrote the provider overrides to file:", "file", providerOverrideFileName) +l.Debug("Wrote the provider overrides to file:", "file", providerOverrideFileName, "component", component, "stack", stack)Also applies to: 105-105, 114-114, 122-122
259-262: Consider using a helper function for error handling.The error handling pattern is repeated in multiple places. Consider extracting it into a helper function.
Create a helper function:
func handleTerraformError(p *tea.Program, spinnerDone chan struct{}, message string, err error, component string, stack string) { p.Quit() <-spinnerDone fmt.Printf("\r✗ %s\n", message) l.Error("Terraform operation failed", "component", component, "stack", stack, "error", err) os.Exit(1) }Then use it in all three places:
-p.Quit() -<-spinnerDone -fmt.Printf("\r✗ %s\n", message) -l.Error("Failed to describe the component", "component", component, "stack", stack, "error", err) -os.Exit(1) +handleTerraformError(p, spinnerDone, message, err, component, stack)Also applies to: 270-273, 286-289
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/exec/terraform_outputs.go(16 hunks)
🧰 Additional context used
🪛 GitHub Check: Build (macos-latest, macos)
internal/exec/terraform_outputs.go
[failure] 236-236:
undefined: l.Trace
[failure] 319-319:
undefined: u.Exit
🪛 GitHub Check: Build (ubuntu-latest, linux)
internal/exec/terraform_outputs.go
[failure] 236-236:
undefined: l.Trace
[failure] 319-319:
undefined: u.Exit
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Summary
🔇 Additional comments (2)
internal/exec/terraform_outputs.go (2)
6-6: LGTM! Good choice of logging package.The addition of
ospackage and structured logging usinggithub.com/charmbracelet/logaligns well with modern logging practices.Also applies to: 17-17
136-136: Verify TF_DATA_DIR handling in cleanTerraformWorkspace.As per PR objectives, this change should handle TF_DATA_DIR environment variable.
Let's verify the implementation:
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Check if cleanTerraformWorkspace handles TF_DATA_DIR # Search for TF_DATA_DIR handling rg -A 5 "TF_DATA_DIR" . # Check cleanTerraformWorkspace implementation ast-grep --pattern 'func cleanTerraformWorkspace($_, $_) { $$$ }'Length of output: 8195
TF_DATA_DIR handling in cleanTerraformWorkspace verified
The cleanTerraformWorkspace function (in internal/exec/terraform_utils.go, lines 27–70) retrieves TF_DATA_DIR via os.Getenv, defaults to ".terraform" when empty, and then normalizes the path as expected. This satisfies the PR objective regarding TF_DATA_DIR handling.
| err, | ||
| )) | ||
| l.Error("Error evaluating terraform output", "output", output, "component", component, "stack", stack, "error", err) | ||
| u.Exit(1) |
There was a problem hiding this comment.
Fix undefined u.Exit function.
The static analysis indicates that u.Exit is undefined.
Replace with the standard os.Exit:
-u.Exit(1)
+os.Exit(1)📝 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.
| u.Exit(1) | |
| os.Exit(1) |
🧰 Tools
🪛 GitHub Check: Build (macos-latest, macos)
[failure] 319-319:
undefined: u.Exit
🪛 GitHub Check: Build (ubuntu-latest, linux)
[failure] 319-319:
undefined: u.Exit
| @@ -235,7 +233,7 @@ | |||
| if !CheckTTYSupport() { | |||
| // set tea.WithInput(nil) workaround tea program not run on not TTY mod issue | |||
| opts = []tea.ProgramOption{tea.WithoutRenderer(), tea.WithInput(nil)} | |||
| u.LogTrace("No TTY detected. Falling back to basic output. This can happen when no terminal is attached or when commands are pipelined.") | |||
| l.Trace("No TTY detected. Falling back to basic output. This can happen when no terminal is attached or when commands are pipelined.") | |||
There was a problem hiding this comment.
Fix undefined l.Trace method.
The static analysis indicates that l.Trace is undefined.
Replace with the correct logging level:
-l.Trace("No TTY detected. Falling back to basic output. This can happen when no terminal is attached or when commands are pipelined.")
+l.Debug("No TTY detected. Falling back to basic output. This can happen when no terminal is attached or when commands are pipelined.")📝 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.
| l.Trace("No TTY detected. Falling back to basic output. This can happen when no terminal is attached or when commands are pipelined.") | |
| l.Debug("No TTY detected. Falling back to basic output. This can happen when no terminal is attached or when commands are pipelined.") |
🧰 Tools
🪛 GitHub Check: Build (macos-latest, macos)
[failure] 236-236:
undefined: l.Trace
🪛 GitHub Check: Build (ubuntu-latest, linux)
[failure] 236-236:
undefined: l.Trace
| l.Error("Failed to describe the component", "component", component, "stack", stack, "error", err) | ||
| os.Exit(1) |
There was a problem hiding this comment.
| l.Error("Failed to describe the component", "component", component, "stack", stack, "error", err) | |
| os.Exit(1) | |
| l.Fatal("Failed to describe the component", "component", component, "stack", stack, "error", err) |
| l.Error("Failed to get remote state backend static type outputs", "error", err) | ||
| os.Exit(1) |
There was a problem hiding this comment.
| l.Error("Failed to get remote state backend static type outputs", "error", err) | |
| os.Exit(1) | |
| l.Fatal("Failed to get remote state backend static type outputs", "error", err) |
| l.Error("Failed to execute terraform output", "component", component, "stack", stack, "error", err) | ||
| os.Exit(1) |
There was a problem hiding this comment.
| l.Error("Failed to execute terraform output", "component", component, "stack", stack, "error", err) | |
| os.Exit(1) | |
| l.Fatal("Failed to execute terraform output", "component", component, "stack", stack, "error", err) |
| l.Error("Error evaluating terraform output", "output", output, "component", component, "stack", stack, "error", err) | ||
| u.Exit(1) |
There was a problem hiding this comment.
| l.Error("Error evaluating terraform output", "output", output, "component", component, "stack", stack, "error", err) | |
| u.Exit(1) | |
| l.Fatal("Error evaluating terraform output", "output", output, "component", component, "stack", stack, "error", err) |
| l.Error("Error evaluating the 'static' remote state backend output", "output", output, "component", component, "stack", stack, "error", err) | ||
| os.Exit(1) |
There was a problem hiding this comment.
| l.Error("Error evaluating the 'static' remote state backend output", "output", output, "component", component, "stack", stack, "error", err) | |
| os.Exit(1) | |
| l.Fatal("Error evaluating the 'static' remote state backend output", "output", output, "component", component, "stack", stack, "error", err) |
what
why
references
Summary by CodeRabbit