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

fix(terraform_docs): Fix non-GNU sed issues, introduced in v1.93.0, as previous fix doesn't work correctly #708

Merged
merged 8 commits into from
Aug 30, 2024
Merged

fix(terraform_docs): Fix non-GNU sed issues, introduced in v1.93.0, as previous fix doesn't work correctly #708

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
fa5f222
sed=''
MaxymVlasov Aug 30, 2024
74a133c
different sed per OS
MaxymVlasov Aug 30, 2024
cc5d581
deal with echo issue
MaxymVlasov Aug 30, 2024
aee5dc9
try array
MaxymVlasov Aug 30, 2024
bb402c9
Update README
MaxymVlasov Aug 30, 2024
c44f074
fix readme
MaxymVlasov Aug 30, 2024
a76f03b
quotes
MaxymVlasov Aug 30, 2024
7067081
Simplify code
MaxymVlasov Aug 30, 2024
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
9 changes: 5 additions & 4 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -585,8 +585,9 @@ Unlike most other hooks, this hook triggers once if there are any changed files
To migrate everything to `terraform-docs` insertion markers, run in repo root:

```bash
grep -rl --null 'BEGINNING OF PRE-COMMIT-TERRAFORM DOCS HOOK' . | xargs -0 sed -i'' -e 's/BEGINNING OF PRE-COMMIT-TERRAFORM DOCS HOOK/BEGIN_TF_DOCS/'
grep -rl --null 'END OF PRE-COMMIT-TERRAFORM DOCS HOOK' . | xargs -0 sed -i'' -e 's/END OF PRE-COMMIT-TERRAFORM DOCS HOOK/END_TF_DOCS/'
if sed --version > /dev/null 2>&1; then SED_CMD=(sed -i''); else SED_CMD=(sed -i ''); fi
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
if sed --version > /dev/null 2>&1; then SED_CMD=(sed -i''); else SED_CMD=(sed -i ''); fi
if sed --version &> /dev/null; then SED_CMD=("sed" "-i''"); else SED_CMD=("sed" "-i" "''"); fi

It may also make sense to prepend all sed invocations with either \ (\sed) or command (command sed) to force executing a binary from PATH rather than a function or an alias 🤔

Also maybe replace if/else with ternary (sed --version &> /dev/null && SED_CMD=("sed" "-i''") || SED_CMD=("sed" "-i" "''"))?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also probably for the GNU sed we can drop the '' bit: SED_CMD=("sed" "-i")

Copy link
Collaborator Author

@MaxymVlasov MaxymVlasov Aug 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

for Mac will be next

trace: replace_old_markers terraform_docs terraform_docs_ main main
       terraform_docs.sh:47: SED_CMD=("sed" "-i" "''")

trace: replace_old_markers terraform_docs terraform_docs_ main main
       terraform_docs.sh:49: sed -i ''\'''\''' -e 's/^<!-- BEGINNING OF PRE-COMMIT-TERRAFORM DOCS HOOK -->$/<!-- BEGIN_TF_DOCS -->/' README.md

because on Ubuntu it will create README.md'' file, I don't think that it works

sed -i''\'''\''' -e 's/^<!-- BEGINNING OF PRE-COMMIT-TERRAFORM DOCS HOOK -->$/<!-- BEGIN_TF_DOCS -->/' README.md

Copy link
Contributor

Choose a reason for hiding this comment

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

There's no need to wrap the individual array elements in quotes.

Using SED_CMD=(sed -i) makes sense for GNU sed as the '' gets dropped anyway.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

When without quotes we get what we need

trace: replace_old_markers terraform_docs terraform_docs_ main main
       terraform_docs.sh:47: SED_CMD=(sed -i '')

trace: replace_old_markers terraform_docs terraform_docs_ main main
       terraform_docs.sh:49: sed -i '' -e 's/^<!-- BEGINNING OF PRE-COMMIT-TERRAFORM DOCS HOOK -->$/<!-- BEGIN_TF_DOCS -->/' README.md

Copy link
Collaborator

@yermulnik yermulnik Aug 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

There's no need to wrap the individual array elements in quotes.

It helps to avoid empty elements to be "dropped".
It is also kinda common to wrap strings in quotes I guess.

Copy link
Collaborator

@yermulnik yermulnik Aug 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

It helps to avoid empty elements to be "dropped".

While they are still part of an array though. So nevermind me (given Max's tests show everything works as expected w/o quotes) =)

grep -rl --null 'BEGINNING OF PRE-COMMIT-TERRAFORM DOCS HOOK' . | xargs -0 ${SED_CMD[@]} -e 's/BEGINNING OF PRE-COMMIT-TERRAFORM DOCS HOOK/BEGIN_TF_DOCS/'
MaxymVlasov marked this conversation as resolved.
Show resolved Hide resolved
grep -rl --null 'END OF PRE-COMMIT-TERRAFORM DOCS HOOK' . | xargs -0 ${SED_CMD[@]} -e 's/END OF PRE-COMMIT-TERRAFORM DOCS HOOK/END_TF_DOCS/'
MaxymVlasov marked this conversation as resolved.
Show resolved Hide resolved
```

```yaml
Expand All @@ -598,7 +599,7 @@ Unlike most other hooks, this hook triggers once if there are any changed files
- --hook-config=--use-standard-markers=true # Boolean. Defaults to true (v1.93+), false (<v1.93). Set to true for compatibility with terraform-docs
```

4. You can provide [any configuration available in `terraform-docs`](https://terraform-docs.io/user-guide/configuration/) as an argument to `terraform_doc` hook, for example:
1. You can provide [any configuration available in `terraform-docs`](https://terraform-docs.io/user-guide/configuration/) as an argument to `terraform_doc` hook, for example:
MaxymVlasov marked this conversation as resolved.
Show resolved Hide resolved

```yaml
- id: terraform_docs
Expand All @@ -609,7 +610,7 @@ Unlike most other hooks, this hook triggers once if there are any changed files
> **Warning**
> Avoid use `recursive.enabled: true` in config file, that can cause unexpected behavior.

5. If you need some exotic settings, it can be done too. I.e. this one generates HCL files:
2. If you need some exotic settings, it can be done too. I.e. this one generates HCL files:
MaxymVlasov marked this conversation as resolved.
Show resolved Hide resolved

```yaml
- id: terraform_docs
Expand Down
12 changes: 10 additions & 2 deletions hooks/terraform_docs.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,8 +40,16 @@ function main {
function replace_old_markers {
local -r file=$1

sed -i'' -e "s/^${old_insertion_marker_begin}$/${insertion_marker_begin}/" "$file"
sed -i'' -e "s/^${old_insertion_marker_end}$/${insertion_marker_end}/" "$file"
# Determine the appropriate sed command based on the operating system (GNU sed or BSD sed)
if sed --version > /dev/null 2>&1; then
SED_CMD=(sed -i'')
else
SED_CMD=(sed -i '')
fi
# shellcheck disable=SC2068
${SED_CMD[@]} -e "s/^${old_insertion_marker_begin}$/${insertion_marker_begin}/" "$file"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change: "${SED_CMD[@]}" and don't disable shellcheck

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, they're both (current and suggested variants) expanded to

trace: replace_old_markers terraform_docs terraform_docs_ main main
       terraform_docs.sh:49: sed -i -e 's/^<!-- BEGINNING OF PRE-COMMIT-TERRAFORM DOCS HOOK -->$/<!-- BEGIN_TF_DOCS -->/' README.md

trace: replace_old_markers terraform_docs terraform_docs_ main main
       terraform_docs.sh:50: sed -i -e 's/^<!-- END OF PRE-COMMIT-TERRAFORM DOCS HOOK -->$/<!-- END_TF_DOCS -->/' README.md

on Ubuntu. Not to sed -i'' -e but sed -i -e 🤔

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So let's wait till anyone who face original issue confirm that it will work

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm on as Mac.

I can confirm it works with the quotes, and not without.

Test script:

old_insertion_marker_begin="Some text foo"
insertion_marker_begin="New Text"

set -x

if sed --version > /dev/null 2>&1; then
  SED_CMD=(sed -i'')
else
  SED_CMD=(sed -i '')
fi

"${SED_CMD[@]}" -e "s/^${old_insertion_marker_begin}$/${insertion_marker_begin}/" tt.txt

With quotes:

+ sed --version
+ SED_CMD=(sed -i '')
+ sed -i '' -e 's/^Some text foo$/New Text/' tt.txt

Without quotes:

+ sed --version
+ SED_CMD=(sed -i '')
+ sed -i -e 's/^Some text foo$/New Text/' tt.txt

Copy link
Contributor

Choose a reason for hiding this comment

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

If I replace sed with gsed, I get this output:

+ gsed --version
+ SED_CMD=(gsed -i'')
+ gsed -i -e 's/^Some text foo$/New Text/' tt.txt

Which works fine with GNU sed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

test this please

repos:
  - repo: https://github.com/antonbabenko/pre-commit-terraform
    rev: a76f03bfc4978741f37f79a6b4a86dc72078a78d
    hooks:
      - id: terraform_docs

Copy link
Collaborator

Choose a reason for hiding this comment

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

Please see my suggestion re wrapping array elements in double quotes.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

okay, final (I hope) solution

repos:
  - repo: https://github.com/antonbabenko/pre-commit-terraform
    rev: 70670818f5d14ba13c268dd366ecfa8374e88f42
    hooks:
      - id: terraform_docs

# shellcheck disable=SC2068
${SED_CMD[@]} -e "s/^${old_insertion_marker_end}$/${insertion_marker_end}/" "$file"
MaxymVlasov marked this conversation as resolved.
Show resolved Hide resolved
}

#######################################################################
Expand Down
Loading