Fix plan output relative #4492
Fix plan output relative #4492
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
📝 Walkthrough""" WalkthroughNew Terraform and Terragrunt fixture directories for Changes
Sequence Diagram(s)sequenceDiagram
participant TestRunner as TestTerragruntPlanAllOutput
participant Terragrunt as terragrunt CLI
participant FS as Filesystem
TestRunner->>FS: Setup fixture environment and output directory
TestRunner->>Terragrunt: Run 'terragrunt plan --all --out-dir plans --working-dir .'
Terragrunt->>FS: Generate plan files in plans/vnet/, plans/resource-group/, plans/private-dns-zone/
TestRunner->>FS: Assert plan files exist in expected subdirectories
Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 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
|
![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
🧹 Nitpick comments (3)
configstack/module.go (1)
137-142: Good fix for relative path computation, but address the linting issue.The logic correctly converts the working directory to an absolute path before computing the relative path, which resolves the issue with
filepath.Rel()when dealing with absolute module paths. The fallback to the original working directory on error is appropriate defensive programming.However, there's a pipeline failure indicating that assignments should be cuddled with other assignments (WSL linting rule).
Apply this diff to fix the linting issue:
// module path will be an absolute path so working dir should absolute too so that Rel() can work wdir, err := filepath.Abs(opts.WorkingDir) if err != nil { wdir = opts.WorkingDir } + path, _ := filepath.Rel(wdir, module.Path)test/fixtures/plan-output/private-dns-zone/main.tf (1)
1-5: Specify variable type for clarityEven in test fixtures, it’s generally a good habit to declare the variable’s
type(e.g.,string) and an optionaldescriptionso thatterraform validatesurfaces clear metadata.
Example:variable "terragrunt_dir" { type = string description = "Path to the module directory supplied by Terragrunt" }test/fixtures/plan-output/resource-group/main.tf (1)
1-5: Mirror variable metadata across fixturesFor consistency with other modules (and to help automated linting), add
typeanddescriptionblocks to theterragrunt_dirvariable here as well, mirroring the suggestion made forprivate-dns-zone/main.tf.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
configstack/module.go(1 hunks)test/fixtures/plan-output/private-dns-zone/main.tf(1 hunks)test/fixtures/plan-output/private-dns-zone/terragrunt.hcl(1 hunks)test/fixtures/plan-output/resource-group/main.tf(1 hunks)test/fixtures/plan-output/resource-group/terragrunt.hcl(1 hunks)test/fixtures/plan-output/vnet/main.tf(1 hunks)test/fixtures/plan-output/vnet/terragrunt.hcl(1 hunks)test/integration_test.go(2 hunks)
🧰 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:
configstack/module.gotest/integration_test.go
🧠 Learnings (7)
test/fixtures/plan-output/vnet/terragrunt.hcl (3)
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.
Learnt from: partcyborg
PR: gruntwork-io/terragrunt#3974
File: config/config_partial.go:448-456
Timestamp: 2025-03-06T23:44:09.413Z
Learning: The TerragruntConfig struct in config/config.go does contain an Engine field that's used to store engine configuration data.
Learnt from: levkohimins
PR: gruntwork-io/terragrunt#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.
test/fixtures/plan-output/resource-group/main.tf (1)
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/fixtures/plan-output/vnet/main.tf (1)
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/fixtures/plan-output/private-dns-zone/main.tf (1)
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/fixtures/plan-output/resource-group/terragrunt.hcl (3)
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.
Learnt from: partcyborg
PR: gruntwork-io/terragrunt#3974
File: config/config_partial.go:448-456
Timestamp: 2025-03-06T23:44:09.413Z
Learning: The TerragruntConfig struct in config/config.go does contain an Engine field that's used to store engine configuration data.
Learnt from: levkohimins
PR: gruntwork-io/terragrunt#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.
test/fixtures/plan-output/private-dns-zone/terragrunt.hcl (3)
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.
Learnt from: partcyborg
PR: gruntwork-io/terragrunt#3974
File: config/config_partial.go:448-456
Timestamp: 2025-03-06T23:44:09.413Z
Learning: The TerragruntConfig struct in config/config.go does contain an Engine field that's used to store engine configuration data.
Learnt from: levkohimins
PR: gruntwork-io/terragrunt#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.
test/integration_test.go (1)
undefined
<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 (3)
CleanupTerraformFolder(713-720)CopyEnvironment(84-97)RunTerragruntCommand(786-790)util/file.go (1)
FileExists(58-61)
🪛 GitHub Actions: Lint
configstack/module.go
[error] 142-142: golangci-lint wsl: assignments should only be cuddled with other assignments
test/integration_test.go
[error] 4203-4203: golangci-lint usetesting: os.Chdir() could be replaced by t.Chdir() in TestTerragruntPlanAllOutput
[error] 4205-4205: golangci-lint usetesting: os.Chdir() could be replaced by t.Chdir() in TestTerragruntPlanAllOutput
🪛 GitHub Actions: Strict Lint
configstack/module.go
[error] 231-231: Cyclomatic complexity for function FindWhereWorkingDirIsIncluded is 11, exceeding max of 10.
[error] 454-454: Cyclomatic complexity for function flagUnitsThatAreIncluded is 13, exceeding max of 10.
[error] 635-635: Global variable 'existingModules' detected (gochecknoglobals).
test/integration_test.go
[error] 3806-3806: Lint error: declaration has 3 blank identifiers (dogsled).
[error] 905-905: Use of 'fmt.Printf' forbidden by pattern (forbidigo).
[error] 4219-4219: Use of 'fmt.Printf' forbidden by pattern (forbidigo).
[error] 1409-2508: Multiple type assertions must be checked (forcetypeassert).
[error] 281-3943: Function length exceeds limit (funlen).
[error] 1583-3998: Multiple unnecessary trailing newlines detected (whitespace).
⏰ Context from checks skipped due to timeout of 90000ms (21)
- GitHub Check: Test (Windows)
- GitHub Check: Build (windows/386)
- GitHub Check: Build (darwin/amd64)
- GitHub Check: Build (windows/amd64)
- GitHub Check: Build (linux/arm64)
- GitHub Check: Test (AWS with Latest OSS Terraform)
- GitHub Check: Test (Mock)
- GitHub Check: Build (darwin/amd64)
- GitHub Check: Test (CAS)
- GitHub Check: Test (AWS with Latest Terraform)
- GitHub Check: Test (Provider Cache with Tofu)
- GitHub Check: Test (Provider Cache with Latest Terraform)
- GitHub Check: Test (Fixtures with Latest Terraform)
- GitHub Check: Test (GCP)
- GitHub Check: Test (AWSGCP)
- GitHub Check: Test (Fixtures with OpenTofu)
- GitHub Check: Test (Fixtures with Latest OSS Terraform)
- GitHub Check: Test (Tflint)
- GitHub Check: Test (ubuntu)
- GitHub Check: Test (macos)
- GitHub Check: build-and-test
🔇 Additional comments (5)
test/fixtures/plan-output/private-dns-zone/terragrunt.hcl (1)
1-7: LGTM! Well-structured test fixture.The Terragrunt configuration is appropriate for testing plan output functionality. The simple setup with source pointing to current directory and terragrunt_dir input aligns well with the test requirements.
test/fixtures/plan-output/vnet/terragrunt.hcl (1)
1-7: LGTM! Consistent test fixture pattern.The configuration follows the same pattern as other test fixtures in the plan-output directory, which ensures consistency in the test setup. The minimal configuration is appropriate for testing plan output functionality.
test/fixtures/plan-output/resource-group/terragrunt.hcl (1)
1-7: LGTM! Maintains consistent test fixture pattern.The configuration is identical to the other test fixtures in the plan-output directory, which ensures uniform behavior during integration testing. This consistency is valuable for comprehensive validation of the plan output functionality.
test/fixtures/plan-output/vnet/main.tf (1)
1-5: LGTM! Simple and effective test fixture.The Terraform configuration is minimal yet complete for testing purposes. The variable declaration without a default value ensures it must be provided via Terragrunt inputs, and the output allows verification of the value. This works perfectly with the corresponding terragrunt.hcl file.
test/integration_test.go (1)
95-95: LGTM! Test fixture constant is properly added.The new test fixture constant follows the existing naming convention and is correctly placed with other fixture constants.
|
Looks like are failing tests related to |
Might be my fault. Let me update the PR and see if that fixes it. |
|
I don't see |
|
Looks like after runner-pool changes got merged, branch have conflicts on |
- Keep both TestTerragruntPlanAllOutput and TestMixedStackConfigIgnored test functions - Remove configstack/module.go as it was refactored to internal/runner in main
- Make working directory absolute before calculating relative paths in getPlanFilePath - This ensures filepath.Rel() works correctly when both paths are absolute - Fixes TestTerragruntPlanAllOutput test that was failing due to incorrect plan file paths
- Use t.Chdir() instead of manual directory change with cleanup - Use t.Logf() instead of fmt.Printf() for test output - Remove t.Parallel() since t.Chdir() requires non-parallel execution
|
Looks like it is failing on lint |
- Remove extra blank line in TestTerragruntPlanAllOutput function - Add proper spacing in unit.go to resolve wsl cuddling violations
Hi @denis256, one of the previous lint comment suggested to use Original change here: dc489ee |
Why use chdir at all instead of using |
Hi Denis and Yousif, thanks for pointing that out. I originally used t.Chdir() because when running terragrunt plan, the current working directory influences where the plan output (e.g., modules like vnet, private-dns-zone, resource-group) is placed under the --out-dir. To reproduce the original issue deterministically, we need to run terragrunt plan from within the fixture directory — the directory that contains the actual Terragrunt configuration. If we don’t chdir into that directory, Terragrunt generates output paths under something like /tmp/test//{vnet, ...}, which varies depending on how the test is run and where the fixture is located. That makes it hard to assert on the output structure. By explicitly chdiring into the fixture dir, the generated output goes under the expected /tmp/test/{vnet, private-dns-zone, resource-group}, making the test predictable and easier to validate. I saw an earlier lint suggestion to use t.Chdir(), which unfortunately conflicts with t.Parallel(), since t.Chdir() is not safe for parallel tests. I’m happy to follow whichever approach you prefer: Let me know which direction you’d prefer. Thanks! |
I don't think you're going to like my response here, but neither! 😁 I think we should be using There should be no difference in the binary between changing directories before running Terragrunt and running Terragrunt with |
|
Hi @yhakbar, made the change according to your suggestion. PTAL when you get a chance. |
|
@yhakbar - Might it make more sense to add a setter to the WorkingDir flag? The default working dir is an absolute path when not specified. Seems like there are a few different issues related to the abs/rel pathing for working dir. |
|
I've figured a top-level fix which allows the tests here to pass and addresses issues I was seeing with using I'll cut an issue for the |
OH interesting. Feel free to contribute to this PR and we can work on things together. Thanks a lot @pseudomorph |
Description
Fixes #4430.
This pull request introduces enhancements to the handling of module paths, adds new test fixtures for validating Terragrunt plans, and implements a new integration test for verifying
terragrunt plan --allfunctionality. Below is a summary of the most important changes:Enhancements to module path handling: Updated
getPlanFilePathinconfigstack/module.goto ensureopts.WorkingDiris converted to an absolute path before callingfilepath.Rel, improving compatibility with absolute module paths.TODOs
Read the Gruntwork contribution guidelines.
Release Notes (draft)
Added / Removed / Updated [X].
Migration Guide
Summary by CodeRabbit