feat: add engine support to dependencies #3974
Conversation
Add support for parsing the `engine` block to `PartialParseConfig` so that terraform commands run in `dependency` blocks use the engine config if present and enabled.
partcyborg
requested review from
denis256,
levkohimins and
yhakbar
as code owners
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
📝 WalkthroughWalkthroughThe update introduces support for an additional "engine" block in the Terragrunt configuration. A new constant, Changes
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)
🪧 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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
config/config_partial.go (2)
86-90: Unused struct declarationThe
terragruntEnginestruct is declared but never used. The EngineBlock case (lines 454-462) directly usesEngineConfiginstead of this struct.Consider either:
- Removing this struct if it's not needed
- Using it consistently by updating the EngineBlock case to follow the pattern used for other blocks:
case EngineBlock: - decoded := EngineConfig{} + decoded := terragruntEngine{} err := file.Decode(&decoded, evalParsingContext) if err != nil { return nil, err } - output.Engine = &decoded + output.Engine = &decoded.Engine🧰 Tools
🪛 golangci-lint (1.62.2)
87-87: type
terragruntEngineis unused(unused)
454-462: Consider following consistent pattern for block parsingThis implementation directly decodes into an
EngineConfigtype rather than using a wrapper struct like other block types (e.g.,terragruntRemoteStatefor RemoteStateBlock).For consistency with other code patterns in this file, consider using the declared
terragruntEnginestruct:case EngineBlock: - decoded := EngineConfig{} + decoded := terragruntEngine{} err := file.Decode(&decoded, evalParsingContext) if err != nil { return nil, err } - output.Engine = &decoded + output.Engine = &decoded.Engine🧰 Tools
🪛 golangci-lint (1.62.2)
461-461: assignments should only be cuddled with other assignments
(wsl)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
config/config_partial.go(4 hunks)config/dependency.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.go`: Review the Go code for quality and correctness. M...
**/*.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.
config/dependency.goconfig/config_partial.go
🪛 golangci-lint (1.62.2)
config/config_partial.go
87-87: type terragruntEngine is unused
(unused)
461-461: assignments should only be cuddled with other assignments
(wsl)
🔇 Additional comments (3)
config/dependency.go (1)
759-759: LGTM: Added engine block support for dependenciesThis change enhances the
PartialParseConfigFilecall to include the newEngineBlockparameter, allowing dependency configurations to utilize engine settings if present.config/config_partial.go (2)
38-38: LGTM: Added EngineBlock to PartialDecodeSectionTypeAdding the
EngineBlockconstant to the enum allows the parser to recognize the engine block section.
339-339: LGTM: Updated documentationDocumentation updated to include the new
EngineBlockin the list of valid values.
There was a problem hiding this comment.
Thanks for cutting this PR!
There are some things I'd like addressed, if you don't mind.
- Please do add a test that uses a dependency via the engine config. A simple test added here to verify that dependencies are loaded with the engine block would be great. If you don't have time for that or aren't comfortable, please say so, and we can look to add it for you in another PR.
- See the comment generated by CodeRabbit here. It looks like there's an unused
terragruntEnginestruct. Please address that.
Thanks for the quick followup. I fixed the CodeRabbit issue. I can add a test that utilizes a dependency, but upon a quick scan of the existing integration tests, it isn't clear to me exactly how I verify that the dependency block is actually following the engine codepath, outside of building a custom client or server that i can somehow verify the list of calls made through it. This would be a pretty big departure from the other tests as they are all using pre-built engine client/servers. Do you have any thoughts on this? Or is it enough to just have a test that uses 2 modules with dependencies? |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
config/config_partial.go (1)
448-456: Linting issue with assignment formattingThe implementation for parsing the
EngineBlockcorrectly follows the pattern used for other blocks. However, there's a linting issue where the assignment at line 455 should follow other assignments rather than being placed directly after the error handling block.err := file.Decode(&decoded, evalParsingContext) if err != nil { return nil, err } + output.Engine = &decoded🧰 Tools
🪛 golangci-lint (1.62.2)
455-455: assignments should only be cuddled with other assignments
(wsl)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
config/config_partial.go(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.go`: Review the Go code for quality and correctness. M...
**/*.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.
config/config_partial.go
🪛 golangci-lint (1.62.2)
config/config_partial.go
455-455: assignments should only be cuddled with other assignments
(wsl)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Pull Request has non-contributor approval
🔇 Additional comments (4)
config/config_partial.go (4)
38-38: Good addition of the EngineBlock constantThe addition of
EngineBlockto thePartialDecodeSectionTypeenum aligns with the PR's objective to support parsing the engine block within the configuration.
333-333: Well-documented new featureDocumentation is properly updated to include the new
EngineBlockin the list of blocks that can be parsed, which maintains good code documentation standards.
448-456: Previous review mentioned an unused "terragruntEngine"A previous review comment mentioned that
terragruntEngineseems to be unused. This variable doesn't appear in the current implementation. Please verify if this was addressed or is still relevant.#!/bin/bash # Check for occurrences of terragruntEngine in the codebase rg "terragruntEngine" config/🧰 Tools
🪛 golangci-lint (1.62.2)
455-455: assignments should only be cuddled with other assignments
(wsl)
448-456:✅ Verification successful
Verify implementation completeness
The PR aims to allow Terraform commands in dependency blocks to use the engine configuration. Let's verify that the
EngineBlockis properly used in the dependency-related code.
🏁 Script executed:
#!/bin/bash # Check where EngineBlock is used in the codebase, particularly in dependency-related code rg "EngineBlock" --type go rg -A 10 "getTerragruntOutputJSON" config/dependency.goLength of output: 5651
EngineBlock integration verified for dependency configuration
- The engine block decoding in
config/config_partial.go(lines 448–456) correctly decodes and assigns the Engine configuration.- In
config/dependency.go, the call toPartialParseConfigFileincludesEngineBlock(along with other blocks), ensuring that Terraform commands in dependency blocks can properly use the engine configuration.- No further changes are necessary.
🧰 Tools
🪛 golangci-lint (1.62.2)
455-455: assignments should only be cuddled with other assignments
(wsl)
It can be as simple as checking logs to verify that the engine was run twice: $ eza -T
.
├── dependency
│ ├── main.tf
│ └── terragrunt.hcl
└── dependent
├── main.tf
└── terragrunt.hcl
$ fd -tf -x bash -c 'echo "# {}" && cat {} && echo'
# ./dependency/terragrunt.hcl
engine {
source = "github.com/gruntwork-io/terragrunt-engine-opentofu"
version = "v0.0.16"
}
# ./dependent/terragrunt.hcl
engine {
source = "github.com/gruntwork-io/terragrunt-engine-opentofu"
version = "v0.0.16"
}
dependency "dependency" {
config_path = "../dependency"
}
# ./dependency/main.tf
output "foo" {
value = "foo"
}
# ./dependent/main.tf
$ terragrunt plan --experimental-engine
15:22:53.810 ERROR [../dependency] tofu: Tofu Initialization started
15:22:53.810 ERROR [../dependency] tofu: Tofu Initialization completed
15:22:53.881 INFO [../dependency] tofu: Initializing the backend...
15:22:53.882 INFO [../dependency] tofu: Initializing provider plugins...
15:22:53.882 INFO [../dependency] tofu: OpenTofu has been successfully initialized!
15:22:53.882 INFO [../dependency] tofu:
15:22:53.882 INFO [../dependency] tofu: You may now begin working with OpenTofu. Try running "tofu plan" to see
15:22:53.882 INFO [../dependency] tofu: any changes that are required for your infrastructure. All OpenTofu commands
15:22:53.882 INFO [../dependency] tofu: should now work.
15:22:53.882 INFO [../dependency] tofu: If you ever set or change modules or backend configuration for OpenTofu,
15:22:53.882 INFO [../dependency] tofu: rerun this command to reinitialize your working directory. If you forget, other
15:22:53.882 INFO [../dependency] tofu: commands will detect it and remind you to do so if necessary.
15:22:53.936 ERROR tofu: Tofu Initialization started
15:22:53.937 ERROR tofu: Tofu Initialization completed
15:22:53.969 INFO tofu: Initializing the backend...
15:22:53.970 INFO tofu: Initializing provider plugins...
15:22:53.970 INFO tofu: OpenTofu has been successfully initialized!
15:22:53.970 INFO tofu:
15:22:53.970 INFO tofu: You may now begin working with OpenTofu. Try running "tofu plan" to see
15:22:53.970 INFO tofu: any changes that are required for your infrastructure. All OpenTofu commands
15:22:53.970 INFO tofu: should now work.
15:22:53.970 INFO tofu: If you ever set or change modules or backend configuration for OpenTofu,
15:22:53.971 INFO tofu: rerun this command to reinitialize your working directory. If you forget, other
15:22:53.971 INFO tofu: commands will detect it and remind you to do so if necessary.
15:22:54.009 STDOUT tofu: No changes. Your infrastructure matches the configuration.
15:22:54.010 STDOUT tofu: OpenTofu has compared your real infrastructure against your configuration and
15:22:54.010 STDOUT tofu: found no differences, so no changes are needed.
15:22:54.013 ERROR [../dependency] tofu: Tofu Shutdown completed
15:22:54.015 ERROR [../dependency] 2025-03-06T15:22:54.015-0500 [INFO] plugin process exited: plugin=../../../.cache/terragrunt/plugins/iac-engine/rpc/v0.0.16/darwin/arm64/terragrunt-iac-engine-opentofu_rpc_v0.0.16_darwin_arm64 id=40806
15:22:54.017 ERROR tofu: Tofu Shutdown completed
15:22:54.019 ERROR 2025-03-06T15:22:54.019-0500 [INFO] plugin process exited: plugin=../../../.cache/terragrunt/plugins/iac-engine/rpc/v0.0.16/darwin/arm64/terragrunt-iac-engine-opentofu_rpc_v0.0.16_darwin_arm64 id=40809It's brittle, and ideally, we would have a better method of verifying this kind of thing, but that's not really in scope for this PR. As long as we can verify that it's working in tests, we're good. If this test fails because we adjust logging too much, then we can create a better system. I created #3980 while playing with this, as we can do much better at communicating to users that they're using an engine than what's displayed here. |
I have now added an integration test that includes a terragrunt dependency block and verifies that it uses the engine for both |
| if_exists = "overwrite_terragrunt" | ||
| } | ||
| config = { | ||
| path = "${get_terragrunt_dir()}/terraform.tfstate" |
There was a problem hiding this comment.
NIT: always bias towards OpenTofu. Don't change this now, we can edit after this PR.
|
Everything looks good, @partcyborg . If tests pass, I'll merge this in. Thanks for contributing this! We're invested in your success with engines. Keep us in the loop as you adopt them. |
Thanks @yhakbar again for all your help. I noticed that integration_test_terraform_aws is failing, but unfortunately I can't see why as I can't login to your circleci. Is there something I need to fix with my code to make this test pass again? |
Hey @partcyborg Sorry, I was looking into this issue, then got sidetracked. This is the assertion that's failing in You should be able to run that test locally to investigate. I'm not sure why it's failing for you, but we will need that resolved before we can merge. |
Thanks again @yhakbar for the quick followup. I found two problems with the code:
Please let me know if this is the right approach, as it did not appear to be appropriate to try and inject the engine functionality into the optimized codepaths. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
config/config_partial.go (1)
455-462: Fix code style: address linting issuesThe implementation of the
EngineBlockcase follows the same pattern as other cases, but there are code style violations from the linter regarding "cuddled" assignments.Consider adding a blank line after line 461 to address the linting issues:
} output.Engine = decoded.Engine +🧰 Tools
🪛 golangci-lint (1.62.2)
461-461: assignments should only be cuddled with other assignments
(wsl)
458-458: only one cuddle assignment allowed before if statement
(wsl)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
config/config_partial.go(4 hunks)config/dependency.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- config/dependency.go
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.go`: Review the Go code for quality and correctness. M...
**/*.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.
config/config_partial.go
🧠 Learnings (1)
config/config_partial.go (1)
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.
🪛 golangci-lint (1.62.2)
config/config_partial.go
461-461: assignments should only be cuddled with other assignments
(wsl)
458-458: only one cuddle assignment allowed before if statement
(wsl)
🔇 Additional comments (3)
config/config_partial.go (3)
38-38: LGTM: New engine block constantAdding
EngineBlockto thePartialDecodeSectionTypeenum follows the same pattern as other configuration block types.
122-126: LGTM: New terragruntEngine structThe
terragruntEnginestruct is well-defined and follows the same pattern as other block-specific structs in the file, with anEnginefield for the parsed configuration and aRemainfield for any additional HCL content.
339-339: LGTM: Documentation updateDocumentation for
PartialParseConfigStringfunction has been properly updated to include the newEngineBlocktype.
I don't understand this comment. There is already a blank line after |
Sometimes the AI makes mistakes. If it seems wrong, just ignore it. If you don't have
The optimized codepath being taken here is a short circuit when attempting to parse dependencies, preventing dependencies of dependencies from being parsed. For the most part, nothing about engines should be affecting that, so I'm confused as to why your changes had any impact on that. We parse things like the I can look into this a bit further next week. |
I guess I could refactor |
This was easier than I thought to wire through. I shouldn't have given up so easily 🙂 Engines now work as expected for dependencies in both the optimized path and the un-optimized path. |
|
Setting a reminder to review this on Monday. Great job, @partcyborg ! |
partcyborg
force-pushed
the
engine-dependencies
branch
from
f49af73 to
60632e1
Compare
Description
Fixes #3973
Add support for parsing the
engineblock toPartialParseConfigso that terraform commands run independencyblocks use the engine config if present and enabled.TODOs
Release Notes (draft)
Added support for IaC engines to dependency blocks.
Summary by CodeRabbit
Summary by CodeRabbit
.gitignorefile to exclude specific files from version control.