[WIP] Stacks RFC: values with dependencies #4056
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughThe changes update label assignment and error handling in the Changes
Sequence Diagram(s)sequenceDiagram
participant T as Stack Processor
participant GU as generateUnits/Stacks
participant PC as processComponent
participant WV as writeValues
participant RV as ReadValues
participant U as Unit (with RawValues)
T->>GU: Initiate stack processing (with list of units)
GU->>PC: Process component with allUnits parameter
PC->>U: Resolve unit references via UnitReference
PC->>WV: Forward component data and unit list
WV->>RV: Trigger writing to terragrunt.values.hcl
RV->>U: Retrieve raw expressions using RawValues
RV-->>T: Return evaluation context with resolved dependencies
Suggested reviewers
🪧 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 (6)
config/hclparse/file.go (2)
78-88: Consider adjusting the switch block layout for clarity.The static analysis tool flags the cuddling of the switch statement with the variable declaration. Although this is purely style-related, separating the variable declaration from the switch statement can improve readability:
- var labelNames []string - switch name { + var labelNames []string + switch name { ... }🧰 Tools
🪛 golangci-lint (1.64.8)
79-79: switch statements should only be cuddled with variables switched
(wsl)
115-115: Consider usingfmt.Errorffor concise error creation.Instead of wrapping
fmt.Sprintfinsideerrors.New, you can usefmt.Errorfdirectly:-return nil, errors.New(fmt.Sprintf(multipleBlockDetailFmt, name)) +return nil, fmt.Errorf(multipleBlockDetailFmt, name)config/stack.go (4)
229-238: Pre-allocate theunitsslice.Since you know the length of
stacks, you can optimize by pre-allocating:- var units []*Unit + units := make([]*Unit, 0, len(stacks)) for _, stack := range stacks { ... }🧰 Tools
🪛 golangci-lint (1.64.8)
230-230: Consider pre-allocating
units(prealloc)
274-274: Remove or utilize the newunitsfield incomponentToProcess.This field is overshadowed by the
allUnitsparameter passed toprocessComponent. If you only needallUnits, the struct field can be removed to prevent confusion.🧰 Tools
🪛 golangci-lint (1.64.8)
274-274: field
unitsis unused(unused)
389-400: Pass context more thoroughly if possible.The static analysis tool hints that deeper calls (e.g.,
getOutputJSONWithCaching) might also need the context. Consider refactoring those calls to ensure they honor cancellation and deadlines.🧰 Tools
🪛 golangci-lint (1.64.8)
389-389: Function
getOutputJSONWithCaching->getTerragruntOutputJSON->cloneTerragruntOptionsForDependencyOutput->PartialParseConfigFileshould pass the context parameter(contextcheck)
390-390: only one cuddle assignment allowed before if statement
(wsl)
644-754: Consider further splitting or validating user expressions.While the current approach for writing values and dependencies works well, you may want to:
- Split this function for better maintainability.
- Add guards or explicit checks for malicious inputs that might embed unexpected HCL code.
Otherwise, the logic is coherent and addresses the new requirement for unit dependencies.
🧰 Tools
🪛 golangci-lint (1.64.8)
716-716: assignments should only be cuddled with other assignments
(wsl)
728-728: branch statements should not be cuddled if block has more than two lines
(wsl)
686-686: ranges should only be cuddled with assignments used in the iteration
(wsl)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
config/hclparse/file.go(2 hunks)config/stack.go(11 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.
config/hclparse/file.goconfig/stack.go
🧬 Code Definitions (1)
config/stack.go (7)
config/locals.go (6) (6)
err(59-59)err(249-251)err(253-255)err(259-261)_(170-170)diags(162-162)config/parsing_context.go (9) (9)
ctx(63-66)ctx(68-71)ctx(73-76)ctx(78-81)ctx(84-88)ctx(90-93)ctx(95-98)NewParsingContext(54-62)ParsingContext(16-52)options/options.go (13) (13)
opts(585-593)opts(597-602)opts(606-618)opts(647-670)opts(673-675)opts(678-684)opts(688-695)opts(698-729)opts(732-743)opts(746-758)opts(805-865)opts(867-885)TerragruntOptions(91-410)config/config.go (5) (5)
configPath(667-667)file(805-805)DefaultTerragruntConfigPath(44-44)config(776-776)config(1021-1021)config/hclparse/file.go (7) (7)
file(23-25)file(28-52)file(61-74)file(77-119)file(121-135)file(137-139)File(17-21)config/config_helpers.go (1) (1)
createTerragruntEvalContext(145-228)config/stack_validation.go (1) (1)
ValidateStackConfig(16-37)
🪛 golangci-lint (1.64.8)
config/hclparse/file.go
79-79: switch statements should only be cuddled with variables switched
(wsl)
config/stack.go
230-230: Consider pre-allocating units
(prealloc)
274-274: field units is unused
(unused)
389-389: Function getOutputJSONWithCaching->getTerragruntOutputJSON->cloneTerragruntOptionsForDependencyOutput->PartialParseConfigFile should pass the context parameter
(contextcheck)
461-461: Function getOutputJSONWithCaching->getTerragruntOutputJSON->cloneTerragruntOptionsForDependencyOutput->PartialParseConfigFile should pass the context parameter
(contextcheck)
511-511: assignments should only be cuddled with other assignments
(wsl)
516-516: return statements should not be cuddled if block has more than two lines
(wsl)
390-390: only one cuddle assignment allowed before if statement
(wsl)
553-553: type assertion on error will fail on wrapped errors. Use errors.As to check for specific errors
(errorlint)
585-585: ranges should only be cuddled with assignments used in the iteration
(wsl)
603-603: assignments should only be cuddled with other assignments
(wsl)
594-594: ranges should only be cuddled with assignments used in the iteration
(wsl)
716-716: assignments should only be cuddled with other assignments
(wsl)
728-728: branch statements should not be cuddled if block has more than two lines
(wsl)
686-686: ranges should only be cuddled with assignments used in the iteration
(wsl)
🔇 Additional comments (10)
config/hclparse/file.go (1)
91-94: Looks good.The dynamic assignment of
TypeandLabelNamesaligns well with the rest of the code.config/stack.go (9)
13-13: No issues with the additional import.Adding
"github.com/hashicorp/hcl/v2"for extended HCL usage is appropriate here.
45-49: Struct changes look good.The
RawValuesfield provides a place to store original expressions for each unit. Make sure it’s only used internally and handled carefully if user input can contain dangerous expressions.
60-64: Evaluate necessity ofUnitReferencestruct.Currently,
UnitReferenceappears unused in this diff. If it's part of a planned feature, leaving it in is fine; otherwise, consider removing it to avoid dead code.
216-216: No concerns with passing units intoprocessComponent.This aligns with the new signature for improved dependency resolution.
277-277: Signature update is valid.Accepting
allUnitsinprocessComponentensures correct handling of references and dependencies.
402-516: Logic for reading stack values looks solid.This correctly merges dependency outputs and the main values block. No immediate concerns.
🧰 Tools
🪛 golangci-lint (1.64.8)
461-461: Function
getOutputJSONWithCaching->getTerragruntOutputJSON->cloneTerragruntOptionsForDependencyOutput->PartialParseConfigFileshould pass the context parameter(contextcheck)
511-511: assignments should only be cuddled with other assignments
(wsl)
516-516: return statements should not be cuddled if block has more than two lines
(wsl)
537-568: Locals processing is properly integrated.Forwarding an empty map and decoding with the existing logic appears consistent with how the rest of the file handles locals.
🧰 Tools
🪛 golangci-lint (1.64.8)
553-553: type assertion on error will fail on wrapped errors. Use errors.As to check for specific errors
(errorlint)
570-614: Raw expression extraction is handled properly.Storing user-supplied expressions in
RawValuesis fine as implemented, assuming all usage is validated. This ensures references can be reconstructed accurately.🧰 Tools
🪛 golangci-lint (1.64.8)
585-585: ranges should only be cuddled with assignments used in the iteration
(wsl)
603-603: assignments should only be cuddled with other assignments
(wsl)
594-594: ranges should only be cuddled with assignments used in the iteration
(wsl)
624-641: Relative path calculation is correct.Using
filepath.Absandfilepath.Relis a standard approach for safe relative path derivation.
|
Thanks for submitting this PoC, @cgetzen . It's a neat idea! Please create an RFC following our GitHub issues based process so we can get more details on the design thought process, think through alternatives, etc. Make sure you link this PoC to it. It really helps to have something like this to understand how the implementation would work. Regarding the design as-is: I don't think it would be a good idea to have dependencies in I'd also like to get a better understanding of the edge cases in this proposal: What if two dependencies are named the same thing across these different files? What's the behavior? Can the |
…ined in terragrunt.values.hcl
Description
Easily pass values between unit definitions from the terragrunt.stack.hcl file while maintaining backwards compatibility with units.
Context:
This PR implements the following format:
Before merging:
TODOs
Read the Gruntwork contribution guidelines.
Release Notes (draft)
Added / Removed / Updated [X].
Migration Guide
Summary by CodeRabbit