Stacks: values generation #3914
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThis pull request simplifies stack command execution by removing the Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant CLI as CLI Command
participant GS as generateStack
participant PS as processStackFile
participant WUV as WriteUnitValues
participant RUV as ReadUnitValues
U->>CLI: Execute stack command
CLI->>GS: Call generateStack
GS->>GS: Set TerragruntStackConfigPath (WorkingDir + defaultStackFile)
GS->>PS: Process stack configuration
PS->>WUV: For each unit, generate unit values file
WUV-->>PS: Unit values written
Note over CLI,RUV: During config parsing
CLI->>RUV: Read unit values from file
RUV-->>CLI: Return unit values
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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
CodeRabbit Configuration File (
|
![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 (4)
cli/commands/stack/generate.go (1)
82-85: Consider adding error context for better debugging.When generating the unit values file, consider wrapping the error with more context about which unit failed.
// generate unit values file if err := config.WriteUnitValues(opts, unit, dest); err != nil { - return errors.New(err) + return errors.New(fmt.Errorf("failed to generate values file for unit %s: %w", unit.Name, err)) }cli/commands/stack/output.go (1)
24-24: Consider extracting path construction to a shared function.This path construction logic is duplicated in both
generate.goandoutput.go. Consider extracting it to a shared function to maintain consistency.+// getDefaultStackConfigPath returns the default stack configuration path +func getDefaultStackConfigPath(workingDir string) string { + return filepath.Join(workingDir, defaultStackFile) +} func generateOutput(ctx context.Context, opts *options.TerragruntOptions) (map[string]map[string]cty.Value, error) { opts.Logger.Debugf("Generating output from %s", opts.TerragruntStackConfigPath) - opts.TerragruntStackConfigPath = filepath.Join(opts.WorkingDir, defaultStackFile) + opts.TerragruntStackConfigPath = getDefaultStackConfigPath(opts.WorkingDir)config/stack.go (2)
96-118: LGTM! Consider enhancing error handling.The function correctly handles the generation of unit values, with good nil checks and debug logging. However, we could improve the error handling by wrapping the file write error with more context.
- if err := os.WriteFile(filePath, file.Bytes(), defaultPerms); err != nil { - return err + if err := os.WriteFile(filePath, file.Bytes(), defaultPerms); err != nil { + return fmt.Errorf("failed to write unit values file %s: %w", filePath, err)
120-150: Maintain consistent error handling style.The function handles the reading of unit values well, but the error handling style varies between direct returns and wrapping with errors.New().
- return nil, errors.New(err) + return nil, fmt.Errorf("failed to parse unit values file %s: %w", filePath, err) - return nil, errors.New(err) + return nil, fmt.Errorf("failed to evaluate parsing context for %s: %w", filePath, err)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
cli/commands/stack/action.go(1 hunks)cli/commands/stack/generate.go(3 hunks)cli/commands/stack/output.go(2 hunks)config/config.go(2 hunks)config/config_helpers.go(1 hunks)config/config_partial.go(2 hunks)config/locals.go(1 hunks)config/parsing_context.go(2 hunks)config/stack.go(4 hunks)options/options.go(0 hunks)test/fixtures/stacks/unit-values/units/app/terragrunt.hcl(1 hunks)test/integration_stacks_test.go(1 hunks)
💤 Files with no reviewable changes (1)
- options/options.go
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build-and-test
- GitHub Check: Pull Request has non-contributor approval
🔇 Additional comments (10)
cli/commands/stack/action.go (1)
20-20: LGTM! Clear and descriptive constant name.The rename from
dirPermtodefaultPermsbetter reflects its purpose as the default permissions for directory creation.cli/commands/stack/generate.go (1)
25-25:❓ Verification inconclusive
Verify path construction across different OS environments.
While joining paths is correct, ensure this works correctly on Windows systems too.
🏁 Script executed:
#!/bin/bash # Check if the code handles Windows paths correctly ast-grep --pattern 'filepath.Join($_)'Length of output: 40
Attention: Please Verify Windows Environment Handling
The change using
filepath.Join(opts.WorkingDir, defaultStackFile)incli/commands/stack/generate.golooks correct since it relies on Go's standard library, which generally manages cross-platform path separators properly. However, the automated check (usingast-grep) didn’t yield explicit confirmation of Windows-specific behavior. I recommend manually verifying that this path construction works as intended on Windows—ideally by running tests in a Windows environment or using a simulator. This extra step will help confirm that error handling and OS-specific formatting are solid.config/parsing_context.go (2)
30-32: LGTM! Well-documented field addition.The new
Valuesfield is properly documented and follows the existing pattern of optional context values.
78-81: LGTM! Consistent method implementation.The
WithValuesmethod follows the established builder pattern used throughout the file.config/locals.go (1)
185-187: LGTM! Clean replacement of metadata handling.The switch from
MetadataUnittoMetadataValuesis clean and maintains consistency with the existing code structure.config/config_partial.go (1)
351-360: LGTM! Clean integration of unit values handling.The addition of unit values handling for the Stacks experiment is well-integrated with the existing parsing logic and maintains proper error handling.
config/config_helpers.go (1)
213-215: LGTM! Clean implementation of the values context.The change properly handles the new values structure while maintaining nil safety checks.
config/config.go (2)
45-45: LGTM! Clear and descriptive constant definition.The constant name and value are self-explanatory and follow the existing naming conventions.
897-906: LGTM! Well-structured integration of unit values.The code properly:
- Checks for the Stacks experiment flag
- Handles errors from ReadUnitValues
- Updates the context with the new values
test/fixtures/stacks/unit-values/units/app/terragrunt.hcl (1)
3-3: LGTM! Consistent update of value references.The changes correctly update the value references to use the new structure, maintaining consistency across both locals and inputs blocks.
Also applies to: 7-8
test/integration_stacks_test.go
Outdated
| func TestStacksUnitValuesRunInApp1(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| helpers.CleanupTerraformFolder(t, testFixtureStacksUnitValues) | ||
| tmpEnvPath := helpers.CopyEnvironment(t, testFixtureStacksUnitValues) | ||
| rootPath := util.JoinPath(tmpEnvPath, testFixtureStacksUnitValues) | ||
|
|
||
| helpers.RunTerragrunt(t, "terragrunt stack run apply --experiment stacks --terragrunt-non-interactive --terragrunt-working-dir "+rootPath) | ||
|
|
||
| // run apply in generated app1 directory | ||
| app1Path := util.JoinPath(rootPath, ".terragrunt-stack", "app1") | ||
| helpers.RunTerragrunt(t, "terragrunt apply --experiment stacks --terragrunt-non-interactive --terragrunt-working-dir "+app1Path) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add assertions to verify the test outcome.
While the test executes the stack commands correctly, it doesn't verify the actual outcome of the operations. Consider adding assertions to validate the expected state after the commands are run.
app1Path := util.JoinPath(rootPath, ".terragrunt-stack", "app1")
helpers.RunTerragrunt(t, "terragrunt apply --experiment stacks --terragrunt-non-interactive --terragrunt-working-dir "+app1Path)
+
+ // Verify the expected outcomes
+ valuesPath := filepath.Join(app1Path, "terragrunt.values.hcl")
+ assert.FileExists(t, valuesPath)
+
+ // Verify the values file content
+ content, err := os.ReadFile(valuesPath)
+ require.NoError(t, err)
+ assert.Contains(t, string(content), "deployment = \"app1\"")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func TestStacksUnitValuesRunInApp1(t *testing.T) { | |
| t.Parallel() | |
| helpers.CleanupTerraformFolder(t, testFixtureStacksUnitValues) | |
| tmpEnvPath := helpers.CopyEnvironment(t, testFixtureStacksUnitValues) | |
| rootPath := util.JoinPath(tmpEnvPath, testFixtureStacksUnitValues) | |
| helpers.RunTerragrunt(t, "terragrunt stack run apply --experiment stacks --terragrunt-non-interactive --terragrunt-working-dir "+rootPath) | |
| // run apply in generated app1 directory | |
| app1Path := util.JoinPath(rootPath, ".terragrunt-stack", "app1") | |
| helpers.RunTerragrunt(t, "terragrunt apply --experiment stacks --terragrunt-non-interactive --terragrunt-working-dir "+app1Path) | |
| } | |
| func TestStacksUnitValuesRunInApp1(t *testing.T) { | |
| t.Parallel() | |
| helpers.CleanupTerraformFolder(t, testFixtureStacksUnitValues) | |
| tmpEnvPath := helpers.CopyEnvironment(t, testFixtureStacksUnitValues) | |
| rootPath := util.JoinPath(tmpEnvPath, testFixtureStacksUnitValues) | |
| helpers.RunTerragrunt(t, "terragrunt stack run apply --experiment stacks --terragrunt-non-interactive --terragrunt-working-dir "+rootPath) | |
| // run apply in generated app1 directory | |
| app1Path := util.JoinPath(rootPath, ".terragrunt-stack", "app1") | |
| helpers.RunTerragrunt(t, "terragrunt apply --experiment stacks --terragrunt-non-interactive --terragrunt-working-dir "+app1Path) | |
| // Verify the expected outcomes | |
| valuesPath := filepath.Join(app1Path, "terragrunt.values.hcl") | |
| assert.FileExists(t, valuesPath) | |
| // Verify the values file content | |
| content, err := os.ReadFile(valuesPath) | |
| require.NoError(t, err) | |
| assert.Contains(t, string(content), "deployment = \"app1\"") | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
config/stack.go (3)
24-25: Consider using more restrictive file permissions.The
defaultPermsvalue of0755might be too permissive for a values file that could contain sensitive information. Consider using0600(user read/write only) or0640(user read/write, group read only) instead.- defaultPerms = 0755 + defaultPerms = 0600
116-118: Minor style tweak: Add line break before range.There's a minor style issue with the cuddle assignment. Let's add a line break before the range statement to match Go's style conventions.
}) + for key, val := range unit.Values.AsValueMap() { body.SetAttributeValue(key, val) }🧰 Tools
🪛 golangci-lint (1.62.2)
116-116: only one cuddle assignment allowed before range statement
(wsl)
127-157: Add input validation for directory path.The function looks solid overall, but let's add some input validation to ensure the directory path is valid.
func ReadUnitValues(ctx context.Context, opts *options.TerragruntOptions, unitDirectory string) (*cty.Value, error) { + if unitDirectory == "" { + return nil, fmt.Errorf("unit directory path cannot be empty") + } + filePath := filepath.Join(unitDirectory, unitValuesFile)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
config/stack.go(4 hunks)
🧰 Additional context used
🪛 golangci-lint (1.62.2)
config/stack.go
116-116: only one cuddle assignment allowed before range statement
(wsl)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build-and-test
- GitHub Check: Pull Request has non-contributor approval
🔇 Additional comments (2)
config/stack.go (2)
98-125: Well-structured implementation with clear error handling!The function is well-implemented with:
- Early return for nil values
- Clear debug logging
- Helpful auto-generated comment in the file
- Proper error handling
🧰 Tools
🪛 golangci-lint (1.62.2)
116-116: only one cuddle assignment allowed before range statement
(wsl)
170-171: Nice optimization with map pre-allocation! 👍Pre-allocating the maps with the known capacity is a good practice that helps avoid unnecessary map growth and rehashing.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
config/stack.go (2)
98-126: Consider adding directory existence check.The function looks solid overall, but we might want to ensure the target directory exists before writing the file. This could prevent cryptic errors if the directory structure isn't ready.
func WriteUnitValues(opts *options.TerragruntOptions, unit *Unit, unitDirectory string) error { + if err := os.MkdirAll(unitDirectory, os.ModePerm); err != nil { + return fmt.Errorf("failed to create directory %s: %w", unitDirectory, err) + } filePath := filepath.Join(unitDirectory, unitValuesFile)
128-163: Enhance error handling and context propagation.The function is well-structured, but we could make a couple of improvements:
- More descriptive error messages would help with debugging
- The
//nolint:contextcheckcomment suggests we should pass the context tocreateTerragruntEvalContextfunc ReadUnitValues(ctx context.Context, opts *options.TerragruntOptions, unitDirectory string) (*cty.Value, error) { if unitDirectory == "" { - return nil, errors.New("unit directory path cannot be empty") + return nil, errors.New("ReadUnitValues: unit directory path cannot be empty") } // ... - //nolint:contextcheck - evalParsingContext, err := createTerragruntEvalContext(parser, file.ConfigPath) + evalParsingContext, err := createTerragruntEvalContext(ctx, parser, file.ConfigPath)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
config/stack.go(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Pull Request has non-contributor approval
🔇 Additional comments (2)
config/stack.go (2)
6-14: LGTM! Good security practice with file permissions.The new imports are well-organized, and setting file permissions to 0600 ensures that only the owner can read/write the values file. 👍
Also applies to: 22-26
176-177: Nice optimization! 🚀Pre-allocating the maps with the known capacity is a good performance optimization, especially for larger stacks.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
config/stack.go (3)
22-26: Consider using more restrictive file permissions.The
defaultPermsvalue of0600provides good security by restricting access to the owner only. However, since this file contains generated values that shouldn't be modified manually, consider using0400(read-only) to prevent accidental modifications.- defaultPerms = 0600 + defaultPerms = 0400
98-134: Add file existence check before writing.The function creates directories but doesn't check if the file already exists. This could lead to unintended overwrites. Consider adding a check and maybe a backup mechanism.
filePath := filepath.Join(unitDirectory, unitValuesFile) + if util.FileExists(filePath) { + opts.Logger.Debugf("Values file already exists at %s, creating backup", filePath) + if err := util.CopyFile(filePath, filePath+".bak"); err != nil { + return errors.Errorf("failed to create backup of %s: %w", filePath, err) + } + } + if unit.Values == nil {
136-171: Add file permission validation.The function reads the values file but doesn't verify its permissions. Since this file contains sensitive configuration, it's good practice to ensure it hasn't been made world-readable.
if util.FileNotExists(filePath) { return nil, nil } + + // Verify file permissions + fileInfo, err := os.Stat(filePath) + if err != nil { + return nil, errors.Errorf("failed to stat %s: %w", filePath, err) + } + if fileInfo.Mode().Perm() > 0600 { + opts.Logger.Warnf("Values file %s has loose permissions: %o", filePath, fileInfo.Mode().Perm()) + } opts.Logger.Debugf("Reading Terragrunt stack values file at %s", filePath)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
config/stack.go(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: build-and-test
- GitHub Check: unessential
- GitHub Check: Pull Request has non-contributor approval
🔇 Additional comments (1)
config/stack.go (1)
184-185: Nice optimization! 👍Pre-allocating the map capacity with
make(map[string]bool, len(config.Units))is a great performance improvement, especially for large stacks.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
config/stack.go (1)
97-133: Consider using more restrictive file permissions.The function looks good overall, but using
os.ModePerm(777) for both directory and file creation might be too permissive. Consider using more restrictive permissions like 0755 for directories and 0644 for files.- if err := os.MkdirAll(unitDirectory, os.ModePerm); err != nil { + if err := os.MkdirAll(unitDirectory, 0755); err != nil { return errors.Errorf("failed to create directory %s: %w", unitDirectory, err) } // ... - if err := os.WriteFile(filePath, file.Bytes(), os.ModePerm); err != nil { + if err := os.WriteFile(filePath, file.Bytes(), 0644); err != nil { return err }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
cli/commands/stack/action.go(0 hunks)cli/commands/stack/generate.go(3 hunks)config/stack.go(4 hunks)
💤 Files with no reviewable changes (1)
- cli/commands/stack/action.go
🚧 Files skipped from review as they are similar to previous changes (1)
- cli/commands/stack/generate.go
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Pull Request has non-contributor approval
- GitHub Check: build-and-test
🔇 Additional comments (4)
config/stack.go (4)
6-14: LGTM! Clean import organization and constant definition.The new imports and constant are well-organized and follow the project's conventions.
Also applies to: 22-25
135-170: LGTM! Well-structured file reading implementation.The function has solid error handling and follows the established patterns in the codebase. Nice touch returning
nilwhen the file doesn't exist instead of throwing an error! 👍
183-184: LGTM! Nice performance optimization.Pre-allocating the maps with the exact capacity needed is a great optimization that avoids unnecessary map resizing. 🚀
1-279: Add tests for the new value generation functionality.The implementation looks solid, but we should ensure it's well-tested. Consider adding tests for:
- Writing values with different types (strings, numbers, maps, etc.)
- Reading values from valid and invalid files
- Edge cases like empty values or missing directories
Would you like me to help create these test cases?
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
config/stack.go (2)
99-135: Enhance error handling and file header comment.The function looks solid! A couple of suggestions to make it even better:
- Wrap the os.WriteFile error with more context
- Add version info to the auto-generated comment
Here's how we could improve it:
if err := os.WriteFile(filePath, file.Bytes(), valueFilePerm); err != nil { - return err + return errors.Errorf("failed to write values file %s: %w", filePath, err) } body.AppendUnstructuredTokens([]*hclwrite.Token{ { Type: hclsyntax.TokenComment, - Bytes: []byte("# Auto-generated by the terragrunt.stack.hcl file by Terragrunt. Do not edit manually\n"), + Bytes: []byte("# Auto-generated by Terragrunt v0.x.x from terragrunt.stack.hcl\n# DO NOT EDIT - any changes will be overwritten\n"), }, })
137-172: Add more context to debug logs.The function looks great! One small suggestion to make debugging easier:
-opts.Logger.Debugf("Reading Terragrunt stack values file at %s", filePath) +opts.Logger.Debugf("Reading Terragrunt stack values file for unit at %s", filePath)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
config/stack.go(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Pull Request has non-contributor approval
- GitHub Check: build-and-test
🔇 Additional comments (2)
config/stack.go (2)
6-14: LGTM! Clean imports and well-defined constants.The new imports are properly organized, and the permission constants follow standard Unix conventions (0755 for directories, 0644 for files).
Also applies to: 22-27
185-186: Nice optimization! 👍Pre-allocating the maps with the known capacity is a good performance optimization.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
config/stack.go (4)
99-100: Enhance function documentation.The function comment could be more descriptive. Consider adding details about the purpose, parameters, and return values.
-// WriteUnitValues generate unit values to the terragrunt.stack.hcl file. +// WriteUnitValues generates and writes unit values to a terragrunt.values.hcl file in the specified unit directory. +// If the unit has no values (Values is nil), the function logs a debug message and returns. +// Parameters: +// - opts: TerragruntOptions containing logger and other configuration +// - unit: Unit containing the values to write +// - unitDirectory: Target directory where the values file will be created +// Returns an error if the directory creation or file writing fails.
100-135: Consider additional input validation.While the function handles empty directory paths well, it might be good to add validation for the
optsandunitparameters.func WriteUnitValues(opts *options.TerragruntOptions, unit *Unit, unitDirectory string) error { + if opts == nil { + return errors.New("WriteUnitValues: options cannot be nil") + } + if unit == nil { + return errors.New("WriteUnitValues: unit cannot be nil") + } if unitDirectory == "" { return errors.New("WriteUnitValues: unit directory path cannot be empty") }
137-172: Make error wrapping consistent.The function has good error handling, but let's make the error wrapping consistent throughout.
if err := file.Decode(&values, evalParsingContext); err != nil { - return nil, errors.New(err) + return nil, errors.Errorf("failed to decode values file %s: %w", filePath, err) }
185-186: Nice performance optimization!Pre-allocating maps with the expected capacity is a good practice to avoid resizing. Consider adding a brief comment to explain this optimization.
+ // Pre-allocate maps with known capacity to avoid resizing names := make(map[string]bool, len(config.Units)) paths := make(map[string]bool, len(config.Units))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
config/stack.go(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build-and-test
- GitHub Check: Pull Request has non-contributor approval
🔇 Additional comments (1)
config/stack.go (1)
6-14: Clean and well-organized additions!The new imports and constants are well-structured and follow Go best practices. The permission constants use standard octal notation (0755, 0644) which aligns with common Unix file permissions.
Also applies to: 22-27
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
config/stack.go (1)
144-179: Consider enhancing error messages for better debugging.The function looks solid! To make debugging easier, we could make the error messages more descriptive.
func ReadUnitValues(ctx context.Context, opts *options.TerragruntOptions, unitDirectory string) (*cty.Value, error) { // ... existing code ... file, err := hclparse.NewParser(parser.ParserOptions...).ParseFromFile(filePath) if err != nil { - return nil, errors.New(err) + return nil, errors.Errorf("failed to parse values file %s: %w", filePath, err) } // ... rest of the code ... if err := file.Decode(&values, evalParsingContext); err != nil { - return nil, errors.New(err) + return nil, errors.Errorf("failed to decode values from %s: %w", filePath, err) } // ... rest of the code ... }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
config/stack.go(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build-and-test
- GitHub Check: Pull Request has non-contributor approval
🔇 Additional comments (3)
config/stack.go (3)
6-14: Nice work on the imports and constants! 👍The new imports are well-organized, and the permission constants follow standard Unix conventions (0755 for directories, 0644 for files).
Also applies to: 22-27
192-194: Smart optimization with map pre-allocation! 🚀Pre-allocating maps with the known capacity is a nice performance touch that avoids unnecessary map resizing.
99-142:❓ Verification inconclusive
Great implementation of WriteUnitValues! Just one safety consideration.
The function is well-structured with proper error handling and permissions. However, we might want to consider file overwrite safety.
Consider adding a check to prevent accidental overwrites of existing files:
func WriteUnitValues(opts *options.TerragruntOptions, unit *Unit, unitDirectory string) error { // ... existing code ... filePath := filepath.Join(unitDirectory, unitValuesFile) + if util.FileExists(filePath) { + opts.Logger.Debugf("Values file already exists at %s, will be overwritten", filePath) + } // ... rest of the code ... }Let's verify if there are any existing uses of this file that might be affected:
🏁 Script executed:
#!/bin/bash # Search for any references to terragrunt.values.hcl rg -l "terragrunt\.values\.hcl"Length of output: 98
I'll run a quick search in the test file to see if there are any tests addressing file overwrite handling. This can help us determine if the file overwrite safety check suggestion might influence existing behavior.
Once we see the results, I'll factor that into our final suggestion.
🏁 Script executed:
#!/bin/bash # Searching for usage of the WriteUnitValues function in the test file rg -n "WriteUnitValues" test/integration_stacks_test.goLength of output: 55
Heads up: Consider handling file overwrites in WriteUnitValues.
The implementation is solid overall—with good error handling and clear logging—and I see that the file is referenced in a few locations. However, our investigation shows that the WriteUnitValues function isn’t directly tested in the integration tests, so we can’t be 100% sure if overwriting an existing file is intended or if it might cause unexpected issues later in the execution flow.
I recommend adding a check to notify (or even optionally prevent) overwriting an existing file. For example:
func WriteUnitValues(opts *options.TerragruntOptions, unit *Unit, unitDirectory string) error { // ... existing code ... filePath := filepath.Join(unitDirectory, unitValuesFile) + if util.FileExists(filePath) { + opts.Logger.Debugf("Values file already exists at %s, will be overwritten", filePath) + } // ... rest of the code ... }Please manually verify that this behavior (overwriting the file) fits your use case. If you want to safeguard against accidental overwrites further, consider implementing a mechanism to abort or back up the file before writing.
Description
terragrunt.values.hclduring stack generateterragrunt.values.hclasvaluesduring executionunit.valueswithvaluesDemo:
Fixes #3911.
TODOs
Read the Gruntwork contribution guidelines.
Release Notes (draft)
Added / Removed / Updated [X].
Migration Guide
Variable
unit.valueswas deprecated and replaced withvalues, usevalues.*to reference unit values.Summary by CodeRabbit
New Features
Refactor
Tests