4211 - fix: Setting correct exit code for hcl validate
#4290
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
📝 WalkthroughWalkthroughThe changes ensure that the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant Validator
User->>CLI: terragrunt hcl validate
CLI->>Validator: Run HCL validation
Validator-->>CLI: Return diagnostics (if any)
alt Diagnostics exist
CLI-->>User: Output diagnostics, return error (non-zero exit code)
else No diagnostics
CLI-->>User: Output success, return nil (zero exit code)
end
Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Possibly related PRs
Suggested reviewers
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (1.64.8)Error: you are using a configuration file for golangci-lint v2 with golangci-lint v1: please use golangci-lint v2 ✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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
|
yhakbar
force-pushed
the
4211/fix/setting-correct-exit-code-for-validate
branch
from
40100a4 to
1a9fb1d
Compare
yhakbar
force-pushed
the
4211/fix/setting-correct-exit-code-for-validate
branch
from
38290d5 to
1831163
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: 0
🧹 Nitpick comments (8)
cli/commands/hcl/validate/validate.go (2)
110-110: Address lint warning for dynamic error creation.The linter suggests using wrapped static errors instead of dynamic errors.
Consider refactoring to use a wrapped static error:
- stackErr = errors.Errorf("%d HCL validation error(s) found", len(diags)) + stackErr = errors.Wrap(errors.New("HCL validation error(s) found"), fmt.Sprintf("%d validation errors", len(diags)))Alternatively, you could define a static error variable and wrap it with context.
17-17: Modernize import statement.The linter suggests replacing the experimental import with the standard library equivalent.
- "golang.org/x/exp/slices" + "slices"The
slicespackage is now part of the standard library in Go 1.21+.test/integration_deprecated_test.go (1)
62-62: Address lint warning for fmt.Printf usage.The linter flags the use of
fmt.Printfas forbidden by pattern.Consider using the test logger instead:
- fmt.Printf("STDERR is %s.\n STDOUT is %s", errOutput, output) + t.Logf("STDERR is %s.\n STDOUT is %s", errOutput, output)This follows Go testing best practices and avoids the linter warning.
docs/_docs/04_reference/02-cli-options.md (2)
1786-1788: Grammar & clarity polish for the flag description
Minor wording tweaks improve readability and match the phrasing used elsewhere.-Path to a file with a list of directories that need to be excluded when running `run --all` commands, by default `.terragrunt-excludes`. Units in these directories will be -excluded during execution of the commands. If a relative path is specified, it should be relative from -[--working-dir](#working-dir). This will only exclude the module, not its dependencies. +Path to a file that lists directories to exclude when running `run --all` commands. +The default file is `.terragrunt-excludes`. +Units in these directories will be excluded during execution. +If a relative path is specified, it must be relative **to** [--working-dir](#working-dir). +This flag excludes only the module itself, not its dependencies.
1790-1794: Example block: explicitly mention non-zero exit code
The surrounding text explains why|| trueis needed, but calling it out in the example comment will save copy-pasting readers a subtle gotcha.-terragrunt run --all plan --queue-excludes-file <(terragrunt hcl validate --show-config-path || true) +# `hcl validate` returns a non-zero exit code on errors, so we append `|| true` +terragrunt run --all plan \ + --queue-excludes-file <(terragrunt hcl validate --show-config-path || true)docs-starlight/src/data/flags/queue-excludes-file.mdx (3)
3-5: Mirror wording changes made in the Jekyll docs
Keeps both documentation paths in sync and fixes the same minor grammatical issue.-description: Path to a file with a list of directories that need to be excluded when running `run --all` commands. +description: Path to a file that lists directories to exclude when running `run --all` commands.
10-11: “relative from” → “relative to”
Same grammar fix as in the primary docs.-If a relative path is specified, it should be relative from +If a relative path is specified, it should be relative to
15-17: Optional: clarify why|| trueis present
Adding a short inline comment keeps the Starlight snippet self-explanatory, matching the suggestion in the Jekyll docs.-terragrunt run --all plan --queue-excludes-file <(terragrunt hcl validate --show-config-path || true) +# hcl validate exits non-zero on validation failures, so we ignore the code here +terragrunt run --all plan --queue-excludes-file <(terragrunt hcl validate --show-config-path || true)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
cli/commands/hcl/validate/validate.go(1 hunks)docs-starlight/src/components/Flag.astro(2 hunks)docs-starlight/src/content.config.ts(1 hunks)docs-starlight/src/data/flags/queue-excludes-file.mdx(1 hunks)docs/_docs/04_reference/02-cli-options.md(1 hunks)test/integration_deprecated_test.go(1 hunks)test/integration_test.go(3 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
`docs-starlight/**/*.astro`: Review the Astro code in the `docs-starlight` direc...
docs-starlight/**/*.astro: Review the Astro code in thedocs-starlightdirectory for quality and correctness. Make sure that the Astro code follows best practices and is easy to understand, maintain, and follows best practices. When possible, suggest improvements to the Astro code to make it better.
⚙️ Source: CodeRabbit Configuration File
List of files the instruction was applied to:
docs-starlight/src/components/Flag.astro
`**/*.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.
⚙️ Source: CodeRabbit Configuration File
List of files the instruction was applied to:
cli/commands/hcl/validate/validate.gotest/integration_deprecated_test.gotest/integration_test.go
`docs-starlight/**/*.md*`: Review the documentation for clarity, grammar, and sp...
docs-starlight/**/*.md*: 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 indocsto 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.
⚙️ Source: CodeRabbit Configuration File
List of files the instruction was applied to:
docs-starlight/src/data/flags/queue-excludes-file.mdx
`docs/**/*.md`: Review the documentation for clarity, grammar, and spelling. Mak...
docs/**/*.md: 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 indocsto the Starlight + Astro based documentation indocs-starlight. Whenever changes are made to thedocsdirectory, ensure that an equivalent change is made in thedocs-starlightdirectory to keep thedocs-starlightdocumentation accurate.
⚙️ Source: CodeRabbit Configuration File
List of files the instruction was applied to:
docs/_docs/04_reference/02-cli-options.md
🧬 Code Graph Analysis (1)
test/integration_test.go (2)
test/helpers/package.go (3)
CleanupTerraformFolder(713-720)CopyEnvironment(84-97)RunTerragruntCommandWithOutput(828-832)util/file.go (1)
JoinPath(480-482)
🪛 GitHub Actions: Strict Lint
cli/commands/hcl/validate/validate.go
[warning] 455-455: Lint warning (err113): do not define dynamic errors, use wrapped static errors instead.
[warning] 17-17: Lint warning (exptostd): Import statement 'golang.org/x/exp/slices' can be replaced by 'slices'.
test/integration_deprecated_test.go
[warning] 62-903: Lint warning (forbidigo): use of fmt.Printf forbidden by pattern.
test/integration_test.go
[warning] 3804-3804: Lint warning (dogsled): declaration has 3 blank identifiers.
[warning] 1407-2492: Lint warning (forcetypeassert): type assertion must be checked in multiple locations.
🔇 Additional comments (8)
docs-starlight/src/content.config.ts (1)
55-55: LGTM! Schema extension follows best practices.The addition of the optional
defaultValproperty to the flags schema is well-implemented, following Zod conventions and maintaining consistency with the existing schema structure.docs-starlight/src/components/Flag.astro (2)
14-14: LGTM! Proper extraction of new property.The
defaultValproperty is correctly extracted from the flag entry data alongside other properties.
24-28: LGTM! Well-implemented conditional rendering.The default value badge is properly implemented with:
- Appropriate conditional rendering using
{defaultVal && (...)}- Consistent styling with existing badges using "tip" variant
- Logical positioning between Type and Aliases sections
cli/commands/hcl/validate/validate.go (1)
104-111: LGTM! Core logic correctly implements the PR objective.The error handling logic properly ensures a non-zero exit code when HCL validation errors are present. The error message format is clear and informative, showing the count of validation errors found.
test/integration_deprecated_test.go (1)
33-33: LGTM! Test correctly updated to match new validation behavior.The change from
require.NoError(t, err)torequire.Error(t, err)properly aligns with the new HCL validation behavior that returns errors when validation fails.test/integration_test.go (3)
705-705: LGTM: Correctly updated test expectation for validation failures.The change from
require.NoError(t, err)torequire.Error(t, err)is appropriate since theterragrunt hcl validatecommand should now return a non-zero exit code when HCL validation errors are present, aligning with the PR objective.
715-731: LGTM: Excellent addition of explicit exit code verification test.This new test function provides dedicated coverage for the core PR objective - ensuring that
terragrunt hcl validatereturns a non-zero exit code when HCL validation errors are found. The test structure is solid:
- Uses the same test fixture with known invalid HCL
- Explicitly verifies that an error is returned (non-zero exit code)
- Validates the error message contains the expected text
- Includes helpful comments explaining the test's purpose
The test name clearly describes its intent and the implementation is straightforward and focused.
746-746: LGTM: Consistent test expectation update.This change maintains consistency with the other HCL validation test updates, correctly expecting an error when running
terragrunt hcl validateon invalid HCL configuration with the--show-config-pathflag.
Description
Fixes #4211.
Returns a final error for
hcl validateso that it users can check the overall exit code.This is technically a breaking change, but I would advocate that we release this as part of a minor release instead of going through a full strict control process. It should be pretty simple to address, and it's typical behavior of linting tools, etc.
TODOs
Read the Gruntwork contribution guidelines.
Release Notes (draft)
Added error for overall exit code to be non-zero if any of the files are invalid in
hcl validatecommand.Migration Guide
If users were previously using
hcl validateexpecting a zero exit code, they'll need to suppress the exit code using something like|| truein their scripts. This is because the command now returns a non-zero exit code if any of the files are invalid.Summary by CodeRabbit
New Features
Bug Fixes
Documentation
queue-excludes-fileflag and--queue-excludes-fileCLI option, clarifying usage, terminology, and examples.Tests