chore: Bumping boilerplate #4805
Conversation
📝 WalkthroughWalkthroughThe PR consolidates scaffold-related flag definitions into a shared module, introduces NoShell and NoHooks security options for both catalog and scaffold commands with CLI-flag-takes-precedence semantics, and refactors scaffold implementation to thread TerragruntOptions throughout helper functions while maintaining backward compatibility. Changes
Sequence DiagramsequenceDiagram
participant CLI as CLI Command
participant Run as scaffold.Run()
participant CatalogMerge as applyCatalogConfigToScaffold()
participant Opts as TerragruntOptions
participant BoilerplateOpts as BoilerplateOptions
CLI->>Run: scaffold module-url<br/>(with --no-shell, --no-hooks)
activate Run
Note over Run: TerragruntOptions flags set<br/>from CLI args
Run->>CatalogMerge: apply catalog config
activate CatalogMerge
Note over CatalogMerge: Read catalog.no_shell,<br/>catalog.no_hooks
alt CLI flags already set
CatalogMerge->>Opts: Skip (CLI takes precedence)
else CLI flags not set (default false)
CatalogMerge->>Opts: Apply catalog config values
end
deactivate CatalogMerge
Run->>BoilerplateOpts: NewBoilerplateOptions()<br/>with Opts.NoShell, Opts.NoHooks
activate BoilerplateOpts
Note over BoilerplateOpts: Final config with<br/>precedence resolved
deactivate BoilerplateOpts
Note over Run: Process template with<br/>shell/hooks disabled if needed
deactivate Run
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes The changes span multiple architectural layers (flags, config, options, implementation) with consistent patterns (threading opts parameter across functions). While individual changes are straightforward, the breadth across many files and the need to verify correct precedence logic (CLI > catalog > defaults) and proper opts threading throughout the call chain requires careful, deliberate review. Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (3 warnings)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
yhakbar
force-pushed
the
chore/bumping-boilerplate
branch
from
8b21d06 to
8e0c762
Compare
yhakbar
force-pushed
the
chore/bumping-boilerplate
branch
from
8e0c762 to
15d5662
Compare
yhakbar
force-pushed
the
chore/bumping-boilerplate
branch
from
15d5662 to
7f7ef5a
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (9)
cli/flags/shared/shared.go (1)
179-214: Redundant assignment in RootFileNameFlagName Action.The
Destination: &opts.ScaffoldRootFileNameon line 187 already handles assigning the flag value to the field. The manual assignment on line 195 (opts.ScaffoldRootFileName = value) is redundant since Actions are executed after the Destination assignment.Apply this diff to remove the redundant assignment:
flags.NewFlag(&cli.GenericFlag[string]{ Name: RootFileNameFlagName, EnvVars: tgPrefix.EnvVars(RootFileNameFlagName), Destination: &opts.ScaffoldRootFileName, Usage: "Name of the root Terragrunt configuration file, if used.", Action: func(ctx *cli.Context, value string) error { if value == "" { return cli.NewExitError("root-file-name flag cannot be empty", cli.ExitCodeGeneralError) } if value != opts.TerragruntConfigPath { - opts.ScaffoldRootFileName = value - return nil } if err := opts.StrictControls.FilterByNames("RootTerragruntHCL").Evaluate(ctx); err != nil { return cli.NewExitError(err, cli.ExitCodeGeneralError) } return nil }, }),docs-starlight/src/content/docs/03-features/06-catalog.mdx (1)
52-53: Optional: Consider rewording for variety.The phrasing "organizations that want to prevent the use of" is repeated on consecutive lines. While clear, you could vary the wording slightly for better readability:
-- `no_shell` (bool): When set to `true`, disables shell command execution in boilerplate templates during scaffolding. This is useful for organizations that want to prevent the use of shell commands in templates for security reasons. -- `no_hooks` (bool): When set to `true`, disables hook execution in boilerplate templates during scaffolding. This is useful for organizations that want to prevent the use of hooks in templates for security reasons. +- `no_shell` (bool): When set to `true`, disables shell command execution in boilerplate templates during scaffolding. This is useful for organizations that want to prevent the execution of shell commands in templates for security reasons. +- `no_hooks` (bool): When set to `true`, disables hook execution in boilerplate templates during scaffolding. This is useful when you need to restrict the execution of hooks in templates for security reasons.cli/commands/scaffold/scaffold_test.go (5)
109-361: Precedence test emulates logic locally; not exercising production code.TestCatalogConfigApplication duplicates precedence logic instead of calling the real resolver (applyCatalogConfigToScaffold). This can drift from implementation and currently masks the CLI-false vs config-true issue.
Prefer one of:
- Move this test into package scaffold (not scaffold_test) and call applyCatalogConfigToScaffold directly, or
- Export a small resolver helper to test, or
- Drive via scaffold.Run with a minimal local template to verify end-to-end.
Also, once you track “flag set” state (see code comment in scaffold.go), validate both directions:
- CLI true overrides config false
- CLI false overrides config true (when explicitly set)
I can draft the refactor and tests if helpful.
443-500: Shell template disabled: good assertions; consider Windows skip.TestBoilerplateShellTemplateFunctionDisabled is correct. However, the template engine may invoke shell binary resolution differently on Windows. Add a Windows skip guard to avoid flaky runs.
func TestBoilerplateShellTemplateFunctionDisabled(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("shell function semantics differ on Windows") + } t.Parallel() ... }And import
runtime. Apply similarly to other shell/hook tests.
557-623: Hooks disabled: relies ontouch; add cross-platform guard.The before/after hooks use
touch, which isn’t present on Windows. Either:
- Wrap commands in
sh -cand skip on Windows, or- Guard the test with
runtime.GOOS != "windows".This will reduce CI flakes.
625-691: Hooks enabled: same portability concern.Apply the same Windows skip or shell wrapping here.
693-755: Both flags disabled: solid coverage; add Windows skip if needed.Same rationale as above.
test/integration_scaffold_test.go (2)
178-206: Shell enabled integration test: OK; consider end-to-end precedence case.Looks good. To fully validate precedence, add a companion test where catalog sets
no_shell = trueand CLI explicitly sets--no-shell=false(once flag-set tracking is implemented) to verify CLI wins.
317-357: Catalog integration: good; add Windows guard if fixtures rely on POSIX tools.If the catalog template or hooks rely on POSIX shell, guard with
runtime.GOOSor provide platform-neutral fixtures.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (28)
cli/commands/catalog/cli.go(2 hunks)cli/commands/scaffold/cli.go(3 hunks)cli/commands/scaffold/scaffold.go(10 hunks)cli/commands/scaffold/scaffold_test.go(4 hunks)cli/flags/shared/shared.go(2 hunks)config/catalog.go(1 hunks)config/config.go(1 hunks)docs-starlight/src/content/docs/03-features/06-catalog.mdx(3 hunks)docs-starlight/src/data/commands/catalog.mdx(1 hunks)docs-starlight/src/data/commands/scaffold.mdx(1 hunks)docs-starlight/src/data/flags/catalog-no-hooks.mdx(1 hunks)docs-starlight/src/data/flags/catalog-no-shell.mdx(1 hunks)docs-starlight/src/data/flags/scaffold-no-hooks.mdx(1 hunks)docs-starlight/src/data/flags/scaffold-no-shell.mdx(1 hunks)go.mod(16 hunks)options/options.go(2 hunks)test/fixtures/scaffold/catalog-config-test/terragrunt.hcl(1 hunks)test/fixtures/scaffold/with-hooks/.boilerplate/boilerplate.yml(1 hunks)test/fixtures/scaffold/with-hooks/.boilerplate/terragrunt.hcl(1 hunks)test/fixtures/scaffold/with-hooks/main.tf(1 hunks)test/fixtures/scaffold/with-shell-and-hooks/.boilerplate/boilerplate.yml(1 hunks)test/fixtures/scaffold/with-shell-and-hooks/.boilerplate/terragrunt.hcl(1 hunks)test/fixtures/scaffold/with-shell-and-hooks/main.tf(1 hunks)test/fixtures/scaffold/with-shell-commands/.boilerplate/boilerplate.yml(1 hunks)test/fixtures/scaffold/with-shell-commands/.boilerplate/terragrunt.hcl(1 hunks)test/fixtures/scaffold/with-shell-commands/main.tf(1 hunks)test/integration_parse_test.go(1 hunks)test/integration_scaffold_test.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.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:
test/integration_parse_test.gooptions/options.gocli/commands/catalog/cli.gocli/commands/scaffold/cli.goconfig/catalog.goconfig/config.gocli/commands/scaffold/scaffold_test.gocli/commands/scaffold/scaffold.gotest/integration_scaffold_test.gocli/flags/shared/shared.go
docs-starlight/**/*.md*
⚙️ CodeRabbit configuration file
Review the documentation for clarity, grammar, and spelling. Make sure that the documentation is easy to understand and follow. There is currently a migration underway from the Jekyll based documentation in
docsto the Starlight + Astro based documentation indocs-starlight. Make sure that thedocs-starlightdocumentation is accurate and up-to-date with thedocsdocumentation, and that any difference between them results in an improvement in thedocs-starlightdocumentation.
Files:
docs-starlight/src/data/flags/scaffold-no-hooks.mdxdocs-starlight/src/data/flags/scaffold-no-shell.mdxdocs-starlight/src/content/docs/03-features/06-catalog.mdxdocs-starlight/src/data/flags/catalog-no-shell.mdxdocs-starlight/src/data/commands/catalog.mdxdocs-starlight/src/data/commands/scaffold.mdxdocs-starlight/src/data/flags/catalog-no-hooks.mdx
🧠 Learnings (2)
📚 Learning: 2025-02-10T13:36:19.542Z
Learnt from: levkohimins
PR: gruntwork-io/terragrunt#3723
File: cli/commands/stack/action.go:160-160
Timestamp: 2025-02-10T13:36:19.542Z
Learning: The project uses a custom error package `github.com/gruntwork-io/terragrunt/internal/errors` which provides similar functionality to `fmt.Errorf` but includes stack traces. Prefer using this package's error functions (e.g., `errors.Errorf`, `errors.New`) over the standard library's error handling.
Applied to files:
cli/commands/scaffold/cli.go
📚 Learning: 2025-04-17T13:02:28.098Z
Learnt from: yhakbar
PR: gruntwork-io/terragrunt#4169
File: cli/commands/hcl/validate/cli.go:29-60
Timestamp: 2025-04-17T13:02:28.098Z
Learning: Avoid shadowing imported packages with local variables in Go code, such as using a variable named `flags` when the `github.com/gruntwork-io/terragrunt/cli/flags` package is imported. Use more specific variable names like `flagSet` instead.
Applied to files:
cli/commands/scaffold/cli.gocli/commands/scaffold/scaffold.gocli/flags/shared/shared.go
🧬 Code graph analysis (6)
cli/commands/catalog/cli.go (1)
cli/flags/shared/shared.go (1)
NewScaffoldingFlags(180-229)
cli/commands/scaffold/cli.go (3)
cli/commands/catalog/cli.go (1)
NewFlags(18-20)options/options.go (1)
TerragruntOptions(99-335)cli/flags/shared/shared.go (1)
NewScaffoldingFlags(180-229)
cli/commands/scaffold/scaffold_test.go (5)
cli/commands/scaffold/scaffold.go (1)
Run(131-248)util/file.go (3)
JoinPath(626-628)FileExists(61-64)ReadFileAsString(369-376)options/options.go (1)
NewTerragruntOptions(386-388)test/helpers/logger/logger.go (1)
CreateLogger(9-14)config/catalog.go (1)
ReadCatalogConfig(83-102)
cli/commands/scaffold/scaffold.go (3)
options/options.go (1)
TerragruntOptions(99-335)cli/flags/shared/shared.go (1)
NoIncludeRootFlagName(33-33)config/catalog.go (1)
ReadCatalogConfig(83-102)
test/integration_scaffold_test.go (2)
test/helpers/package.go (1)
RunTerragruntCommandWithOutput(1004-1008)util/file.go (1)
ReadFileAsString(369-376)
cli/flags/shared/shared.go (4)
options/options.go (1)
TerragruntOptions(99-335)internal/cli/generic_flag.go (1)
GenericFlag(19-48)internal/cli/errors.go (1)
NewExitError(53-69)internal/cli/bool_flag.go (1)
BoolFlag(13-49)
🪛 LanguageTool
docs-starlight/src/content/docs/03-features/06-catalog.mdx
[style] ~53-~53: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: .... This is useful for organizations that want to prevent the use of hooks in templates f...
(REP_WANT_TO_VB)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: integration_tests / Test (Fixtures with Latest OSS Terraform)
- GitHub Check: integration_tests / Test (Fixtures with Latest OSS Terraform)
- GitHub Check: integration_tests / Test (Fixtures with Latest OSS Terraform)
- GitHub Check: integration_tests / Test (Fixtures with Latest OSS Terraform)
- GitHub Check: integration_tests / Test (Fixtures with Latest OSS Terraform)
- GitHub Check: integration_tests / Test (Fixtures with Latest OSS Terraform)
- GitHub Check: integration_tests / Test (Fixtures with Latest OSS Terraform)
- GitHub Check: integration_tests / Test (Fixtures with Latest OSS Terraform)
- GitHub Check: integration_tests / Test (Fixtures with Latest OSS Terraform)
- GitHub Check: integration_tests / Test (Fixtures with Latest OSS Terraform)
🔇 Additional comments (36)
go.mod (3)
84-89: AWS SDK coordinated updates look reasonable.The AWS SDK main package and service modules (config, credentials, s3, sts) are bumped consistently. Patch and minor version bumps for these stable packages carry low risk, but confirm the service SDK versions align with the main package expectations.
10-10: Routine dependency maintenance updates approved.Patch and minor version bumps for google.golang.org/api, golang.org/x/exp, and AWS SDK are standard maintenance. These appear routine unless there are known compatibility gaps with the codebase.
Also applies to: 77-77, 99-99
23-23: Boilerplate v0.10.1 bump is safe and fully supported.The web search confirms boilerplate v0.10.1 has no breaking changes and adds GitHub Actions workflows migration and a RenderVariablesWithContext function. The PR implementation is complete:
- NoShell and NoHooks flags are fully implemented in
options/options.go,config/catalog.go, andcli/commands/scaffold/scaffold.go, with flags passed directly to boilerplate's BoilerplateOptions.- Test coverage is comprehensive: config parsing, CLI flag override behavior, optional attribute handling, and actual boilerplate behavior with these flags enabled/disabled are all tested in
cli/commands/scaffold/scaffold_test.go.- Backward compatibility is maintained: both flags are optional in config and default to false.
The version bump is appropriate for the scaffolding feature additions in this PR.
options/options.go (2)
331-334: LGTM! Security flags added correctly.The NoShell and NoHooks fields are well-documented and appropriately positioned in the struct. The comments clearly explain their purpose for disabling shell commands and hooks in boilerplate templates.
443-444: LGTM! Proper initialization with secure defaults.Both NoShell and NoHooks are correctly initialized to
false, which maintains backward compatibility by keeping shell and hooks enabled by default. Users must explicitly opt into the security restrictions.cli/flags/shared/shared.go (2)
31-35: LGTM! Scaffolding flag constants follow established naming conventions.The new scaffolding-related constants are well-named and grouped appropriately with clear comments.
215-229: LGTM! NoShell and NoHooks flags properly configured.The NoShellFlagName and NoHooksFlagName flags are correctly wired to their respective TerragruntOptions fields with clear usage descriptions and environment variable support.
docs-starlight/src/data/commands/catalog.mdx (1)
20-21: LGTM! Documentation updated with new security flags.The catalog-no-shell and catalog-no-hooks flags are correctly added to the flags list.
test/fixtures/scaffold/with-shell-commands/.boilerplate/boilerplate.yml (1)
1-5: LGTM! Well-structured test fixture.The boilerplate definition is clear and properly formatted for testing scaffolding functionality with variables.
test/fixtures/scaffold/with-shell-and-hooks/main.tf (1)
1-21: LGTM! Well-designed test fixture for shell and hooks testing.The Terraform configuration correctly defines variables to capture shell command outputs and corresponding outputs for verification. The structure is clean and suitable for testing the NoShell and NoHooks functionality.
test/fixtures/scaffold/catalog-config-test/terragrunt.hcl (1)
1-6: LGTM! Appropriate test fixture for catalog config testing.The catalog configuration correctly sets no_shell and no_hooks to test the security flag behavior with catalog-driven defaults. The relative path reference is appropriate for the test fixture structure.
docs-starlight/src/data/commands/scaffold.mdx (1)
15-16: LGTM! Documentation updated with new security flags.The scaffold-no-shell and scaffold-no-hooks flags are correctly added to the flags list, consistent with the catalog command documentation.
docs-starlight/src/data/flags/catalog-no-shell.mdx (1)
1-28: LGTM! Comprehensive and well-structured documentation.The documentation clearly explains the security purpose of the no-shell flag with appropriate warnings and examples. The caution about untrusted templates and the note about non-interactive mode are valuable additions for users.
test/integration_parse_test.go (1)
32-33: LGTM! Correctly excluding template files from parsing.The template files contain Go template syntax (e.g.,
{{ shell "..." }}) that isn't valid HCL before template processing, so excluding them from the parse test is appropriate.test/fixtures/scaffold/with-shell-and-hooks/.boilerplate/boilerplate.yml (1)
1-17: LGTM! Well-structured test fixture.The boilerplate configuration correctly defines a test variable and lifecycle hooks for exercising the scaffolding functionality with the new NoHooks flag.
test/fixtures/scaffold/with-hooks/.boilerplate/terragrunt.hcl (1)
1-8: LGTM! Valid template structure.The template correctly uses Go template syntax and will produce valid HCL after processing. The test fixture appropriately exercises hook functionality.
cli/commands/catalog/cli.go (1)
8-19: LGTM! Excellent refactoring to consolidate flag definitions.Using
shared.NewScaffoldingFlagseliminates duplication and ensures consistent flag behavior across catalog and scaffold commands while maintaining the same public API.test/fixtures/scaffold/with-shell-commands/.boilerplate/terragrunt.hcl (1)
1-10: LGTM! Template correctly exercises shell functionality.The Go template syntax
{{ shell "echo" "-n" "VALUE" }}is correct and will be processed before HCL parsing. The nested quotes are valid template syntax and are appropriately handled by marking this file in knownBadFiles.test/fixtures/scaffold/with-shell-and-hooks/.boilerplate/terragrunt.hcl (1)
1-10: LGTM! Correctly combines shell and hook functionality.The template appropriately tests both shell commands and hooks with valid Go template syntax. The file is properly excluded from direct HCL parsing via knownBadFiles.
docs-starlight/src/data/flags/scaffold-no-hooks.mdx (1)
1-28: LGTM! Clear and comprehensive documentation.The documentation effectively explains the security benefits of the
--no-hooksflag, includes appropriate warnings about untrusted templates, and provides a helpful usage example.config/catalog.go (2)
42-43: LGTM! Properly structured optional configuration fields.Using
*boolallows for tri-state logic (true/false/unset), which is appropriate for configuration that can be overridden by CLI flags or left unset to use defaults.
49-49: LGTM! String() method correctly updated.The verbose representation now includes the new NoShell and NoHooks fields, which will be helpful for debugging and logging.
test/fixtures/scaffold/with-hooks/main.tf (1)
1-11: LGTM!This test fixture provides a clean, minimal Terraform configuration for testing hooks functionality. The variable and output definitions are correct and appropriate for the test scenarios.
test/fixtures/scaffold/with-hooks/.boilerplate/boilerplate.yml (1)
1-17: LGTM!This boilerplate configuration is well-designed for testing hook execution. The before and after hooks with echo commands provide clear signals for verifying hook behavior during scaffold operations.
docs-starlight/src/data/flags/catalog-no-hooks.mdx (1)
1-28: LGTM!The documentation is comprehensive and well-structured. It clearly explains:
- The flag's purpose and security implications
- Important security warnings via Aside blocks
- Non-interactive mode behavior
- Example usage
The documentation follows the established pattern for flag documentation files.
docs-starlight/src/data/flags/scaffold-no-shell.mdx (1)
1-28: LGTM!The documentation effectively communicates the flag's purpose and security considerations. The structure mirrors other flag documentation, maintaining consistency across the docs.
config/config.go (1)
581-588: LGTM!The implementation correctly extends the catalog block serialization to include
no_shellandno_hooksattributes. The code follows the established pattern with proper nil checks and consistent use of the catalogAsCty conversion.cli/commands/scaffold/cli.go (1)
28-65: LGTM!Excellent refactoring that consolidates shared scaffolding flags into a shared module. The implementation:
- Eliminates code duplication by delegating to
shared.NewScaffoldingFlags- Cleanly appends scaffold-specific flags
- Maintains backward compatibility
- Follows the DRY principle
This aligns well with the parallel changes in the catalog command, providing consistent flag behavior across both commands.
docs-starlight/src/content/docs/03-features/06-catalog.mdx (1)
48-71: Excellent documentation of the new security features.The new "Security Configuration" section effectively communicates:
- Purpose and usage of
no_shellandno_hooksoptions- Security considerations with appropriate warnings
- Configuration precedence (CLI flags > catalog configuration > defaults)
- Clear examples for both catalog and scaffold commands
The placement before "Custom templates for scaffolding" is logical, as security should be addressed before discussing customization.
test/fixtures/scaffold/with-shell-commands/main.tf (1)
1-21: LGTM!This test fixture appropriately supports testing of shell command execution during scaffolding. The dual variables and outputs allow verification of multiple shell command results, which is useful for comprehensive test coverage.
cli/commands/scaffold/scaffold.go (1)
110-129: Constructor looks good; threads security flags and non-interactive correctly.No issues spotted; matches boilerplate options usage and keeps defaults centralized.
cli/commands/scaffold/scaffold_test.go (1)
22-36: Helper is solid; centralizes default BoilerplateOptions for tests.Good defaults (OnMissingKey/Config, NonInteractive). No changes needed.
test/integration_scaffold_test.go (4)
207-234: Shell disabled integration test: OK.Assertions are clear and resilient.
235-259: Hooks enabled: OK.Validates output minimally; good signal.
261-286: Hooks disabled: OK.Validates that generation remains valid.
287-315: Both flags disabled: OK.Covers combined path.
| if _, found := vars[rootFileName]; !found { | ||
| vars[rootFileName] = opts.ScaffoldRootFileName | ||
| } else { | ||
| l.Warnf("The %s variable is already set in the var flag(s). The --%s flag will be ignored.", rootFileName, NoIncludeRootFlagName) | ||
| l.Warnf( | ||
| "The %s variable is already set in the var flag(s). The --%s flag will be ignored.", | ||
| rootFileName, | ||
| shared.NoIncludeRootFlagName, | ||
| ) | ||
| } |
There was a problem hiding this comment.
Wrong flag name in warning for RootFileName.
The warning for RootFileName uses shared.NoIncludeRootFlagName. It should reference the root-file-name flag. Replace with the correct constant.
- l.Warnf(
- "The %s variable is already set in the var flag(s). The --%s flag will be ignored.",
- rootFileName,
- shared.NoIncludeRootFlagName,
- )
+ l.Warnf(
+ "The %s variable is already set in the var flag(s). The --%s flag will be ignored.",
+ rootFileName,
+ shared.RootFileNameFlagName, // define if missing
+ )If a dedicated constant doesn’t exist yet, add one alongside NoIncludeRootFlagName.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In cli/commands/scaffold/scaffold.go around lines 221 to 229, the warning
message for when the root-file-name var is already set incorrectly references
shared.NoIncludeRootFlagName; update the message to use the correct flag
constant for the root-file-name flag (replace shared.NoIncludeRootFlagName with
the appropriate shared constant, e.g. shared.RootFileNameFlagName). If that
constant does not exist yet, add a new exported constant next to
NoIncludeRootFlagName in the shared package (name it clearly like
RootFileNameFlagName), then use it in the l.Warnf call so the warning prints the
correct flag name.
| // applyCatalogConfigToScaffold applies catalog configuration settings to scaffold options. | ||
| // CLI flags take precedence over config file settings. | ||
| func applyCatalogConfigToScaffold(ctx context.Context, l log.Logger, opts *options.TerragruntOptions) { | ||
| catalogCfg, err := config.ReadCatalogConfig(ctx, l, opts) | ||
| if err != nil { | ||
| // Don't fail if catalog config can't be read - it's optional | ||
| l.Debugf("Could not read catalog config for scaffold: %v", err) | ||
| return | ||
| } | ||
|
|
||
| if catalogCfg == nil { | ||
| return | ||
| } | ||
|
|
||
| // Apply config settings only if CLI flags weren't explicitly set | ||
| // Since both NoShell and NoHooks default to false, we apply the config value | ||
| // only if it's true (enabling the restriction) | ||
| if catalogCfg.NoShell != nil && *catalogCfg.NoShell && !opts.NoShell { | ||
| l.Debugf("Applying catalog config: no_shell = true") | ||
|
|
||
| opts.NoShell = true | ||
| } | ||
|
|
||
| if catalogCfg.NoHooks != nil && *catalogCfg.NoHooks && !opts.NoHooks { | ||
| l.Debugf("Applying catalog config: no_hooks = true") | ||
|
|
||
| opts.NoHooks = true | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Precedence bug: CLI “false” cannot override catalog “true”.
applyCatalogConfigToScaffold can’t distinguish default false from an explicitly passed --no-shell=false/--no-hooks=false, so config=true always wins. This contradicts “CLI flags take precedence” and your unit test expectation (e.g., “cli_override_config_true_with_false”). Fix by tracking “flag was set” state and honoring it.
Suggested approach:
- Add NoShellFlagSet/NoHooksFlagSet to options.TerragruntOptions and set them in the flags layer.
- Gate config application on these.
Diff (scaffold.go):
-func applyCatalogConfigToScaffold(ctx context.Context, l log.Logger, opts *options.TerragruntOptions) {
+func applyCatalogConfigToScaffold(ctx context.Context, l log.Logger, opts *options.TerragruntOptions) {
catalogCfg, err := config.ReadCatalogConfig(ctx, l, opts)
if err != nil {
l.Debugf("Could not read catalog config for scaffold: %v", err)
return
}
if catalogCfg == nil {
return
}
- if catalogCfg.NoShell != nil && *catalogCfg.NoShell && !opts.NoShell {
+ // Only apply when CLI did not set the flag explicitly.
+ if catalogCfg.NoShell != nil && *catalogCfg.NoShell && !opts.NoShellFlagSet {
l.Debugf("Applying catalog config: no_shell = true")
opts.NoShell = true
}
- if catalogCfg.NoHooks != nil && *catalogCfg.NoHooks && !opts.NoHooks {
+ if catalogCfg.NoHooks != nil && *catalogCfg.NoHooks && !opts.NoHooksFlagSet {
l.Debugf("Applying catalog config: no_hooks = true")
opts.NoHooks = true
}
}And in options.TerragruntOptions (separate file):
type TerragruntOptions struct {
// existing...
NoShell bool
NoHooks bool
+ // Set by flag parsing layer to preserve CLI-precedence semantics
+ NoShellFlagSet bool
+ NoHooksFlagSet bool
}Update flag parsing to set these booleans when the corresponding CLI flags are present.
Run to locate flags to update:
🏁 Script executed:
#!/usr/bin/env bash
rg -nP --type=go -C3 '\bNoShell\b|\bNoHooks\b|no-?shell|no-?hooks' cli/ --hiddenLength of output: 20069
---
🏁 Script executed:
```shell
#!/bin/bash
# Find TerragruntOptions struct definition
fd -e go -x grep -l 'type TerragruntOptions struct' {} \;
Length of output: 20
🏁 Script executed:
#!/bin/bash
# Show snippet of TerragruntOptions struct
fd -e go -x grep -A 30 'type TerragruntOptions struct' {} \;Length of output: 1468
---
🏁 Script executed:
```shell
#!/bin/bash
# Get entire TerragruntOptions struct definition
cat options/options.go | awk '/type TerragruntOptions struct/,/^}/ {print NR": "$0}'
Length of output: 12716
🏁 Script executed:
#!/bin/bash
# Check cli package and flag parsing details
rg -nP --type=go 'import.*cli' cli/flags/shared/shared.go -A 5 -B 2Length of output: 0
🏁 Script executed:
#!/bin/bash
# Look at complete flag parsing context for NoShell and NoHooks
rg -nP --type=go -B 10 -A 5 'Destination:.*NoShell|Destination:.*NoHooks' cli/flags/shared/shared.goLength of output: 795
---
🌐 Web query:
```web
urfave/cli BoolFlag explicit value detection when flag not provided
💡 Result:
Use the flag/Context IsSet check instead of the boolean value.
- Context.IsSet("name") returns true when the flag was provided (including --no-foo), false when it was not provided. Use that to distinguish "not provided" from "provided false". [1][2]
Minimal example (urfave/cli/v3):
app := &cli.App{
Flags: []cli.Flag{
&cli.BoolFlag{Name: "foo"},
},
Action: func(c *cli.Context) error {
if c.IsSet("foo") {
// user explicitly passed --foo (true) or --no-foo (false)
val := c.Bool("foo")
fmt.Println("explicit:", val)
} else {
// flag not provided
fmt.Println("not provided")
}
return nil
},
}
References:
- Context and Flag API (IsSet / BoolFlag docs). [1]
- Change notes adding IsSet / BoolFlag.Value features. [2]
Sources:
[1] pkg.go.dev — github.com/urfave/cli/v3 (Flag/Context docs).
[2] urfave/cli CHANGELOG (IsSet / BoolFlag.Value).
🏁 Script executed:
#!/bin/bash
# Find where flags are used/applied, look for OnAction or Action callbacks
rg -nP --type=go 'OnAction|Action.*func|flag.*action' cli/ -A 3 -B 3 | head -100Length of output: 6426
🏁 Script executed:
#!/bin/bash
# Look for where applyCatalogConfigToScaffold is called
rg -nP --type=go 'applyCatalogConfigToScaffold' cli/Length of output: 353
🏁 Script executed:
#!/bin/bash
# View the call site of applyCatalogConfigToScaffold
sed -n '120,145p' cli/commands/scaffold/scaffold.goLength of output: 940
🏁 Script executed:
#!/bin/bash
# Check scaffold command Action callback signature
sed -n '74,95p' cli/commands/scaffold/cli.goLength of output: 502
Precedence bug confirmed: CLI "false" cannot override catalog "true".
The review comment correctly identifies the issue. The applyCatalogConfigToScaffold function checks !opts.NoShell and !opts.NoHooks, which cannot distinguish an explicit --no-shell=false from a default unset state, so catalog config with true always wins—contradicting the stated precedence.
The architecture prevents using urfave/cli's Context.IsSet() at the applyCatalogConfigToScaffold call site (it receives a Go context.Context, not a CLI Context). The suggested approach of adding NoShellFlagSet and NoHooksFlagSet tracking fields aligns with the existing pattern (TFPathExplicitlySet at options.go:326) and is the correct fix.
Required changes:
- options/options.go: Add
NoShellFlagSet boolandNoHooksFlagSet boolfields toTerragruntOptionsstruct. - cli/flags/shared/shared.go: Add
Actioncallbacks toNoShellFlagNameandNoHooksFlagNameflags (similar to lines 104–115 in cli/commands/stack/cli.go) to set the tracking fields when flags are parsed. - cli/commands/scaffold/scaffold.go: Update the precedence checks at lines 267 and 273 to use
!opts.NoShellFlagSetand!opts.NoHooksFlagSetinstead of!opts.NoShelland!opts.NoHooks.
🤖 Prompt for AI Agents
In cli/commands/scaffold/scaffold.go around lines 250 to 278: the function
currently cannot tell if the CLI explicitly set --no-shell/--no-hooks to false
vs left them unset, so catalog true always wins; add two boolean tracking fields
NoShellFlagSet and NoHooksFlagSet to TerragruntOptions in options/options.go,
add Action callbacks on the NoShell and NoHooks flag definitions in
cli/flags/shared/shared.go that set those tracking fields when the flags are
parsed, and update the precedence checks in this file (lines ~267 and ~273) to
use !opts.NoShellFlagSet and !opts.NoHooksFlagSet instead of !opts.NoShell and
!opts.NoHooks so explicit CLI false overrides catalog true.
* chore: Bumping Boilerplate to `v0.10.0` * fix: Adding support for `no-shell` and `no-hooks` * feat: Adding tests for scaffold * fix: Fixing initialization of BoilerplateOptions * chore: Refactoring out shared `scaffold` flags to `shared` * docs: Updating docs with additional call-out * chore: Upgrading `boilerplate` * fix: Addressing lints * fix: Fixing test for Boilerplate bump * fix: Addressing lints * fix: Ignoring boilerplated configurations during tests
Description
Upgrades to the latest version of Boilerplate.
Also adds
--no-shelland--no-hooksto control the relevant settings in Boilerplate.TODOs
Read the Gruntwork contribution guidelines.
Release Notes (draft)
Added / Removed / Updated [X].
Migration Guide
Summary by CodeRabbit
Release Notes
--no-shelland--no-hooksflags to scaffold and catalog commands for controlling template execution behavior.no_shellandno_hooksoptional settings, with CLI flags taking priority.