-
- Notifications
You must be signed in to change notification settings - Fork 576
fix(terragrunt_providers_lock, terragrunt_validate): Proper argument handling #933
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughInsert an explicit end-of-options marker ( Changes
Sequence Diagram(s)sequenceDiagram autonumber actor User participant Hook as Hook Script participant TG as Terragrunt participant Sub as Subcommand (providers lock / validate) User->>Hook: Run hook Hook->>TG: terragrunt run --all -- <subcommand> [args] note right of TG #DDEBF7: `--` prevents TG from parsing further options TG->>Sub: Invoke <subcommand> with [args] Sub-->>TG: Exit code / output TG-->>Hook: Propagate result Hook-->>User: Return status Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested labels
Suggested reviewers
✨ Finishing touches
🧪 Generate unit tests
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. 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 Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (2)
hooks/terragrunt_validate.sh (1)
17-21: Add a brief comment to prevent regressions.Document why “--” is required here (Terragrunt parses flags before the separator).
if common::terragrunt_version_ge_0.78; then - local -ra RUN_ALL_SUBCOMMAND=(run --all -- validate) + # NOTE: keep the `--` to ensure flags after it go to Terraform, not Terragrunt. + local -ra RUN_ALL_SUBCOMMAND=(run --all -- validate) else - local -ra RUN_ALL_SUBCOMMAND=(run-all -- validate) + # NOTE: legacy run-all also requires `--` before the underlying command. + local -ra RUN_ALL_SUBCOMMAND=(run-all -- validate) fihooks/terragrunt_providers_lock.sh (1)
17-21: Minor DRY refactor: compose from RUN_PREFIX and SUBCOMMAND.Keeps scripts consistent and reduces duplication across hooks.
- if common::terragrunt_version_ge_0.78; then - local -ra RUN_ALL_SUBCOMMAND=(run --all -- providers lock) - else - local -ra RUN_ALL_SUBCOMMAND=(run-all -- providers lock) - fi + local -ra SUBCOMMAND=(providers lock) + local -a RUN_PREFIX=() + if common::terragrunt_version_ge_0.78; then + RUN_PREFIX=(run --all) + else + RUN_PREFIX=(run-all) + fi + # `--` ensures subsequent flags are forwarded to Terraform. + local -ra RUN_ALL_SUBCOMMAND=("${RUN_PREFIX[@]}" -- "${SUBCOMMAND[@]}")
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
hooks/terragrunt_providers_lock.sh(1 hunks)hooks/terragrunt_validate.sh(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
hooks/terragrunt_providers_lock.sh (1)
hooks/terragrunt_validate_inputs.sh (2)
run_hook_on_whole_repo(70-79)main(10-27)
hooks/terragrunt_validate.sh (1)
hooks/terragrunt_validate_inputs.sh (3)
run_hook_on_whole_repo(70-79)main(10-27)per_dir_hook_unique_part(44-62)
🔇 Additional comments (2)
hooks/terragrunt_validate.sh (1)
18-21: LGTM: correct use of “--” to forward args to Terraform under run/run-all.This fixes the -platform parsing issue and keeps behavior consistent across >=0.78 and legacy paths.
hooks/terragrunt_providers_lock.sh (1)
18-21: LGTM: “--” correctly inserted before providers lock.This ensures args like -platform=… reach Terraform in both version branches.
| @MaxymVlasov JFYI: the Validate PR title is failing and the error is a way too cryptic to me — will you have a chance to look into it please? UPD: Maybe |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
hooks/terragrunt_providers_lock.sh (1)
17-21: Correct use of end-of-options marker for TG ≥ 0.78; keep run-all unchanged for older TGAdding
--afterrun --allis right for forwarding Terraform flags; retainingrun-all providers lockfor < 0.78 avoids deviating from older UX/docs. Consider documenting this inline to prevent regressions.Apply this tiny doc diff:
if common::terragrunt_version_ge_0.78; then - local -ra RUN_ALL_SUBCOMMAND=(run --all -- providers lock) + # TG >= 0.78: use `run --all` and `--` to forward flags to Terraform/OpenTofu. + local -ra RUN_ALL_SUBCOMMAND=(run --all -- providers lock) else - local -ra RUN_ALL_SUBCOMMAND=(run-all providers lock) + # TG < 0.78: keep `run-all` (no standalone `--`) for compatibility with legacy behavior/docs. + local -ra RUN_ALL_SUBCOMMAND=(run-all providers lock) fiPlease sanity‑check both paths with TG v0.77.x and v0.78+ to confirm that hook‑provided
-platformargs are forwarded in “whole repo” runs.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
hooks/terragrunt_providers_lock.sh(1 hunks)hooks/terragrunt_validate.sh(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- hooks/terragrunt_validate.sh
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: jannis-a PR: antonbabenko/pre-commit-terraform#933 File: hooks/terragrunt_providers_lock.sh:18-21 Timestamp: 2025-09-13T19:34:00.281Z Learning: In terragrunt hooks, the `--` separator should only be added before Terraform commands that are invoked through Terragrunt (like `providers lock`, `validate`), but not before native Terragrunt commands (like `hcl validate`). The separator tells Terragrunt to pass subsequent arguments to the underlying Terraform command, which is not applicable for Terragrunt's own commands. 📚 Learning: 2025-09-13T19:34:00.281Z
Learnt from: jannis-a PR: antonbabenko/pre-commit-terraform#933 File: hooks/terragrunt_providers_lock.sh:18-21 Timestamp: 2025-09-13T19:34:00.281Z Learning: In terragrunt hooks, the `--` separator should only be added before Terraform commands that are invoked through Terragrunt (like `providers lock`, `validate`), but not before native Terragrunt commands (like `hcl validate`). The separator tells Terragrunt to pass subsequent arguments to the underlying Terraform command, which is not applicable for Terragrunt's own commands. Applied to files:
hooks/terragrunt_providers_lock.sh
🔇 Additional comments (1)
hooks/terragrunt_providers_lock.sh (1)
71-73: Invocation is safely quotedArray +
"${args[@]}"preserves argument boundaries; good for flags like-platform=linux_amd64.
## [1.100.1](v1.100.0...v1.100.1) (2025-09-27) ### Bug Fixes * **`terragrunt_providers_lock`, `terragrunt_validate`:** Properly handle arguments passed from terragrunt to TF ([#933](#933)) ([ea46354](ea46354))
| This PR is included in version 1.100.1 🎉 |
Put an
xinto the box if that apply:Description of your changes
Terragrunt's
run/run-allcommand requires a--before commands that get passed to Terraform to proper handle passed arguments. Without the--any arguments passed viaargs: [--arg=-foo]would get interpreted as Terragrunt argument rather than a Terraform argument.The currently used way without
--is just a shortcut:How can we test changes
Try e.g. this configuration:
Without this PR, the above would fail with: