fix(terraform_docs): Allow having whitespaces in path to .terraform-docs.yaml config file
#796
Conversation
WalkthroughThe changes to the Changes
Sequence DiagramsequenceDiagram
participant Script as terraform_docs.sh
participant Command as terraform-docs
participant ConfigFile as .terraform-docs.yml
Script->>Script: Check for --config flag
alt Config flag present
Script->>Script: Set have_config_flag = true
Script->>Command: Execute with config flag
else No config flag
Script->>Script: Set have_config_flag = false
Script->>Command: Execute without config flag
end
Command->>ConfigFile: Read configuration (if exists)
Command-->>Script: Generate documentation
The sequence diagram illustrates the enhanced logic for handling configuration flags in the 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms (10)
🔇 Additional comments (3)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
hooks/terraform_docs.sh(3 hunks)
🧰 Additional context used
🪛 Shellcheck (0.10.0)
hooks/terraform_docs.sh
[warning] 241-241: config_arg is referenced but not assigned.
(SC2154)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: 🧪 Tests / pytest@🐍3.9@windows-2025
- GitHub Check: 🧪 Tests / pytest@🐍3.13@windows-2025
- GitHub Check: 🧪 Tests / pytest@🐍3.12@windows-2025
- GitHub Check: 🧪 Tests / pytest@🐍3.12@macos-14
- GitHub Check: 🧪 Tests / pytest@🐍3.12@macos-13
- GitHub Check: 🧪 Tests / pytest@🐍3.11@windows-2025
- GitHub Check: 🧪 Tests / pytest@🐍3.10@macos-14
- GitHub Check: 🧹 Linters / pre-commit@🐍3.13@ubuntu-latest
- GitHub Check: pre-commit
🔇 Additional comments (1)
hooks/terraform_docs.sh (1)
Line range hint
20-23: Verify path handling compatibility.The initial config path handling using
pwdmight interfere with the new whitespace handling logic. Please verify that both mechanisms work together correctly, especially with paths containing spaces.Run this script to test various path scenarios:
tarfu
changed the title
tarfu
changed the title
…uments in args string and format
MaxymVlasov
changed the title
terraform_docs): Allow having whitespaces in path to .terraform-docs.yaml config file
MaxymVlasov
added
hook/terraform_docs
There was a problem hiding this comment.
Thank you for catch and fixing this bug.
Simplest workaround - do not use spaces in path, as it's never ending good in many cases
More comprehensive bash review will be preformed by @yermulnik
Sidenote: there will be no such problems in Python hooks, but they are not ready yet.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
hooks/terraform_docs.sh (1)
242-245: Handling and passing the config option cleanly.Here you declare a local variable
config_optionsand, if the flag is set, assign it the value--config=$config_file. Then it is passed as a separate argument in theterraform-docscommand call. This approach cleanly separates the config flag from the rest of the arguments.
- Suggestion:
• For added clarity and to prevent any unintended behavior, you might explicitly initializeconfig_optionsto an empty string (e.g.local config_options="") before conditionally reassigning it.
• Verify that the quoting (using"$config_options") behaves as expected whenconfig_optionsis empty.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
hooks/terraform_docs.sh(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: 🧪 Tests / pytest@🐍3.9@windows-2025
- GitHub Check: 🧪 Tests / pytest@🐍3.13@macos-13
- GitHub Check: 🧪 Tests / pytest@🐍3.12@windows-2025
- GitHub Check: 🧪 Tests / pytest@🐍3.12@macos-14
- GitHub Check: 🧪 Tests / pytest@🐍3.10@windows-2025
🔇 Additional comments (2)
hooks/terraform_docs.sh (2)
90-90: Initialize the config flag variable appropriately.The newly introduced variable
have_config_flag=falseat line 90 is correctly defined to track the presence of the--configflag. This initialization is consistent with our overall design.
145-156: Complex extraction logic for the--configflag value.This block sets
have_config_flagto true and then uses extended globbing to extract the config file path from theargsstring. While the use ofshopt -s extgloband the parameter expansions help support both the=and whitespace-based separators, the logic is fairly complex and could be brittle in certain edge cases (e.g., paths with multiple spaces or dashes near a--token).
- Recommendation:
• Consider adding clarifying inline comments or refactoring this block into a helper function to improve readability and maintainability.
• Ensure extensive test coverage for various config path formats (paths with leading/trailing spaces, multiple internal spaces, and even cases where the path might include sequences like--).
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
hooks/terraform_docs.sh(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: 🧪 Tests / pytest@🐍3.9@windows-2025
- GitHub Check: 🧪 Tests / pytest@🐍3.9@macos-14
- GitHub Check: 🧪 Tests / pytest@🐍3.9@macos-13
- GitHub Check: 🧪 Tests / pytest@🐍3.13@windows-2025
- GitHub Check: 🧪 Tests / pytest@🐍3.13@macos-14
- GitHub Check: 🧪 Tests / pytest@🐍3.13@macos-13
- GitHub Check: 🧪 Tests / pytest@🐍3.12@macos-14
- GitHub Check: 🧪 Tests / pytest@🐍3.10@windows-2025
🔇 Additional comments (2)
hooks/terraform_docs.sh (2)
91-92: Initialization of config flag variable
The addition oflocal have_config_flag=falsecorrectly initializes a flag to track whether the--configflag is present. This is in line with the objective of handling config paths with spaces.
241-244: Conditional Insertion of Config Options
Introducing a dedicated local variableconfig_optionsand conditionally assigning it withensures that the
--configflag (with its cleaned-up value) is passed to theterraform-docscommand only when it is detected.A minor note: please verify that the variable
tf_docs_formatteris defined in all cases before its use in the command invocation, as it is used unconditionally on line 244.✅ Verification successful
I'll perform an additional search in the codebase to confirm whether
tf_docs_formatteris defined before its use. One useful approach is to search for assignments to this variable across the codebase, and particularly inhooks/terraform_docs.shbefore line 244.Please run the following script:
tf_docs_formatter is defined before its use.
- The variable is locally set to
"md"on line 143 inhooks/terraform_docs.sh.- The command on line 244 uses it after its assignment, ensuring it is always defined before use.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
[[ $have_config_flag == true ]] && config_options="--config=$config_file"Length of output: 18
Script:
#!/bin/bash echo "Searching for assignments to tf_docs_formatter across the repository:" rg -n "tf_docs_formatter=" . echo "" echo "Searching for usage and assignment in hooks/terraform_docs.sh (lines 1-244):" awk 'NR<=244' hooks/terraform_docs.sh | grep "tf_docs_formatter" || echo "No assignment found in the first 244 lines"Length of output: 647
hooks/terraform_docs.sh
Outdated
| # Enable extended pattern matching operators | ||
| shopt -qp extglob || EXTGLOB_IS_NOT_SET=true && shopt -s extglob | ||
| # Trim any args before the `--config` arg value | ||
| local config_file=${args##*--config(+([[:space:]])|=)} |
There was a problem hiding this comment.
| local config_file=${args##*--config(+([[:space:]])|=)} | |
| local config_file=${args##*--config@(+([[:space:]])|=)} |
There was a problem hiding this comment.
Thanks, added it and added back the blank line.
There was a problem hiding this comment.
Thanks.
Did you have a chance to verify the final changeset?
There was a problem hiding this comment.
If it works as expected and w/o issues, then please merge the latest changes from master and we should be good to merge this PR 👍🏻
|
Thanks for merging master :) |
MaxymVlasov
changed the title
terraform_docs): Allow having whitespaces in path to .terraform-docs.yaml config fileterraform_docs): Allow having whitespaces in path to .terraform-docs.yaml config file
## [1.97.2](v1.97.1...v1.97.2) (2025-02-03) ### Bug Fixes * **`terraform_docs`:** Allow having whitespaces in path to `.terraform-docs.yaml` config file ([#796](#796)) ([7d83911](7d83911))
|
This PR is included in version 1.97.2 🎉 |
|
this change broke our pre-commit hook for terraform_docs.. Getting the following error: v1.97.1 is working fine for us with the above config. |
* Fix bug introduced via #796 by passing config file only when it is defined * While here make array declarations in `common::parse_cmdline` in `hooks/_common.sh` compliant with Bash v3 * While here suppress error outputs from `grep` for non-existing config file in `hooks/terraform_docs.sh` where error output makes no sense
* Fix bug introduced via #796 by passing config file only when it is defined * While here make array declarations in `common::parse_cmdline` in `hooks/_common.sh` compliant with Bash v3 * While here suppress error outputs from `grep` for non-existing config file in `hooks/terraform_docs.sh` where error output makes no sense
|
This issue has been resolved in version 1.97.3 🎉 |
Put an
xinto the box if that apply:Description of your changes
This resolves an issue if the path to the config file for terraform-docs has multiple paths.
How can we test changes
Summary by CodeRabbit