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

[acodeninja]

• 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 -->

[yermulnik]

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.

[acodeninja]

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.

[yermulnik]

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

[acodeninja]

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

[yermulnik]

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:]]+ ]]

[acodeninja]

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.

[yermulnik]

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?

[yermulnik]

[[ "$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.

[acodeninja]

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

[yermulnik]

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

[MaxymVlasov]

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]

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]

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]

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]

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]

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

[antonbabenko]

This PR is included in version 1.72.0 🎉