feat: Add support of __GIT_WORKING_DIR__ placeholder for all hooks#945
Conversation
Move `__GIT_WORKING_DIR__` permutation feature over right into `common::parse_cmdline` function so that it is available to all hooks that utilize `common::parse_cmdline` function. Resolves #944
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughSummary by CodeRabbit
WalkthroughCentralized runtime replacement of the Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant CLI as pre-commit CLI
participant Common as common::parse_cmdline
participant Hook as terraform_* hook
rect #f0f9ff
CLI->>Common: invoke hook with --args (may include __GIT_WORKING_DIR__)
note right of Common: Replace __GIT_WORKING_DIR__ → $PWD\nBuild ARGS, FILES, HOOK_ID
Common-->>CLI: return parsed ARGS, FILES, HOOK_ID
end
rect #f6fff0
CLI->>Hook: invoke hook main with ARGS, FILES
Hook->>Hook: use ARGS (already substituted)
Hook-->>CLI: run checks and return status
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
|
@GSokol Could you please verify if the 405a6d5 commit works for you? Thanks @MaxymVlasov Do you have a use case to test it too please? |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
README.md (1)
497-517: Minor formatting inconsistencies in examples.Lines 497-517 and 713-721 appear to have formatting changes (indentation/spacing). Static analysis flagged MD049 (emphasis style) at line 845 for asterisk vs. underscore usage. Ensure all code examples follow consistent formatting conventions throughout the README to maintain clarity and adherence to markdownlint rules.
Also applies to: 713-721
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
README.md(5 hunks)hooks/_common.sh(1 hunks)hooks/terraform_checkov.sh(0 hunks)hooks/terraform_tflint.sh(1 hunks)hooks/terraform_tfsec.sh(0 hunks)hooks/terraform_trivy.sh(0 hunks)
💤 Files with no reviewable changes (3)
- hooks/terraform_checkov.sh
- hooks/terraform_tfsec.sh
- hooks/terraform_trivy.sh
🧰 Additional context used
🪛 GitHub Actions: Common issues check
hooks/terraform_tflint.sh
[error] 22-22: ShellCheck SC2153: Possible misspelling: ARGS may not be assigned. Did you mean args?
🪛 LanguageTool
README.md
[grammar] ~372-~372: There might be a mistake here.
Context: ...eholder in --args. It will be replaced by the Git working directory (repo root)...
(QB_NEW_EN)
[grammar] ~375-~375: There might be a mistake here.
Context: ...ave multiple directories and want to run terraform_tflint in all of them while ...
(QB_NEW_EN)
[grammar] ~376-~376: There might be a mistake here.
Context: ...e sharing a single config file — use the __GIT_WORKING_DIR__ placeholder in the...
(QB_NEW_EN)
[grammar] ~845-~845: Use a hyphen to join words.
Context: ...arning messages, rather than just module relative paths. *Note: this requires `tf...
(QB_NEW_EN_HYPHEN)
🪛 markdownlint-cli2 (0.18.1)
README.md
55-55: Link fragments should be valid
(MD051, link-fragments)
506-506: Link text should be descriptive
(MD059, descriptive-link-text)
845-845: Emphasis style
Expected: underscore; Actual: asterisk
(MD049, emphasis-style)
845-845: Emphasis style
Expected: underscore; Actual: asterisk
(MD049, emphasis-style)
⏰ 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: 🧪 Tests / pytest@🐍3.13@windows-2025
- GitHub Check: 🧹 Linters / pre-commit@🐍3.13@ubuntu-latest
- GitHub Check: 🧪 Tests / pytest@🐍3.13@macos-13
- GitHub Check: 🧪 Tests / pytest@🐍3.9@macos-13
- GitHub Check: 🧪 Tests / pytest@🐍3.12@macos-13
- GitHub Check: 🧪 Tests / pytest@🐍3.9@windows-2025
- GitHub Check: 🧪 Tests / pytest@🐍3.10@windows-2025
- GitHub Check: 🧪 Tests / pytest@🐍3.10@macos-14
- GitHub Check: 🧪 Tests / pytest@🐍3.9@macos-14
- GitHub Check: 🧪 Tests / pytest@🐍3.12@windows-2025
🔇 Additional comments (4)
README.md (1)
370-384: Excellent documentation of the__GIT_WORKING_DIR__placeholder feature.The new section clearly explains the placeholder's purpose, provides a practical use case, and includes a concrete YAML configuration example. The placement after the environment variables section is logical and consistent with the documentation structure. Static analysis flagged minor grammar suggestions (LanguageTool), but these do not impede clarity.
hooks/_common.sh (1)
86-88: Correctly centralizes__GIT_WORKING_DIR__placeholder replacement inparse_cmdline.The logic
"${ARG//__GIT_WORKING_DIR__/$PWD}"uses proper bash parameter expansion to replace all occurrences of the placeholder with the current working directory. By centralizing this inparse_cmdline, all hooks using this function automatically benefit from the feature without per-hook duplication. The strategic placement before appending to the ARGS array ensures every argument is processed consistently.Consider verifying that edge cases like paths containing special characters (spaces, quotes) are handled correctly by downstream hooks.
hooks/terraform_tflint.sh (2)
1-77: Correctly removes per-hook__GIT_WORKING_DIR__replacement now that logic is centralized.The removal of the per-hook replacement loop aligns with the PR objective to consolidate this logic into
common::parse_cmdline. The rest of the hook's logic remains intact and will benefit from the centralized replacement in_common.sh.
22-22: ShellCheck warning SC2153 is likely a false positive.The pipeline reports a warning that
ARGSmay not be assigned (line 22:tflint --init "${ARGS[@]}"). However,ARGSis a global array declared and populated incommon::parse_cmdline(in_common.shline 55), which is sourced and called before this line. ShellCheck may not detect globals from sourced scripts. This is a known limitation.If the warning persists, you can suppress it with a shellcheck directive:
# shellcheck disable=SC2153Verify whether similar warnings appear in other hook files (terraform_checkov.sh, terraform_trivy.sh, terraform_tfsec.sh) or if this is specific to terraform_tflint.sh.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
README.md (1)
370-376: Minor grammar issue: Rephrase for clarity.Line 372 uses "It will be replaced" in a slightly awkward construction. Consider: "The placeholder will be replaced at runtime with the Git working directory (repo root)." This reads more naturally. Similarly, line 375 uses "For instance" which is acceptable but could be tightened: "For example, if you have multiple directories and want to run terraform_tflint across all of them while sharing a single config file—use the GIT_WORKING_DIR placeholder in the file path:"
- You can use `__GIT_WORKING_DIR__` placeholder in `--args`. It will be replaced - by the Git working directory (repo root) at run time. - - For instance, if you have multiple directories and want to run - `terraform_tflint` in all of them while sharing a single config file — use the - `__GIT_WORKING_DIR__` placeholder in the file path. For example: + You can use the `__GIT_WORKING_DIR__` placeholder in `--args`. At runtime, it is replaced + with the Git working directory (repo root). + + For example, if you have multiple directories and want to run `terraform_tflint` + across all of them while sharing a single config file, use the `__GIT_WORKING_DIR__` + placeholder in the file path:
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
README.md(5 hunks)
🧰 Additional context used
🪛 LanguageTool
README.md
[grammar] ~372-~372: There might be a mistake here.
Context: ...eholder in --args. It will be replaced by the Git working directory (repo root)...
(QB_NEW_EN)
[grammar] ~375-~375: There might be a mistake here.
Context: ...ave multiple directories and want to run terraform_tflint in all of them while ...
(QB_NEW_EN)
[grammar] ~376-~376: There might be a mistake here.
Context: ...e sharing a single config file — use the __GIT_WORKING_DIR__ placeholder in the...
(QB_NEW_EN)
[grammar] ~845-~845: Use a hyphen to join words.
Context: ...arning messages, rather than just module relative paths. *Note: this requires `tf...
(QB_NEW_EN_HYPHEN)
🪛 markdownlint-cli2 (0.18.1)
README.md
506-506: Link text should be descriptive
(MD059, descriptive-link-text)
845-845: Emphasis style
Expected: underscore; Actual: asterisk
(MD049, emphasis-style)
845-845: Emphasis style
Expected: underscore; Actual: asterisk
(MD049, emphasis-style)
⏰ 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-14
- GitHub Check: 🧪 Tests / pytest@🐍3.13@macos-14
- GitHub Check: 🧪 Tests / pytest@🐍3.9@macos-14
- GitHub Check: 🧪 Tests / pytest@🐍3.10@macos-14
- GitHub Check: 🧪 Tests / pytest@🐍3.12@macos-14
- GitHub Check: 🧹 Linters / pre-commit@🐍3.13@ubuntu-latest
- GitHub Check: pre-commit
🔇 Additional comments (2)
README.md (2)
55-55: The TOC link fragment is correct and requires no changes.The heading anchor generated from
### All hooks: Usage of \GIT_WORKING_DIR` placeholder in `--args`matches the TOC fragment#all-hooks-usage-of-git_working_dir-placeholder-in---args` exactly. GitHub's markdown slug rules strip backticks and special characters consistently, so the link will resolve correctly.Additionally, there is no markdown linter (markdownlint, mdl, etc.) configured in the project's
.pre-commit-config.yamlor CI/CD pipeline—only Git hooks, YAML, Bash, Python, and GitHub Actions linting are enabled. The MD051 concern does not apply.Likely an incorrect or invalid review comment.
370-384: Documentation and implementation are consistent and correct.The verification confirms:
- Pre-commit framework and Git itself guarantee that hooks execute with working directory set to the repository root
- Using
$PWDin the replacement logic (line 88 ofhooks/_common.sh) is appropriate and accurate- The README documentation statement "will be replaced by the Git working directory (repo root) at run time" correctly describes the actual behavior
- The YAML example aligns with implementation expectations
yermulnik
added this to the
Features implementing and cleanup without breaking changes milestone
That works: The following command passes with the config now: podman run --rm \
--userns=keep-id -v "$(pwd):/project:U" \
-w /project \
-u="${UID}:${GID}" \
localhost/ci-helper:latest pre-commit run --all-files |
There was a problem hiding this comment.
Pull Request Overview
This PR converts the __GIT_WORKING_DIR__ placeholder functionality from individual hook implementations to a global feature available in the common::parse_cmdline function. This makes the placeholder available to all hooks that use this common parsing function instead of being limited to specific hooks.
Key changes:
- Moved
__GIT_WORKING_DIR__replacement logic from individual hooks to the common parsing function - Removed duplicate code blocks from four different hook files
- Updated documentation to reflect the global availability of this feature
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
hooks/_common.sh |
Added __GIT_WORKING_DIR__ placeholder replacement logic to the common::parse_cmdline function |
hooks/terraform_trivy.sh |
Removed duplicate __GIT_WORKING_DIR__ replacement code and added shellcheck disable comment |
hooks/terraform_tfsec.sh |
Removed duplicate __GIT_WORKING_DIR__ replacement code |
hooks/terraform_tflint.sh |
Removed duplicate __GIT_WORKING_DIR__ replacement code and added shellcheck disable comment |
hooks/terraform_checkov.sh |
Removed duplicate __GIT_WORKING_DIR__ replacement code and added shellcheck disable comment |
README.md |
Updated documentation to describe the global __GIT_WORKING_DIR__ feature and removed hook-specific examples |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
@MaxymVlasov Please validate code change and review PR when you have a chance 🤞🏻 |
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
README.md(5 hunks)
🧰 Additional context used
🪛 LanguageTool
README.md
[grammar] ~848-~848: Use a hyphen to join words.
Context: ...arning messages, rather than just module relative paths. *Note: this requires `tf...
(QB_NEW_EN_HYPHEN)
🪛 markdownlint-cli2 (0.18.1)
README.md
509-509: Link text should be descriptive
(MD059, descriptive-link-text)
848-848: Emphasis style
Expected: underscore; Actual: asterisk
(MD049, emphasis-style)
848-848: Emphasis style
Expected: underscore; Actual: asterisk
(MD049, emphasis-style)
⏰ 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). (5)
- GitHub Check: 🧪 Tests / pytest@🐍3.13@macos-14
- GitHub Check: 🧪 Tests / pytest@🐍3.12@macos-14
- GitHub Check: 🧪 Tests / pytest@🐍3.9@macos-13
- GitHub Check: 🧪 Tests / pytest@🐍3.10@macos-13
- GitHub Check: 🧪 Tests / pytest@🐍3.13@macos-13
🔇 Additional comments (1)
README.md (1)
370-386: New section appropriately documents centralized__GIT_WORKING_DIR__replacement.The documentation is clear, includes rationale, and provides a practical example. Placement mirrors the environment variable section, maintaining consistency.
| You can specify custom arguments. E.g.: | ||
|
|
||
| For deprecated hook you need to specify each argument separately: | ||
| ```yaml | ||
| - id: terraform_checkov | ||
| args: | ||
| - --args=--quiet | ||
| - --args=--skip-check CKV2_AWS_8 | ||
| ``` | ||
|
|
||
| ```yaml | ||
| - id: checkov | ||
| args: [ | ||
| "-d", ".", | ||
| "--skip-check", "CKV2_AWS_8", | ||
| ] | ||
| ``` | ||
| Check all available arguments [here](https://www.checkov.io/2.Basics/CLI%20Command%20Reference.html). | ||
|
|
||
| 2. When you have multiple directories and want to run `terraform_checkov` in all of them and share a single config file - use the `__GIT_WORKING_DIR__` placeholder. It will be replaced by `terraform_checkov` hooks with the Git working directory (repo root) at run time. For example: | ||
| For deprecated hook you need to specify each argument separately: | ||
|
|
||
| ```yaml | ||
| - id: terraform_checkov | ||
| args: | ||
| - --args=--config-file __GIT_WORKING_DIR__/.checkov.yml | ||
| ``` | ||
| ```yaml | ||
| - id: checkov | ||
| args: [ | ||
| "-d", ".", | ||
| "--skip-check", "CKV2_AWS_8", | ||
| ] | ||
| ``` |
There was a problem hiding this comment.
Improve link text for accessibility and clarity.
At line 509, the link text "here" is not descriptive. Replace with text that indicates the destination, e.g., "Check all available checkov CLI arguments."
- Check all available arguments [here](https://www.checkov.io/2.Basics/CLI%20Command%20Reference.html).
+ Check all available [checkov CLI arguments](https://www.checkov.io/2.Basics/CLI%20Command%20Reference.html).📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| You can specify custom arguments. E.g.: | |
| For deprecated hook you need to specify each argument separately: | |
| ```yaml | |
| - id: terraform_checkov | |
| args: | |
| - --args=--quiet | |
| - --args=--skip-check CKV2_AWS_8 | |
| ``` | |
| ```yaml | |
| - id: checkov | |
| args: [ | |
| "-d", ".", | |
| "--skip-check", "CKV2_AWS_8", | |
| ] | |
| ``` | |
| Check all available arguments [here](https://www.checkov.io/2.Basics/CLI%20Command%20Reference.html). | |
| 2. When you have multiple directories and want to run `terraform_checkov` in all of them and share a single config file - use the `__GIT_WORKING_DIR__` placeholder. It will be replaced by `terraform_checkov` hooks with the Git working directory (repo root) at run time. For example: | |
| For deprecated hook you need to specify each argument separately: | |
| ```yaml | |
| - id: terraform_checkov | |
| args: | |
| - --args=--config-file __GIT_WORKING_DIR__/.checkov.yml | |
| ``` | |
| ```yaml | |
| - id: checkov | |
| args: [ | |
| "-d", ".", | |
| "--skip-check", "CKV2_AWS_8", | |
| ] | |
| ``` | |
| You can specify custom arguments. E.g.: | |
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
509-509: Link text should be descriptive
(MD059, descriptive-link-text)
🤖 Prompt for AI Agents
In README.md around lines 500 to 519, the link text "here" is not descriptive;
update the link text to a meaningful phrase like "checkov CLI arguments" so the
sentence reads: "Check all available checkov CLI arguments" with the existing
URL. Ensure you replace only the link text (not the URL) and keep surrounding
punctuation and markdown formatting intact for accessibility and clarity.
| ``` | ||
|
|
||
| 3. By default, pre-commit-terraform performs directory switching into the terraform modules for you. If you want to delegate the directory changing to the binary - this will allow tflint to determine the full paths for error/warning messages, rather than just module relative paths. *Note: this requires `tflint>=0.44.0`.* For example: | ||
| 2. By default, pre-commit-terraform performs directory switching into the terraform modules for you. If you want to delegate the directory changing to the binary - this will allow tflint to determine the full paths for error/warning messages, rather than just module relative paths. *Note: this requires `tflint>=0.44.0`.* For example: |
There was a problem hiding this comment.
Fix emphasis style and grammar issues.
Two minor style issues flagged by linting:
- Emphasis style (MD049): Use underscores instead of asterisks for consistency. Change
*Note: this requires...*to_Note: this requires..._ - Hyphenation (grammar): Use hyphen for compound adjective: "module-relative paths" instead of "module relative paths"
- 2. By default, pre-commit-terraform performs directory switching into the terraform modules for you. If you want to delegate the directory changing to the binary - this will allow tflint to determine the full paths for error/warning messages, rather than just module relative paths. *Note: this requires `tflint>=0.44.0`.* For example:
+ 2. By default, pre-commit-terraform performs directory switching into the terraform modules for you. If you want to delegate the directory changing to the binary - this will allow tflint to determine the full paths for error/warning messages, rather than just module-relative paths. _Note: this requires `tflint>=0.44.0`._ For example:📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| 2. By default, pre-commit-terraform performs directory switching into the terraform modules for you. If you want to delegate the directory changing to the binary - this will allow tflint to determine the full paths for error/warning messages, rather than just module relative paths. *Note: this requires `tflint>=0.44.0`.* For example: | |
| 2. By default, pre-commit-terraform performs directory switching into the terraform modules for you. If you want to delegate the directory changing to the binary - this will allow tflint to determine the full paths for error/warning messages, rather than just module-relative paths. _Note: this requires `tflint>=0.44.0`._ For example: |
🧰 Tools
🪛 LanguageTool
[grammar] ~848-~848: Use a hyphen to join words.
Context: ...arning messages, rather than just module relative paths. *Note: this requires `tf...
(QB_NEW_EN_HYPHEN)
🪛 markdownlint-cli2 (0.18.1)
848-848: Emphasis style
Expected: underscore; Actual: asterisk
(MD049, emphasis-style)
848-848: Emphasis style
Expected: underscore; Actual: asterisk
(MD049, emphasis-style)
🤖 Prompt for AI Agents
In README.md around line 848, fix two style issues: replace the
asterisk-emphasized phrase "*Note: this requires `tflint>=0.44.0`.*" with
underscore emphasis `_Note: this requires \`tflint>=0.44.0\`._` and change
"module relative paths" to the hyphenated "module-relative paths" so the
sentence reads with consistent Markdown emphasis and correct compound adjective
hyphenation.
MaxymVlasov
changed the title
__GIT_WORKING_DIR__ to a global scope__GIT_WORKING_DIR__ placeholder for all hooks
MaxymVlasov
deleted the
issues/944/feat_Convert__GIT_WORKING_DIR__to_a_global_scope
branch
|
This PR is included in version 1.103.0 🎉 |
Put an
xinto the box if that apply:Description of your changes
Move
__GIT_WORKING_DIR__permutation feature over right intocommon::parse_cmdlinefunction so that it is available to all hooks that utilizecommon::parse_cmdlinefunction.Resolves #944