fix: Adding explicit check to see if user set --tf-path
#4493
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
📝 WalkthroughWalkthroughThis change refactors the handling of the Terraform binary path option throughout the codebase, renaming the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant Options
participant Config
User->>CLI: Run terragrunt with --tf-path or TG_TF_PATH
CLI->>Options: Set TFPath, set TFPathExplicitlySet = true
CLI->>Config: Load terragrunt.hcl (terraform_binary)
Config->>Options: If !TFPathExplicitlySet, set TFPath from config
CLI->>CLI: Use Options.TFPath to invoke Terraform/OpenTofu
Assessment against linked issues
Possibly related PRs
Suggested reviewers
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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
|
yhakbar
force-pushed
the
fix/fixing-tf-path-again
branch
from
4b9994e to
cab47c5
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
test/helpers/package.go(1 hunks)test/integration_test.go(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- test/helpers/package.go
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.go`: Review the Go code for quality and correctness. Make sure that the Go code follows best practices, is performant, and is easy to understand and maintain.
**/*.go: Review the Go code for quality and correctness. Make sure that the Go code follows best practices, is performant, and is easy to understand and maintain.
⚙️ Source: CodeRabbit Configuration File
List of files the instruction was applied to:
test/integration_test.go
🧠 Learnings (2)
📓 Common learnings
Learnt from: yhakbar
PR: gruntwork-io/terragrunt#4169
File: cli/commands/hcl/validate/cli.go:29-60
Timestamp: 2025-04-17T13:02:28.098Z
Learning: Avoid shadowing imported packages with local variables in Go code, such as using a variable named `flags` when the `github.com/gruntwork-io/terragrunt/cli/flags` package is imported. Use more specific variable names like `flagSet` instead.
Learnt from: yhakbar
PR: gruntwork-io/terragrunt#3868
File: docs-starlight/patches/@astrojs%[email protected]:33-33
Timestamp: 2025-02-10T23:20:04.295Z
Learning: In Terragrunt projects, all `.hcl` files can be assumed to be Terragrunt configurations by default, with specific exceptions like `.terraform.lock.hcl` that need explicit handling.
test/integration_test.go (2)
undefined
<retrieved_learning>
Learnt from: yhakbar
PR: #4169
File: cli/commands/hcl/validate/cli.go:29-60
Timestamp: 2025-04-17T13:02:28.098Z
Learning: Avoid shadowing imported packages with local variables in Go code, such as using a variable named flags when the github.com/gruntwork-io/terragrunt/cli/flags package is imported. Use more specific variable names like flagSet instead.
</retrieved_learning>
<retrieved_learning>
Learnt from: levkohimins
PR: #3723
File: cli/commands/stack/action.go:160-160
Timestamp: 2025-02-10T13:36:19.542Z
Learning: The project uses a custom error package github.com/gruntwork-io/terragrunt/internal/errors which provides similar functionality to fmt.Errorf but includes stack traces. Prefer using this package's error functions (e.g., errors.Errorf, errors.New) over the standard library's error handling.
</retrieved_learning>
🧬 Code Graph Analysis (1)
test/integration_test.go (2)
test/helpers/package.go (7)
CleanupTerraformFolder(820-827)CopyEnvironment(86-99)IsTerraformInstalled(755-757)IsOpenTofuInstalled(750-752)TofuBinary(64-64)TerraformBinary(63-63)RunTerragruntCommandWithOutput(935-939)util/file.go (1)
JoinPath(481-483)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: Test (Windows)
- GitHub Check: License Check
- GitHub Check: Test OIDC (GHA AWS)
- GitHub Check: Test (macos)
- GitHub Check: build-and-test
- GitHub Check: Pull Request has non-contributor approval
🔇 Additional comments (3)
test/integration_test.go (3)
114-115: LGTM! Clear constant naming for test fixtures.The split of the original constant into two more specific ones (
testFixtureTfPathBasicandtestFixtureTfPathTofuTerraform) improves clarity and supports the test restructuring.
4152-4166: LGTM! Clean fixture path update.The update to use
testFixtureTfPathBasicis consistent with the test refactoring and maintains the existing test logic.
4168-4226: Excellent comprehensive test for tf-path binary overrides.This test effectively validates the interaction between feature flags and tf-path overrides with both Terraform and OpenTofu. Key strengths:
- Proper prerequisite checking with binary availability
- Comprehensive test cases covering various flag/binary combinations
- Clear expected outcomes for each scenario
- Good use of table-driven test pattern
- Follows established testing patterns in the codebase
The test ensures that tf-path overrides work correctly regardless of feature flag settings, which is important for user experience.
| if tfPath := os.Getenv("TG_TF_PATH"); tfPath != "" { | ||
| // Unset after using t.Setenv so that it'll be reset after the test. | ||
| t.Setenv("TG_TF_PATH", "") | ||
| os.Unsetenv("TG_TF_PATH") | ||
| } |
There was a problem hiding this comment.
Fix inconsistent environment variable handling.
The current pattern is problematic:
t.Setenv("TG_TF_PATH", "") // Sets to empty string, schedules restoration
os.Unsetenv("TG_TF_PATH") // Immediately unsets completelyThis creates undefined behavior. Choose one approach:
- Option 1 (Recommended): If you need the variable to be empty:
-t.Setenv("TG_TF_PATH", "")
-os.Unsetenv("TG_TF_PATH")
+t.Setenv("TG_TF_PATH", "")- Option 2: If you need the variable completely unset, don't use
t.Setenvwhich schedules restoration:
-t.Setenv("TG_TF_PATH", "")
-os.Unsetenv("TG_TF_PATH")
+os.Unsetenv("TG_TF_PATH")
+t.Cleanup(func() { os.Setenv("TG_TF_PATH", tfPath) })🤖 Prompt for AI Agents
In test/integration_test.go around lines 4140 to 4144, the environment variable
TG_TF_PATH is being set to an empty string using t.Setenv and then immediately
unset with os.Unsetenv, causing inconsistent behavior. To fix this, either use
only t.Setenv("TG_TF_PATH", "") if you want the variable to be empty and
restored after the test, or use only os.Unsetenv("TG_TF_PATH") if you want it
completely unset without restoration. Remove the redundant call to ensure
consistent environment variable handling.
Description
Fixes #4489
Separately track if a user has explicitly set the
--tf-path, and use that instead of checking if the version in--tf-pathdiffers from the default.TODOs
Read the Gruntwork contribution guidelines.
Release Notes (draft)
Updated implementation of
--tf-pathcheck to just store a boolean indicating that the user has explicitly set the--tf-pathflag, rather than checking if the version in--tf-pathdiffers from the default.Migration Guide
Summary by CodeRabbit
Refactor
TerraformPathtoTFPath.Tests
Chores