feat(terraform_providers_lock): Fix logical issue in hook modes: Add check-lockfile-is-cross-platform and regenerate-lockfile-if-some-platform-missed modes. Last one is same as only-check-is-current-lockfile-cross-platform, which now is deprecated. Check README for more details
#950
Conversation
📝 WalkthroughSummary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughParses multiple Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
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 |
MaxymVlasov
changed the title
terraform_providers_lock): Log in which folder hook failedterraform_providers_lock): Logging in which folder hook failed
There was a problem hiding this comment.
Pull request overview
This PR enhances error logging for the terraform_providers_lock hook by displaying which folder the hook failed in, making it easier to debug issues when working with multiple directories.
Key Changes
- Added error message with directory path when
terraform providers lockcommand fails - Included helpful guidance pointing users to common resolution (running
terraform init)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
hooks/terraform_providers_lock.sh (1)
149-153: Critical: Core issue #949 is not addressed—early-exit logic still allows fallthrough to terraform providers lock.The conditional at lines 149–153 remains unchanged and still has the fundamental flaw described in issue #949. When
mode == "only-check-is-current-lockfile-cross-platform"andlockfile_contains_all_needed_shareturns non-zero (invalid/incomplete lockfile), the condition evaluates to false, execution continues to line 158, and the hook runsterraform providers lockanyway—triggering unwantedterraform init.Per issue #949, this block should be restructured to always exit when in that mode, using the exit code from the lockfile check:
- if [ "$mode" == "only-check-is-current-lockfile-cross-platform" ] && - lockfile_contains_all_needed_sha "$platforms_count"; then - - exit 0 - fi + if [ "$mode" == "only-check-is-current-lockfile-cross-platform" ]; then + lockfile_contains_all_needed_sha "$platforms_count" + exit_code=$? + return $exit_code + fiThis ensures the hook does not run
terraform providers lock(and avoid cascadingterraform init) when only checking cross-platform lockfile status in check-only mode.
🧹 Nitpick comments (1)
hooks/terraform_providers_lock.sh (1)
161-165: Error message text has grammar and clarity issues.Minor improvements to user-facing messaging:
- Line 162–163: "you didn't run requiring 'terraform init'" → awkward phrasing
- Suggested revision: "you likely skipped 'terraform init'" or "you need to run 'terraform init' first"
- common::colorify "red" "$dir_path run failed. Detailed error above. - Most common issue is that you didn't run requiring 'terraform init' before running this hook. It can be run by 'terraform_validate' hook - https://github.com/antonbabenko/pre-commit-terraform#terraform_validate - " + common::colorify "red" "$dir_path run failed. See details above. + The most common cause is missing 'terraform init'. You can run it via the terraform_validate hook: https://github.com/antonbabenko/pre-commit-terraform#terraform_validate + "
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
hooks/terraform_providers_lock.sh(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: jannis-a
Repo: antonbabenko/pre-commit-terraform PR: 933
File: hooks/terragrunt_providers_lock.sh:18-21
Timestamp: 2025-09-13T19:34:00.317Z
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.317Z
Learnt from: jannis-a
Repo: antonbabenko/pre-commit-terraform PR: 933
File: hooks/terragrunt_providers_lock.sh:18-21
Timestamp: 2025-09-13T19:34:00.317Z
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/terraform_providers_lock.sh
📚 Learning: 2025-08-12T19:49:13.257Z
Learnt from: actuarysailor
Repo: antonbabenko/pre-commit-terraform PR: 925
File: .pre-commit-hooks.yaml:216-227
Timestamp: 2025-08-12T19:49:13.257Z
Learning: In Terraform projects, most module folders are downloaded dependencies (similar to GitHub Actions) rather than locally maintained code. Users typically want to document only the root module to avoid commit noise from modules they consume but don't maintain. The terraform_docs_docker hook's current design with pass_filenames: false and args targeting the current directory (.) is appropriate for this common single-module repository pattern.
Applied to files:
hooks/terraform_providers_lock.sh
🧬 Code graph analysis (1)
hooks/terraform_providers_lock.sh (1)
hooks/_common.sh (1)
common::colorify(443-464)
⏰ 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). (8)
- GitHub Check: 🧪 Tests / pytest@🐍3.14@windows-2025
- GitHub Check: 🧪 Tests / pytest@🐍3.14@macos-15
- GitHub Check: 🧪 Tests / pytest@🐍3.12@macos-15
- GitHub Check: 🧪 Tests / pytest@🐍3.13@macos-15
- GitHub Check: 🧪 Tests / pytest@🐍3.11@windows-2025
- GitHub Check: 🧪 Tests / pytest@🐍3.11@macos-15
- GitHub Check: 🧪 Tests / pytest@🐍3.10@macos-15
- GitHub Check: pre-commit
🔇 Additional comments (1)
hooks/terraform_providers_lock.sh (1)
160-168: Error propagation and return flow are correctly implemented.The exit code is properly captured, conditionally logged with helpful context, and returned. This ensures failures bubble up correctly and users see actionable guidance.
Co-authored-by: George Yermulnik (Georgii Iermulnik) <yz@yz.kiev.ua>
This comment was marked as resolved.
This comment was marked as resolved.
github-actions
bot
added
the
stale
MaxymVlasov
removed
the
stale
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
hooks/terraform_providers_lock.sh (1)
157-157: Optional: Consider listing only the missing platforms.The current message displays all required platforms, but it would be more helpful to show specifically which platforms are missing from the lockfile. However, implementing this would require additional logic to compare the lockfile contents against the required platforms.
The current implementation still provides value, so this enhancement could be deferred.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
hooks/terraform_providers_lock.sh
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-13T19:34:00.317Z
Learnt from: jannis-a
Repo: antonbabenko/pre-commit-terraform PR: 933
File: hooks/terragrunt_providers_lock.sh:18-21
Timestamp: 2025-09-13T19:34:00.317Z
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/terraform_providers_lock.sh
🧬 Code graph analysis (1)
hooks/terraform_providers_lock.sh (1)
hooks/_common.sh (1)
common::colorify(443-464)
⏰ 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). (6)
- GitHub Check: 🧪 Tests / pytest@🐍3.13@macos-15-intel
- GitHub Check: 🧪 Tests / pytest@🐍3.11@macos-15-intel
- GitHub Check: 🧪 Tests / pytest@🐍3.10@macos-15-intel
- GitHub Check: 🧪 Tests / pytest@🐍3.12@macos-15-intel
- GitHub Check: 🧹 Linters / pre-commit@🐍3.13@ubuntu-latest
- GitHub Check: 🧪 Tests / pytest@🐍3.12@macos-15
🔇 Additional comments (2)
hooks/terraform_providers_lock.sh (2)
104-104: LGTM: Platform names tracking implementation is correct.The platform names array is properly declared and populated using correct parameter expansion to extract platform values from
-platform=arguments.Also applies to: 108-108
166-170: LGTM: Error handling implementation is solid.The error handling correctly addresses past review feedback:
- Uses arithmetic comparison (
-ne) as suggested- Incorporates the improved grammar from previous reviews
- Includes
$dir_pathto indicate which folder failed (addressing the PR title objective)- Provides helpful guidance about
terraform initand theterraform_validatehookThe original exit code is properly preserved and returned.
…is-current-lockfile-cross-platform` to `regenerate-lockfile-if-some-platform-missed`
There was a problem hiding this comment.
Actionable comments posted: 5
Fix all issues with AI Agents 🤖
In @README.md:
- Line 799: Change the compound adjective "time consuming" to the hyphenated
form "time-consuming" in the sentence that reads "Also, it could run another
slow and time consuming command - `terraform init`" so it becomes "slow and
time-consuming command - `terraform init`"; update that exact phrase in
README.md to use the hyphenated form.
- Line 815: The mode description has a sentence fragment ("Regenerate lockfile
from scratch. Can be useful for upgrading providers in lockfile to latest
versions"); update that line in README.md to be a single, complete sentence such
as "Regenerates the lockfile from scratch and can be useful for upgrading
providers in the lockfile to their latest versions" so the description is
grammatically correct and clear.
- Around line 770-772: Update the description for
--mode=check-lockfile-is-cross-platform: fix grammar and clarity by changing
"Checks that lockfile has the same amount of platform (`h1:`) checksums as
specified in hook configuration. It **does not** check are these checksums are
valid or that they are belongs to needed platforms." to something like "Checks
that the lockfile has the same number of platform (`h1:`) checksums as specified
in the hook configuration. It does not check whether these checksums are valid
or whether they belong to the required platforms." Ensure you replace the
original sentences under the --mode=check-lockfile-is-cross-platform summary
accordingly.
- Line 796: The sentence fragment "Make up-to-date lockfile by adding/removing
providers and only then check that lockfile has all required SHAs. If any missed
- adds them." should be rewritten as a complete, grammatical description:
replace that line in the README with a single clear sentence such as "Make an
up-to-date lockfile by adding or removing providers, then check that the
lockfile contains all required SHAs and add any that are missing." Locate and
update the exact fragment shown in the diff to ensure the mode description is a
full sentence.
- Line 783: Update the sentence describing the mode to use correct grammar and
clearer tense: change "Checks that lockfile has all required SHAs for all
providers already added to lockfile, and if any missed - try to add them (but
could fail if `terraform init` wasn't run previously)" to something like "Checks
that the lockfile contains all required SHAs for providers already added; if any
are missing, attempts to add them (this may fail if `terraform init` hasn't been
run previously)." Locate and replace that sentence in README.md.
🧹 Nitpick comments (1)
hooks/terraform_providers_lock.sh (1)
157-166: Correctly implements pure check-only mode, resolving issue #949.This new mode properly exits immediately with status 1 when the lockfile is missing required platforms, without attempting to regenerate it. This addresses the core issue described in #949 where the old implementation would proceed to run
terraform providers lockeven in check-only mode.Optional: Consider formatting consistency
The leading
\nand multi-line string formatting differs from the error message at lines 185-187. Consider using consistent formatting:- common::colorify "red" "\n$dir_path/.terraform.lock.hcl missing some of required platforms. - All required platforms: ${platforms_names[*]}." + common::colorify "red" "$dir_path/.terraform.lock.hcl missing some of required platforms. +All required platforms: ${platforms_names[*]}"However, the current formatting may be intentional for output readability.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
README.mdhooks/terraform_providers_lock.sh
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-13T19:34:00.317Z
Learnt from: jannis-a
Repo: antonbabenko/pre-commit-terraform PR: 933
File: hooks/terragrunt_providers_lock.sh:18-21
Timestamp: 2025-09-13T19:34:00.317Z
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/terraform_providers_lock.shREADME.md
🧬 Code graph analysis (1)
hooks/terraform_providers_lock.sh (1)
hooks/_common.sh (1)
common::colorify(443-464)
🪛 LanguageTool
README.md
[grammar] ~799-~799: Use a hyphen to join words.
Context: ...Also, it could run another slow and time consuming command - terraform init ...
(QB_NEW_EN_HYPHEN)
[style] ~815-~815: To form a complete sentence, be sure to include a subject.
Context: ... Regenerate lockfile from scratch. Can be useful for upgrading providers in lo...
(MISSING_IT_THERE)
⏰ 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). (6)
- GitHub Check: 🧪 Tests / pytest@🐍3.10@macos-15
- GitHub Check: 🧪 Tests / pytest@🐍3.12@macos-15-intel
- GitHub Check: 🧪 Tests / pytest@🐍3.12@macos-15
- GitHub Check: 🧪 Tests / pytest@🐍3.11@macos-15
- GitHub Check: 🧪 Tests / pytest@🐍3.13@macos-15
- GitHub Check: 🧹 Linters / pre-commit@🐍3.13@ubuntu-latest
🔇 Additional comments (4)
hooks/terraform_providers_lock.sh (4)
104-109: LGTM! Clean platform name collection.The implementation correctly extracts platform names from
-platform=arguments and stores them for later use in error messages. The parameter expansion syntax${arg#*=}properly removes the prefix up to and including the=sign.
138-155: Well-implemented deprecation path with backward compatibility.The code properly handles the transition from the old mode name
only-check-is-current-lockfile-cross-platformto the new nameregenerate-lockfile-if-some-platform-missed, providing clear deprecation notices while maintaining backward compatibility.
168-176: LGTM! Properly implements regenerate-if-missing behavior.This mode correctly checks for missing platform SHAs and, when found, prints a warning and continues to regenerate the lockfile. The yellow color appropriately indicates a warning rather than an error, as the hook will attempt to fix the issue automatically.
184-189: Excellent error handling with helpful guidance.The implementation properly:
- Uses arithmetic comparison operators (
-ne) as suggested in past reviews- Captures and returns the exit code from
terraform providers lock- Provides clear, grammatically correct error messages with actionable guidance
The improved error message helps users understand the most common issue (missing
terraform init) and points them to theterraform_validatehook for a solution.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
hooks/terraform_providers_lock.sh (2)
178-179: Minor grammar issue in error message.The phrase "missing some of required platforms" is slightly awkward grammatically.
🔎 Suggested grammar improvements
Choose one of these options:
Option 1: Add article "the"
- common::colorify "red" "\n$dir_path/.terraform.lock.hcl missing some of required platforms. + common::colorify "red" "\n$dir_path/.terraform.lock.hcl missing some of the required platforms. All required platforms: ${platforms_names[*]}."Option 2: Add "is" and remove "of"
- common::colorify "red" "\n$dir_path/.terraform.lock.hcl missing some of required platforms. + common::colorify "red" "\n$dir_path/.terraform.lock.hcl is missing some required platforms. All required platforms: ${platforms_names[*]}."
189-190: Minor grammar issue in warning message.Same grammatical issue as the check mode: "missing some of required platforms" is slightly awkward.
🔎 Suggested grammar improvements
Choose one of these options:
Option 1: Add article "the"
- common::colorify "yellow" "\n$dir_path/.terraform.lock.hcl missing some of required platforms. + common::colorify "yellow" "\n$dir_path/.terraform.lock.hcl missing some of the required platforms. All required platforms: ${platforms_names[*]}."Option 2: Add "is" and remove "of"
- common::colorify "yellow" "\n$dir_path/.terraform.lock.hcl missing some of required platforms. + common::colorify "yellow" "\n$dir_path/.terraform.lock.hcl is missing some required platforms. All required platforms: ${platforms_names[*]}."
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
hooks/terraform_providers_lock.sh
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-13T19:34:00.317Z
Learnt from: jannis-a
Repo: antonbabenko/pre-commit-terraform PR: 933
File: hooks/terragrunt_providers_lock.sh:18-21
Timestamp: 2025-09-13T19:34:00.317Z
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/terraform_providers_lock.sh
🧬 Code graph analysis (1)
hooks/terraform_providers_lock.sh (1)
hooks/_common.sh (1)
common::colorify(443-464)
⏰ 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). (7)
- GitHub Check: 🧪 Tests / pytest@🐍3.13@macos-15-intel
- GitHub Check: 🧪 Tests / pytest@🐍3.11@macos-15-intel
- GitHub Check: 🧪 Tests / pytest@🐍3.12@macos-15-intel
- GitHub Check: 🧪 Tests / pytest@🐍3.11@windows-2025
- GitHub Check: 🧪 Tests / pytest@🐍3.11@macos-15
- GitHub Check: 🧪 Tests / pytest@🐍3.10@macos-15
- GitHub Check: 🧹 Linters / pre-commit@🐍3.13@ubuntu-latest
🔇 Additional comments (3)
hooks/terraform_providers_lock.sh (3)
104-110: LGTM! Platform name collection is well-implemented.The array correctly captures platform names from
-platform=arguments for user-facing messages.
133-155: LGTM! Mode validation is comprehensive and well-structured.The deprecation handling is clear and properly maps the old mode name to the new behavior while informing users about the change.
199-203: LGTM! Error handling is correct and past review comments have been addressed.The arithmetic comparison using
-neis appropriate, and the grammar issue with "requiring" has been fixed. The error message now correctly states "required 'terraform init' command was likely not run" and provides helpful guidance with a documentation link.
MaxymVlasov
changed the title
terraform_providers_lock): Logging in which folder hook failedterraform_providers_lock): Fix logical issue in hook modes: Add check-lockfile-is-cross-platform and regenerate-lockfile-if-some-platform-missed modes. Last one is same as only-check-is-current-lockfile-cross-platform, which now is deprecated. Check README for more details
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
README.md (5)
771-771: Fix grammar errors in mode description.The sentence contains two grammar errors:
- "does not check are these checksums are valid" (double verb)
- "they are belongs to needed platforms" (incorrect verb form)
🔎 Proposed fix
- Checks that lockfile has the same amount of platform (`h1:`) checksums as specified in hook configuration. It **does not** check are these checksums are valid or that they are belongs to needed platforms. + Checks that lockfile has the same amount of platform (`h1:`) checksums as specified in hook configuration. It **does not** check if these checksums are valid or that they belong to the needed platforms.
783-783: Fix grammar in mode description.The phrase "if any missed - try to add them" uses incorrect verb tenses.
🔎 Proposed fix
- Checks that lockfile has all required SHAs for all providers already added to lockfile, and if any missed - try to add them (but could fail if `terraform init` wasn't run previously) + Checks that lockfile has all required SHAs for all providers already added to lockfile, and if any are missing - tries to add them (but could fail if `terraform init` wasn't run previously)
796-796: Fix sentence fragment in mode description.The description starts with an incomplete sentence using an imperative verb.
🔎 Proposed fix
- Make up-to-date lockfile by adding/removing providers and only then check that lockfile has all required SHAs. If any missed - adds them. + Makes an up-to-date lockfile by adding/removing providers and only then checks that the lockfile has all required SHAs. If any are missing, it adds them.
799-799: Use hyphenated compound adjective."Time consuming" should be hyphenated when used as a compound adjective before a noun.
🔎 Proposed fix
- > Next [`terraform_validate`](#terraform_validate) hook flag requires additional dependency to be installed: `jq`. Also, it could run another slow and time consuming command - `terraform init` + > Next [`terraform_validate`](#terraform_validate) hook flag requires additional dependency to be installed: `jq`. Also, it could run another slow and time-consuming command - `terraform init`
815-815: Fix sentence fragment in mode description.The description contains an incomplete sentence ("Can be useful...").
🔎 Proposed fix
- Regenerate lockfile from scratch. Can be useful for upgrading providers in lockfile to latest versions + Regenerates the lockfile from scratch. This can be useful for upgrading providers in the lockfile to their latest versions.
🧹 Nitpick comments (2)
hooks/terraform_providers_lock.sh (2)
172-181: Capture and preserve the exit code from lockfile validation.The
lockfile_contains_all_needed_shafunction returns different exit codes based on failure type (1-99 for invalid lockfile, 100+ for missing SHAs). Currently, the code hardcodesexit 1which loses this diagnostic information.🔎 Proposed fix to preserve exit code
if [ "$mode" == "check-lockfile-is-cross-platform" ]; then - - if lockfile_contains_all_needed_sha "$platforms_count"; then + lockfile_contains_all_needed_sha "$platforms_count" + exit_code=$? + if [[ $exit_code -eq 0 ]]; then exit 0 fi common::colorify "red" "\n$dir_path/.terraform.lock.hcl missing some of required platforms. All required platforms: ${platforms_names[*]}." - exit 1 + exit $exit_code fi
183-191: Consider capturing the validation exit code for consistency.While this mode intentionally continues to regenerate the lockfile on validation failure, capturing the exit code would improve consistency with the
check-lockfile-is-cross-platformmode and could be useful for debugging.🔎 Optional improvement
if [ "$mode" == "regenerate-lockfile-if-some-platform-missed" ]; then - - if lockfile_contains_all_needed_sha "$platforms_count"; then + lockfile_contains_all_needed_sha "$platforms_count" + validation_exit_code=$? + if [[ $validation_exit_code -eq 0 ]]; then exit 0 fi common::colorify "yellow" "\n$dir_path/.terraform.lock.hcl missing some of required platforms. All required platforms: ${platforms_names[*]}." fi
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
README.mdhooks/terraform_providers_lock.sh
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-13T19:34:00.317Z
Learnt from: jannis-a
Repo: antonbabenko/pre-commit-terraform PR: 933
File: hooks/terragrunt_providers_lock.sh:18-21
Timestamp: 2025-09-13T19:34:00.317Z
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/terraform_providers_lock.shREADME.md
🧬 Code graph analysis (1)
hooks/terraform_providers_lock.sh (1)
hooks/_common.sh (1)
common::colorify(443-464)
🪛 LanguageTool
README.md
[grammar] ~799-~799: Use a hyphen to join words.
Context: ...Also, it could run another slow and time consuming command - terraform init ...
(QB_NEW_EN_HYPHEN)
[style] ~815-~815: To form a complete sentence, be sure to include a subject.
Context: ... Regenerate lockfile from scratch. Can be useful for upgrading providers in lo...
(MISSING_IT_THERE)
⏰ 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). (6)
- GitHub Check: 🧪 Tests / pytest@🐍3.10@macos-15
- GitHub Check: 🧪 Tests / pytest@🐍3.10@macos-15-intel
- GitHub Check: 🧪 Tests / pytest@🐍3.13@macos-15
- GitHub Check: 🧪 Tests / pytest@🐍3.11@macos-15-intel
- GitHub Check: 🧪 Tests / pytest@🐍3.12@macos-15
- GitHub Check: 🧹 Linters / pre-commit@🐍3.13@ubuntu-latest
🔇 Additional comments (3)
hooks/terraform_providers_lock.sh (3)
104-104: LGTM! Helpful addition for error context.Collecting platform names in an array allows the hook to display which platforms are required in error messages, improving user experience.
Also applies to: 108-108
134-152: LGTM! Solid mode validation with deprecation handling.The case statement properly validates modes, provides clear deprecation warnings for the old mode name, and exits with helpful error messages for invalid modes.
199-206: Excellent error handling implementation!The code properly:
- Captures the exit code from
terraform providers lock- Uses arithmetic comparison (
-ne) as recommended- Provides clear, actionable error guidance pointing users to the
terraform_validatehook- Preserves and returns the original exit code
Co-authored-by: George Yermulnik (Georgii Iermulnik) <yz@yz.kiev.ua>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
README.md (4)
783-783: Fix verb tense in mode description.The phrase "if any missed - try to add them" uses inconsistent verb tenses. Use present tense throughout for clarity.
Suggested fix
- Checks that lockfile has checksums (`h1:`) for all requested platforms for all providers tracked by the lockfile, and if any are missed - tries to add them (but could fail if `terraform init` wasn't run previously). + Checks that the lockfile contains checksums (`h1:`) for all requested platforms for all providers tracked by the lockfile. If any are missing, attempts to add them (but may fail if `terraform init` wasn't run previously).Based on past review feedback.
796-796: Use American English spelling and improve grammar.Two issues: (1) "afterwards" should be "afterward" in American English, and (2) the phrase "If any are missed - adds them" has inconsistent subject-verb agreement.
Suggested fix
- Regenerates lockfile for all required providers and checks that the lockfile tracks all required platform checksums (`h1:`) afterwards. If any are missed - adds them; superfluous providers are removed. + Regenerates lockfile for all required providers and checks that the lockfile tracks all required platform checksums (`h1:`) afterward. If any are missing, it adds them; superfluous providers are removed.Based on past review feedback and static analysis.
799-799: Use hyphenated compound adjective.When "time consuming" is used as a compound adjective before a noun, it should be hyphenated as "time-consuming".
Suggested fix
- > The following [`terraform_validate`](#terraform_validate) hook's flag requires additional dependency to be installed: [`jq`](https://github.com/jqlang/jq). Also, it could run another slow and time consuming command - `terraform init` + > The following [`terraform_validate`](#terraform_validate) hook's flag requires additional dependency to be installed: [`jq`](https://github.com/jqlang/jq). Also, it could run another slow and time-consuming command - `terraform init`Based on past review feedback and static analysis.
815-815: Fix sentence fragment and article usage.The description contains a sentence fragment ("May be useful...") and uses the incorrect article "the" before "scratch".
Suggested fix
- Regenerates lockfile from the scratch. May be useful for upgrading providers in the lockfile to the latest versions. + Regenerates the lockfile from scratch. This can be useful for upgrading providers in the lockfile to their latest versions.Based on past review feedback and static analysis.
🧹 Nitpick comments (3)
hooks/terraform_providers_lock.sh (2)
133-152: Mode validation and deprecation handling looks solid.The case statement properly validates all supported modes and provides clear messaging for both deprecated and invalid modes. The deprecation path for
only-check-is-current-lockfile-cross-platformis well-documented with actionable guidance.Optional: Remove extraneous blank line for consistency
mode=$value - case $mode in
172-191: Mode-specific short-circuit behavior is correctly implemented.Both modes now properly handle the lockfile check results:
check-lockfile-is-cross-platformexits immediately with appropriate error messagingregenerate-lockfile-if-some-platform-missedwarns and continues to add missing platformsThis addresses the core issue from #949 where the check mode was incorrectly proceeding to regenerate the lockfile.
Optional: Remove blank lines for consistency
if [ "$mode" == "check-lockfile-is-cross-platform" ]; then - if lockfile_contains_all_needed_sha "$platforms_count"; then exit 0 fi - common::colorify "red" "\n$dir_path/.terraform.lock.hcl missing some of required platforms.if [ "$mode" == "regenerate-lockfile-if-some-platform-missed" ]; then - if lockfile_contains_all_needed_sha "$platforms_count"; then exit 0 fi - common::colorify "yellow" "$dir_path/.terraform.lock.hcl missing some of required platforms.README.md (1)
762-762: Clarify the timeline for the default mode change.The phrase "will be changed, probably, to" introduces ambiguity. Consider specifying when this change will occur to set clear expectations.
Suggested clarification
-> Why? When v2.x will be introduced - the default mode will be changed, probably, to `check-lockfile-is-cross-platform`. +> Why? When v2.0 is released - the default mode will be changed to `check-lockfile-is-cross-platform`.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
README.mdhooks/terraform_providers_lock.sh
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-13T19:34:00.317Z
Learnt from: jannis-a
Repo: antonbabenko/pre-commit-terraform PR: 933
File: hooks/terragrunt_providers_lock.sh:18-21
Timestamp: 2025-09-13T19:34:00.317Z
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/terraform_providers_lock.shREADME.md
🧬 Code graph analysis (1)
hooks/terraform_providers_lock.sh (1)
hooks/_common.sh (1)
common::colorify(443-464)
🪛 LanguageTool
README.md
[locale-violation] ~796-~796: In American English, ‘afterward’ is the preferred variant. ‘Afterwards’ is more commonly used in British English and other dialects.
Context: ...all required platform checksums (h1:) afterwards. If any are missed - adds them; superfl...
(AFTERWARDS_US)
[grammar] ~799-~799: Use a hyphen to join words.
Context: ...Also, it could run another slow and time consuming command - terraform init ...
(QB_NEW_EN_HYPHEN)
[style] ~815-~815: To form a complete sentence, be sure to include a subject.
Context: ... Regenerates lockfile from the scratch. May be useful for upgrading providers in th...
(MISSING_IT_THERE)
⏰ 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). (7)
- GitHub Check: 🧪 Tests / pytest@🐍3.10@macos-15
- GitHub Check: 🧪 Tests / pytest@🐍3.12@macos-15
- GitHub Check: 🧪 Tests / pytest@🐍3.11@macos-15-intel
- GitHub Check: 🧪 Tests / pytest@🐍3.14@windows-2025
- GitHub Check: 🧪 Tests / pytest@🐍3.10@macos-15-intel
- GitHub Check: 🧪 Tests / pytest@🐍3.14@macos-15-intel
- GitHub Check: 🧹 Linters / pre-commit@🐍3.13@ubuntu-latest
🔇 Additional comments (2)
hooks/terraform_providers_lock.sh (2)
104-109: LGTM! Platform names collection for error messages.The addition of the
platforms_namesarray properly extracts platform identifiers from-platform=arguments and will improve error messages by listing specific required platforms.
199-206: LGTM! Improved error handling with helpful guidance.The exit code handling now:
- Uses arithmetic comparison (
-ne) appropriately- Provides clear, actionable error messages directing users to the
terraform_validatehook- Preserves the original exit code for proper pre-commit integration
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
hooks/terraform_providers_lock.sh (4)
134-134: Quote the case variable for safety.The variable should be quoted to handle edge cases with spaces or special characters, following shell scripting best practices.
🔎 Suggested fix
- case $mode in + case "$mode" in
139-144: Consider improving deprecation message formatting.The deprecation message has awkward line breaks. Consider reformatting for better readability.
🔎 Suggested improvement
only-check-is-current-lockfile-cross-platform) - common::colorify "yellow" "DEPRECATION NOTICE: Flag '--mode=only-check-is-current-lockfile-cross-platform' was renamed - to '--mode=regenerate-lockfile-if-some-platform-missed' to better reflect its behavior. Please update your configuration. -" + common::colorify "yellow" "DEPRECATION NOTICE: Flag '--mode=only-check-is-current-lockfile-cross-platform' was renamed to '--mode=regenerate-lockfile-if-some-platform-missed' to better reflect its behavior. Please update your configuration." mode="regenerate-lockfile-if-some-platform-missed" ;;
172-194: Excellent fix! Core logic correctly addresses issue #949.The new case-based flow properly implements the intended behavior:
check-lockfile-is-cross-platformexits immediately with an error when platforms are missing, preventing unintendedterraform providers lockexecution.regenerate-lockfile-if-some-platform-missedwarns but continues to regenerate the lockfile.This correctly fixes the bug where the check-only mode would proceed to regenerate the lockfile instead of exiting.
Minor: Quote the case variable on line 172.
Similar to line 134,
$modeshould be quoted:- case "$mode" in + case "$mode" in
173-183: Optional: Consider preserving the actual exit code for diagnostics.The current implementation always exits with
1on failure. You could preserve the actual exit code fromlockfile_contains_all_needed_sha(which returns different codes for invalid lockfiles vs. missing platforms) for more detailed diagnostics.🔎 Optional improvement
"check-lockfile-is-cross-platform") - if lockfile_contains_all_needed_sha "$platforms_count"; then + lockfile_contains_all_needed_sha "$platforms_count" + exit_code=$? + if [[ $exit_code -eq 0 ]]; then exit 0 fi common::colorify "red" "$dir_path/.terraform.lock.hcl missing some of required platforms. All required platforms: ${platforms_names[*]} " - exit 1 + exit $exit_code ;;However, returning a simple binary exit code (0 or 1) is also perfectly reasonable for a check mode and may be preferred for simplicity.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
hooks/terraform_providers_lock.sh
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-13T19:34:00.317Z
Learnt from: jannis-a
Repo: antonbabenko/pre-commit-terraform PR: 933
File: hooks/terragrunt_providers_lock.sh:18-21
Timestamp: 2025-09-13T19:34:00.317Z
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/terraform_providers_lock.sh
🧬 Code graph analysis (1)
hooks/terraform_providers_lock.sh (1)
hooks/_common.sh (1)
common::colorify(443-464)
⏰ 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). (6)
- GitHub Check: 🧪 Tests / pytest@🐍3.14@macos-15
- GitHub Check: 🧪 Tests / pytest@🐍3.13@windows-2025
- GitHub Check: 🧪 Tests / pytest@🐍3.12@macos-15
- GitHub Check: 🧪 Tests / pytest@🐍3.12@macos-15-intel
- GitHub Check: 🧪 Tests / pytest@🐍3.11@macos-15-intel
- GitHub Check: 🧹 Linters / pre-commit@🐍3.13@ubuntu-latest
🔇 Additional comments (2)
hooks/terraform_providers_lock.sh (2)
104-110: LGTM! Good addition for error messaging.The
platforms_namesarray properly collects platform names from the-platformarguments, enabling more informative error messages later in the hook.
199-209: LGTM! Error handling is well-implemented.The code correctly:
- Uses arithmetic comparison (
-ne 0) for the exit code check- Provides a helpful error message with guidance about
terraform initand theterraform_validatehook- Preserves and returns the original exit code from
terraform providers lockThe error message wording also addresses the grammar concerns from previous reviews.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
hooks/terraform_providers_lock.sh (2)
129-129: Minor: Consider rephrasing for clarity.The phrase "not more than one" is slightly awkward. Consider simplifying:
- common::colorify "yellow" 'Invalid hook config. Make sure that you specify not more than one "--mode" flag.' + common::colorify "yellow" 'Invalid hook config. The "--mode" flag can only be specified once.'
172-194: Case-based mode logic correctly implements check and regenerate behaviors.The case statement properly implements:
check-lockfile-is-cross-platform: Exits immediately with success (0) or failure (1) without runningterraform providers lock, fixing the issue from #949.regenerate-lockfile-if-some-platform-missed: Exits on success or prints a warning and falls through to regenerate the lockfile.always-regenerate-lockfile: Implicitly falls through (no case branch) to always runterraform providers lock.The fall-through behavior for
always-regenerate-lockfileis correct but implicit.Optional: Make always-regenerate-lockfile explicit for clarity
While the current fall-through works correctly, adding an explicit case branch with a comment would make the intent clearer:
"regenerate-lockfile-if-some-platform-missed") if lockfile_contains_all_needed_sha "$platforms_count"; then exit 0 fi common::colorify "yellow" "$dir_path/.terraform.lock.hcl missing some of required platforms. All required platforms: ${platforms_names[*]} " ;; + "always-regenerate-lockfile") + # Intentionally fall through to always regenerate lockfile + ;; esac
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
hooks/terraform_providers_lock.sh
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-13T19:34:00.317Z
Learnt from: jannis-a
Repo: antonbabenko/pre-commit-terraform PR: 933
File: hooks/terragrunt_providers_lock.sh:18-21
Timestamp: 2025-09-13T19:34:00.317Z
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/terraform_providers_lock.sh
🧬 Code graph analysis (1)
hooks/terraform_providers_lock.sh (1)
hooks/_common.sh (1)
common::colorify(443-464)
⏰ 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). (7)
- GitHub Check: 🧪 Tests / pytest@🐍3.11@macos-15-intel
- GitHub Check: 🧪 Tests / pytest@🐍3.14@macos-15-intel
- GitHub Check: 🧪 Tests / pytest@🐍3.13@macos-15-intel
- GitHub Check: 🧪 Tests / pytest@🐍3.13@windows-2025
- GitHub Check: 🧪 Tests / pytest@🐍3.12@macos-15-intel
- GitHub Check: 🧹 Linters / pre-commit@🐍3.13@ubuntu-latest
- GitHub Check: 🧪 Tests / pytest@🐍3.10@macos-15-intel
🔇 Additional comments (3)
hooks/terraform_providers_lock.sh (3)
104-104: LGTM! Helpful addition for error messages.The
platforms_namesarray correctly captures platform names from-platform=arguments and enhances error messages by listing all required platforms.Also applies to: 108-108
134-152: Excellent mode validation with clear deprecation path.The nested case statement cleanly validates mode values, provides helpful error messages for invalid modes, and includes a clear deprecation warning that automatically remaps the old flag name to maintain backward compatibility.
202-206: Excellent error handling with helpful guidance.The exit code handling properly:
- Uses arithmetic comparison (
-ne) as suggested in past reviews.- Improves the error message grammar to clearly explain the likely cause (missing
terraform init).- Provides actionable guidance pointing to the
terraform_validatehook.- Preserves and returns the original exit code.
MaxymVlasov
changed the title
terraform_providers_lock): Fix logical issue in hook modes: Add check-lockfile-is-cross-platform and regenerate-lockfile-if-some-platform-missed modes. Last one is same as only-check-is-current-lockfile-cross-platform, which now is deprecated. Check README for more detailsterraform_providers_lock): Fix logical issue in hook modes: Add check-lockfile-is-cross-platform and regenerate-lockfile-if-some-platform-missed modes. Last one is same as only-check-is-current-lockfile-cross-platform, which now is deprecated. Check README for more details
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
hooks/terraform_providers_lock.sh (1)
170-190: Consider adding an explicit case branch foralways-regenerate-lockfilemode.While the current fall-through behavior is correct for
always-regenerate-lockfilemode (no check, just regenerate), adding an explicit case branch would make the intent clearer:🔎 Proposed enhancement for clarity
case "$mode" in "check-lockfile-is-cross-platform") if lockfile_contains_all_needed_sha "$platforms_count"; then exit 0 fi common::colorify "red" "$dir_path/.terraform.lock.hcl missing some of required platforms. All required platforms: ${platforms_names[*]}" exit 1 ;; "regenerate-lockfile-if-some-platform-missed") if lockfile_contains_all_needed_sha "$platforms_count"; then exit 0 fi common::colorify "yellow" "$dir_path/.terraform.lock.hcl missing some of required platforms. All required platforms: ${platforms_names[*]}" ;; + "always-regenerate-lockfile") + # Always regenerate without checking - fall through to terraform providers lock + ;; esac
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
hooks/terraform_providers_lock.sh
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-09-13T19:34:00.317Z
Learnt from: jannis-a
Repo: antonbabenko/pre-commit-terraform PR: 933
File: hooks/terragrunt_providers_lock.sh:18-21
Timestamp: 2025-09-13T19:34:00.317Z
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/terraform_providers_lock.sh
📚 Learning: 2025-08-12T19:49:13.257Z
Learnt from: actuarysailor
Repo: antonbabenko/pre-commit-terraform PR: 925
File: .pre-commit-hooks.yaml:216-227
Timestamp: 2025-08-12T19:49:13.257Z
Learning: In Terraform projects, most module folders are downloaded dependencies (similar to GitHub Actions) rather than locally maintained code. Users typically want to document only the root module to avoid commit noise from modules they consume but don't maintain. The terraform_docs_docker hook's current design with pass_filenames: false and args targeting the current directory (.) is appropriate for this common single-module repository pattern.
Applied to files:
hooks/terraform_providers_lock.sh
🧬 Code graph analysis (1)
hooks/terraform_providers_lock.sh (1)
hooks/_common.sh (2)
common::colorify(443-464)common::terraform_init(534-584)
⏰ 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). (7)
- GitHub Check: 🧪 Tests / pytest@🐍3.14@macos-15
- GitHub Check: 🧪 Tests / pytest@🐍3.12@macos-15-intel
- GitHub Check: 🧪 Tests / pytest@🐍3.10@macos-15-intel
- GitHub Check: 🧪 Tests / pytest@🐍3.10@windows-2025
- GitHub Check: 🧪 Tests / pytest@🐍3.14@windows-2025
- GitHub Check: 🧪 Tests / pytest@🐍3.13@macos-15-intel
- GitHub Check: 🧹 Linters / pre-commit@🐍3.13@ubuntu-latest
🔇 Additional comments (7)
hooks/terraform_providers_lock.sh (7)
104-104: LGTM! Platform names collection is well implemented.The
platforms_namesarray is properly declared and populated using parameter expansion to extract platform values from-platform=arguments.Also applies to: 108-108
126-153: LGTM! Mode validation with deprecation handling is well structured.The case-based validation matches best practices and addresses past review feedback. The deprecation mapping for
only-check-is-current-lockfile-cross-platform→regenerate-lockfile-if-some-platform-missedis clear, and the error messages enumerate all valid modes.
157-159: LGTM! Comments accurately document the three supported modes.The comment clarifies that
check-lockfile-is-cross-platformwill become the default in v2.0, providing clear guidance for future migration.
163-163: LGTM! Migration instructions link is helpful.The updated deprecation notice directs users to comprehensive migration guidance.
170-190: LGTM! Mode implementation correctly addresses issue #949.The case-based flow properly distinguishes between the two modes:
- check-lockfile-is-cross-platform: Pure validation that exits immediately without regenerating (red error on failure)
- regenerate-lockfile-if-some-platform-missed: Check with fallback that prints a warning and proceeds to regenerate when needed (yellow warning)
This fixes the core issue where the old mode failed to short-circuit and unintentionally regenerated the lockfile. The explicit
if-then-fistructure is clearer than a one-liner and makes the exit conditions more noticeable.
198-201: LGTM! Error handling with proper arithmetic comparison.Uses
-nefor arithmetic comparison (addressing past review feedback) and provides helpful guidance linking to the terraform_validate hook documentation. The grammar of the error message is clear and correct.
203-204: LGTM! Exit code is properly propagated.The comment clearly documents that the exit code is returned to
common::per_dir_hook, and the implementation correctly propagates the result.
# [1.105.0](v1.104.1...v1.105.0) (2026-01-06) ### Features * **`terraform_providers_lock`:** Fix logical issue in hook modes: Add `check-lockfile-is-cross-platform` and `regenerate-lockfile-if-some-platform-missed` modes. Last one is same as `only-check-is-current-lockfile-cross-platform`, which now is deprecated. Check README for more details ([#950](#950)) ([95a52e3](95a52e3))
|
This PR is included in version 1.105.0 🎉 |
Put an
xinto the box if that apply:Description of your changes
check-lockfile-is-cross-platformmode, that just check is there rihgt number of SHAs for quick check.only-check-is-current-lockfile-cross-platformtoregenerate-lockfile-if-some-platform-missedto better describe current behavior.only-check-is-current-lockfile-cross-platformflag--modeflagsFixes #949
Related #54
How can we test changes
Run one by one: