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.
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