feat: When a config file is given, do not specify formatter on cli (terraform_docs) #386

acodeninja · 2022-05-18T11:14:58Z

This PR introduces breaking change.
This PR fixes a bug.
This PR adds new functionality.
This PR enhances existing functionality.

Description of your changes

The change is to the terraform_docs hook. Currently this hook will ignore the formatter set in any specified config file, defaulting to the md formatter. On our project we use the markdown document formatter but this is ignored by the hook. I consider this a bug but thought a PR would be better than a new issue for you.

How can we test changes

Run:

$ mkdir test-tf-docs-pre-commit
$ cd test-tf-docs-pre-commit
$ git init
$ pre-commit install

Create the following files.

.terraform-docs.yml

formatter: toml

.pre-commit-config.yaml

repos:
  - repo: https://github.com/acodeninja/pre-commit-terraform.git
    rev: v1.71.0
    hooks:
      - id: terraform_docs
        args:
          - --args=--config=.terraform-docs.yml

main.tf

variable "some-input" {
  description = "An input variable"
  type        = string
}

output "some-output" {
  value = var.some-input
}

README.md

<!-- BEGINNING OF PRE-COMMIT-TERRAFORM DOCS HOOK -->

<!-- END OF PRE-COMMIT-TERRAFORM DOCS HOOK -->

Then run pre-commit run --all-files.

You will see the output in the readme after running will be:

<!-- BEGINNING OF PRE-COMMIT-TERRAFORM DOCS HOOK -->
## Requirements

No requirements.

## Providers

No providers.

## Modules

No modules.

## Resources

No resources.

## Inputs

| Name | Description | Type | Default | Required |
|------|-------------|------|---------|:--------:|
| <a name="input_some-input"></a> [some-input](#input\_some-input) | An input variable | `string` | n/a | yes |

## Outputs

| Name | Description |
|------|-------------|
| <a name="output_some-output"></a> [some-output](#output\_some-output) | n/a |
<!-- END OF PRE-COMMIT-TERRAFORM DOCS HOOK -->

This is not the configured and desired format according to the .terraform-docs.yml configuration.

Now, change the .pre-commit-config.yaml file to the following:

repos:
  - repo: https://github.com/acodeninja/pre-commit-terraform.git
    rev: 855f9c52ed0e717dd953a3cbe445bfee1cd17662
    hooks:
      - id: terraform_docs
        args:
          - --args=--config=.terraform-docs.yml

And run pre-commit run --all-files again.

You will now see the readme file contents is as expected from the configuration of terraform docs.

<!-- BEGINNING OF PRE-COMMIT-TERRAFORM DOCS HOOK -->
header = ""
footer = ""
modules = []
providers = []
requirements = []
resources = []

[[inputs]]
  name = "some-input"
  type = "string"
  description = "An input variable"
  required = true
  [inputs.default]

[[outputs]]
  name = "some-output"
  description = ""
<!-- END OF PRE-COMMIT-TERRAFORM DOCS HOOK -->

hooks/terraform_docs.sh

yermulnik · 2022-05-18T12:27:12Z

hooks/terraform_docs.sh

+  # Decide formatter to use
+  #
+  local tf_docs_formatter="md"
+  if [[ "$args" == *"--config"* ]]; then


Thanks for the contribution. Good point.
Can this if conditional be re-worked by adding check for --config arg into case block on lines 125-135 and setting some var conditionally, e.g. config_file="true", and here instead of if statement do something like [[ $config_file != "true" ]] && local tf_docs_formatter="md"? Thanks.

So I had a look at the case block which appears to be looking at the values in the $hook_config variable. This doesn't actually contain details passed in by the args property, those are contained in the $args variable. I'm happy to alter the PR, but this doesn't appear to be the best way.

Would something like this be ok?

# # Override formatter if no config file set # [[ "$args" != *"--config"* ]] && local tf_docs_formatter="md"

I can of course break this up a little and do something like:

# # Override formatter if no config file set # [[ "$args" == *"--config"* ]] && local has_config_file="true" [[ "$has_config_file" != "true" ]] && local tf_docs_formatter="md"

Let me know what you think.

Oh, my bad. I indeed misread that part of code 🤦🏻
Yep [[ "$args" != *"--config"* ]] && local tf_docs_formatter="md" looks good to me. Thanks.

It's no worries, that was actually my first solution this morning.

The conditional statement may probably be improved a bit like this to cover short opt and to ensure the match is not a part of some other opt/value and to ensure this opt has a value (not sure whether one can put = instead of between opt and its value though):

[[ ! $args =~ (^|[[:space:]])(-c|--config)[[:space:]]+[^[:space:]]+ ]]

On the short code I agree, however this currently isn't supported in any case. See line 19:

ARGS=${ARGS[*]/--config=/--config=$(pwd)\/}

The absolute path resolution only kicks in if the full option --config is set. I can include this fix in my PR, but it feels out of scope. Happy to alter as required though.

Oh, so = sign is allowed. Looks like we need to improve code on line 19 since it doesn't cover short options and space instead of equal sign 🤔 Though this indeed is out of scope of your PR. Let's pitch upon [[ ! $args =~ (^|[[:space:]])--config=[^[:space:]]+ ]] && local tf_docs_formatter="md" so that these two are aligned?

[[ "$args" != *"--config="* ]] && local tf_docs_formatter="md" looks good too if you prefer it more =) (the only thing I'd like to ask to put is equal sign so that — again — the two bits of code aligned with each other)
Thank you.

I've swapped out for the regex approach so hopefully good to go.

Co-authored-by: George L. Yermulnik <yz@yz.kiev.ua>

yermulnik

LGTM
Anyone has capacity to test this with Bash v3 please?

MaxymVlasov · 2022-05-18T14:32:15Z

Maybe we can just pin the bash version used in Mac here

pre-commit-terraform/Dockerfile

Line 177 in 51147f1

bash=~5 \

In Mac used bash 3.2

yermulnik · 2022-05-18T14:34:19Z

Anyone has capacity to test this with Bash v3 please?

So the "(..|..)" thing doesn't work in Bash v3. @acodeninja Looks like [[ "$args" != *"--config="* ]] is our best choice. Apologies for the fuss 🤷🏻

acodeninja · 2022-05-18T14:37:21Z

Anyone has capacity to test this with Bash v3 please?

So the "(..|..)" thing doesn't work in Bash v3. @acodeninja Looks like [[ "$args" != *"--config="* ]] is our best choice. Apologies for the fuss 🤷🏻

No worries, pushed an update and updated the PR heading to include the correct hash for testing.

yermulnik · 2022-05-18T14:44:30Z

Maybe we can just pin the bash version used in Mac here

I just built bash3 locally which appeared to be quickest way for me =)
Hope this will suffice for future use cases; I only need to stop forgetting that we agreed upon supporting ancient Bash for lazy Mac users =)

> ~/tmp/bash/bash --version
GNU bash, version 3.2.0(5)-beta (x86_64-unknown-linux-gnu)
Copyright (C) 2005 Free Software Foundation, Inc.

And I'm having an objection on pinning Docker to one version of Bash, while allowing and supporting another version of Bash outside Docker.

MaxymVlasov · 2022-05-18T15:10:30Z

Maybe we can just pin the bash version used in Mac here

I just built bash3 locally which appeared to be quickest way for me =) And I'm having an objection on pinning Docker to one version of Bash, while allowing and supporting another version of Bash outside Docker.

Well, not possible to install bash 3 via apk add. The oldest bash in Alpine is 4.3
https://pkgs.alpinelinux.org/packages?name=bash&branch=v3.3

yermulnik · 2022-05-18T15:29:36Z

Well, not possible to install bash 3 via apk add. The oldest bash in Alpine is 4.3

Yep, the only lucky folks to use ancient Bash are Mac addicts 🤣

MaxymVlasov

Tested, works fine

# [1.72.0](v1.71.0...v1.72.0) (2022-05-25) ### Features * When a config file is given, do not specify formatter on cli (terraform_docs) ([#386](#386)) ([962054b](962054b))

antonbabenko · 2022-05-25T12:32:25Z

This PR is included in version 1.72.0 🎉

feat: When a config file is given, do not specify formatter on cli

bc1c2f6

acodeninja requested review from MaxymVlasov and yermulnik as code owners May 18, 2022 11:14

yermulnik reviewed May 18, 2022

View reviewed changes

acodeninja and others added 3 commits May 18, 2022 13:32

unset tf_docs_formatter variable instead of setting to empty

58f26a7

Co-authored-by: George L. Yermulnik <yz@yz.kiev.ua>

Only set tf_docs_formatter if --config arg not included

00b2639

Move to using regex for config test and align with reqs for = sign

ace1460

MaxymVlasov added feature New feature or request hook/terraform_docs Bash hook labels May 18, 2022

MaxymVlasov requested a review from yermulnik May 18, 2022 13:46

yermulnik approved these changes May 18, 2022

View reviewed changes

Moved back to string contains syntax for bash v3

855f9c5

yermulnik approved these changes May 18, 2022

View reviewed changes

MaxymVlasov approved these changes May 25, 2022

View reviewed changes

MaxymVlasov requested a review from antonbabenko May 25, 2022 12:23

antonbabenko changed the title ~~feat: When a config file is given, do not specify formatter on cli~~ feat: When a config file is given, do not specify formatter on cli (terraform_docs) May 25, 2022

antonbabenko merged commit 962054b into antonbabenko:master May 25, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: When a config file is given, do not specify formatter on cli (terraform_docs) #386

feat: When a config file is given, do not specify formatter on cli (terraform_docs) #386

acodeninja commented May 18, 2022 •

edited

yermulnik May 18, 2022

acodeninja May 18, 2022

yermulnik May 18, 2022

acodeninja May 18, 2022

yermulnik May 18, 2022 •

edited

acodeninja May 18, 2022

yermulnik May 18, 2022

yermulnik May 18, 2022 •

edited

acodeninja May 18, 2022

yermulnik left a comment

MaxymVlasov commented May 18, 2022 •

edited

yermulnik commented May 18, 2022

acodeninja commented May 18, 2022

yermulnik commented May 18, 2022 •

edited

MaxymVlasov commented May 18, 2022

yermulnik commented May 18, 2022

MaxymVlasov left a comment

antonbabenko commented May 25, 2022

feat: When a config file is given, do not specify formatter on cli (terraform_docs) #386

feat: When a config file is given, do not specify formatter on cli (terraform_docs) #386

Conversation

acodeninja commented May 18, 2022 • edited

Description of your changes

How can we test changes

yermulnik May 18, 2022

Choose a reason for hiding this comment

acodeninja May 18, 2022

Choose a reason for hiding this comment

yermulnik May 18, 2022

Choose a reason for hiding this comment

acodeninja May 18, 2022

Choose a reason for hiding this comment

yermulnik May 18, 2022 • edited

Choose a reason for hiding this comment

acodeninja May 18, 2022

Choose a reason for hiding this comment

yermulnik May 18, 2022

Choose a reason for hiding this comment

yermulnik May 18, 2022 • edited

Choose a reason for hiding this comment

acodeninja May 18, 2022

Choose a reason for hiding this comment

yermulnik left a comment

Choose a reason for hiding this comment

MaxymVlasov commented May 18, 2022 • edited

yermulnik commented May 18, 2022

acodeninja commented May 18, 2022

yermulnik commented May 18, 2022 • edited

MaxymVlasov commented May 18, 2022

yermulnik commented May 18, 2022

MaxymVlasov left a comment

Choose a reason for hiding this comment

antonbabenko commented May 25, 2022

acodeninja commented May 18, 2022 •

edited

yermulnik May 18, 2022 •

edited

yermulnik May 18, 2022 •

edited

MaxymVlasov commented May 18, 2022 •

edited

yermulnik commented May 18, 2022 •

edited