Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 5 commits into from
May 25, 2022
Merged

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

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
bc1c2f6
feat: When a config file is given, do not specify formatter on cli
acodeninja May 18, 2022
58f26a7
unset tf_docs_formatter variable instead of setting to empty
acodeninja May 18, 2022
00b2639
Only set tf_docs_formatter if --config arg not included
acodeninja May 18, 2022
ace1460
Move to using regex for config test and align with reqs for = sign
acodeninja May 18, 2022
855f9c5
Moved back to string contains syntax for bash v3
acodeninja May 18, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 11 additions & 2 deletions hooks/terraform_docs.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,15 @@ function terraform_docs {
esac
done

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

@yermulnik yermulnik May 18, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator

@yermulnik yermulnik May 18, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

# Allow config file to specify formatter
tf_docs_formatter=""
acodeninja marked this conversation as resolved.
Show resolved Hide resolved
fi

local dir_path
for dir_path in $(echo "${paths[*]}" | tr ' ' '\n' | sort -u); do
dir_path="${dir_path//__REPLACED__SPACE__/ }"
Expand Down Expand Up @@ -181,7 +190,7 @@ function terraform_docs {

if [[ "$terraform_docs_awk_file" == "0" ]]; then
# shellcheck disable=SC2086
terraform-docs md $args ./ > "$tmp_file"
terraform-docs $tf_docs_formatter $args ./ > "$tmp_file"
else
# Can't append extension for mktemp, so renaming instead
local tmp_file_docs
Expand All @@ -192,7 +201,7 @@ function terraform_docs {

awk -f "$terraform_docs_awk_file" ./*.tf > "$tmp_file_docs_tf"
# shellcheck disable=SC2086
terraform-docs md $args "$tmp_file_docs_tf" > "$tmp_file"
terraform-docs $tf_docs_formatter $args "$tmp_file_docs_tf" > "$tmp_file"
rm -f "$tmp_file_docs_tf"
fi

Expand Down