fix(terraform_docs): Fix issue and prioritize output.file setting from .terraform-docs.yml config over --hook-config=--path-to-file=
#698
Conversation
sworisbreathing
requested review from
MaxymVlasov and
yermulnik
as code owners
sworisbreathing
changed the title
|
This actually ignore any setting set in Maybe that's a good idea to prioritize |
MaxymVlasov
changed the title
terraform_docs): fix issue and prioritize .terraform-docs.yml config output.file setting over --hook-config=--path-to-file=
MaxymVlasov
changed the title
terraform_docs): fix issue and prioritize .terraform-docs.yml config output.file setting over --hook-config=--path-to-file=terraform_docs): fix issue and prioritize output.file setting from .terraform-docs.yml config over --hook-config=--path-to-file=
MaxymVlasov
changed the title
terraform_docs): fix issue and prioritize output.file setting from .terraform-docs.yml config over --hook-config=--path-to-file=terraform_docs): Fix issue and prioritize output.file setting from .terraform-docs.yml config over --hook-config=--path-to-file=
hooks/terraform_docs.sh
Outdated
| # Prioritize `.terraform-docs.yml` `output.file` over | ||
| # `--hook-config=--path-to-file=` if it set | ||
| local output_file | ||
| # Get latest non-commented `output.file` from `.terraform-docs.yml` | ||
| output_file=$(grep -A1000 -e '^output:$' "$config_file" | grep ' file:' | grep -v '#' | tail -n 1 || true) | ||
|
|
||
| if [ "$output_file" ]; then | ||
| # Extract filename from `output.file` line | ||
| text_file=$(echo "$output_file" | awk -F':' '{print $2}' | tr -d '[:space:]"' | tr -d "'") | ||
| fi |
There was a problem hiding this comment.
Done it this way to avoid introduction of new dependencies (yq)
| # Prioritize `.terraform-docs.yml` `output.file` over | ||
| # `--hook-config=--path-to-file=` if it set |
There was a problem hiding this comment.
I can't seem to understand the reasoning: ain't it better to raise error when both options are used instead of silently ignoring hook config's one? 🤔
There was a problem hiding this comment.
What I mean is that if a user provides this option to hook, then the user expects it to use this option. When there's a race condition, we should raise error or indicate incorrect setting by other mean(s) (e.g. by printing warning message that the one from YAML is used and the hook config's one is ignored).
I'm ok with either approach when this is clearly brought to user's attention and knowledge.
There was a problem hiding this comment.
Reasoning was described in #382
I'll add warning msg
There was a problem hiding this comment.
Took a read and... and didn't get it =( I'd appreciate "tl;dr" =)
It's most common that command line parameters override config file params. Which is sort of comparable with --hook-config.
Either way I'm relying on your opinion in this case 👍🏻
There was a problem hiding this comment.
I love removing code. Much more than write new one :D
Because you don't need to maintain code which not exist.
At the end, I'd like to replace as much hand-made settings as possible by 3rd-party, in this case - by terraform-docs settings. So, at the end, I'd like to deprecate and remove every --hook-config which can be replaced by terraform-docs settings, as long as it will not violate main idea of how pre-commit hooks works
There was a problem hiding this comment.
So keeping this in mind, we can clearly see that native terraform-docs settings should have precedence over --hook-config settings, as long as they not break hook execution - in last case we try explicitly disable such settings (or at least mention in docs that they shouldn't be used)
There was a problem hiding this comment.
Aha, now I think I got the point: hook config's parameter is a workaround from when tf-docs wasn't mature enough — this makes sense to me now 👍🏻 Thanks.
## [1.92.2](v1.92.1...v1.92.2) (2024-08-16) ### Bug Fixes * **`terraform_docs`:** Fix issue and prioritize `output.file` setting from `.terraform-docs.yml` config over `--hook-config=--path-to-file=` ([#698](#698)) ([9d6a22b](9d6a22b))
|
This PR is included in version 1.92.2 🎉 |
|
Wow thanks for fixing up the PR @MaxymVlasov and the quick review/merge @yermulnik and @antonbabenko. I've just upgraded to the new release and it's working great! |
Put an
xinto the box if that apply:Description of your changes
This PR explicitly overrides any output files configured in
.terraform-docs.ymlto ensure the rendered documentation is written to stdout (which the hook pipes to a file and then injects into the appropriate destination file).Fixes #697
How can we test changes
setup as per the steps to reproduce in #697 and run the hook.