fix: always fetch dep output from remote state when selected #4818
Conversation
pseudomorph
requested review from
denis256,
wakeful and
yhakbar
as code owners
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThe function getTerragruntOutputJSON now conditions the “init-folder” output path on both isInit and FetchDependencyOutputFromState. If FetchDependencyOutputFromState is true, it bypasses reading from the init folder and proceeds to fetch outputs from remote state. No exported/public signatures changed. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller
participant Config as config/dependency.go
participant InitFolder as Init Folder Reader
participant RemoteState as Remote State
Caller->>Config: getTerragruntOutputJSON(isInit, FetchDependencyOutputFromState)
alt isInit == true AND FetchDependencyOutputFromState == false
note over Config,InitFolder: Use init-folder outputs
Config->>InitFolder: getTerragruntOutputJSONFromInitFolder()
InitFolder-->>Config: Outputs JSON
Config-->>Caller: Outputs JSON
else
note over Config,RemoteState: Fetch from remote state
Config->>RemoteState: Read outputs from state
RemoteState-->>Config: Outputs JSON
Config-->>Caller: Outputs JSON
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Pre-merge checks (2 passed, 1 warning)❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. ✨ Finishing Touches
🧪 Generate unit tests
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. Comment |
![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/dependency.go (2)
797-816: Skip unnecessary isInit check when always-fetch-from-state is setMinor perf/IO win: we still call terragruntAlreadyInit even though the result is ignored when FetchDependencyOutputFromState is true. Short-circuit earlier and only compute isInit/workingDir when the flag is false.
Apply this refactor:
@@ - // In optimization mode, see if there is already an init-ed folder that terragrunt can use, and if so, run - // `terraform output` in the working directory. - isInit, workingDir, err := terragruntAlreadyInit(l, targetTGOptions, targetConfig, ctx) - if err != nil { - return nil, err - } + // NOTE: When FetchDependencyOutputFromState is set, skip checking for an init-ed folder entirely. @@ - // Do not fetch from init-ed folder if user has explicitly asked to fetch from state. - if isInit && !ctx.TerragruntOptions.FetchDependencyOutputFromState { - return getTerragruntOutputJSONFromInitFolder(ctx, l, workingDir, remoteStateTGConfig.GetIAMRoleOptions()) - } + if ctx.TerragruntOptions.FetchDependencyOutputFromState { + return getTerragruntOutputJSONFromRemoteState( + ctx, + l, + targetConfig, + remoteStateTGConfig.RemoteState, + remoteStateTGConfig.GetIAMRoleOptions(), + ) + } + + // In optimization mode, see if there is already an init-ed folder; if so, read outputs directly. + isInit, workingDir, err := terragruntAlreadyInit(l, targetTGOptions, targetConfig, ctx) + if err != nil { + return nil, err + } + if isInit { + return getTerragruntOutputJSONFromInitFolder(ctx, l, workingDir, remoteStateTGConfig.GetIAMRoleOptions()) + }
1039-1053: Typos + avoid extra allocation in S3 pathRename steateBody → stateBody and unmarshal directly from bytes to reduce a string alloc/copy.
- steateBody, err := io.ReadAll(result.Body) + stateBody, err := io.ReadAll(result.Body) @@ - jsonState := string(steateBody) - jsonMap := make(map[string]any) - - err = json.Unmarshal([]byte(jsonState), &jsonMap) + var jsonMap map[string]any + err = json.Unmarshal(stateBody, &jsonMap)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
config/dependency.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
⚙️ CodeRabbit configuration file
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.
Files:
config/dependency.go
🧬 Code graph analysis (1)
config/dependency.go (1)
options/options.go (1)
TerragruntOptions(100-332)
🔇 Additional comments (1)
config/dependency.go (1)
812-815: Flag now correctly bypasses init-ed folder — LGTMThis enforces the intended behavior: when FetchDependencyOutputFromState is true, we no longer read from an init-ed working dir. Matches the PR objective without affecting the false case.
|
This can also happen when there is a terragrunt unit that is using loose tf files instead of a 
When this unit is used as dependency, the code path in question is triggered, and then even with the 'fetch output from state` option set, it instead calls terraform output I think the proposed fix is fine, I had dabbed with reordering the operations to reach for remote state before running output in an init'd dependency d69eed7 I'm not familiar enough with terragrunt to know which is a more appropriate fix. Because the init check doesn't hit on the more common path |
|
@pseudomorph thanks for opening this PR. It seems entirely reasonable. Do you mind adding some testing to confirm that this resolves the issue we're looking to address here? |
Description
There are cases where parallelization might affect an init-ed dir during read, such as when using Terragrunt with Atlantis. In these cases, when a user passes the option to fetch dependency output from (remote) state, it should not use output from init-ed dirs.
TODOs
Read the Gruntwork contribution guidelines.
Release Notes (draft)
Updated: --fetch-dependency-output-from-state now will always pull directly from state, even when dependency dirs are init-ed.
Migration Guide
Summary by CodeRabbit