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

Changes from 1 commit
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
Prev Previous commit
Next Next commit
try array
  • Loading branch information
@MaxymVlasov
MaxymVlasov committed Aug 30, 2024
commit aee5dc9a8d6625a47396f2b4797368c57cbb5f49
13 changes: 6 additions & 7 deletions hooks/terraform_docs.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,15 +42,14 @@ function replace_old_markers {

# 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''"
SED_CMD=(sed -i'')
else
# shellcheck disable=SC2089
SED_CMD="sed -i ''"
SED_CMD=(sed -i '')
fi
# shellcheck disable=SC2090
$SED_CMD -e "s/^${old_insertion_marker_begin}$/${insertion_marker_begin}/" "$file"
# shellcheck disable=SC2090
$SED_CMD -e "s/^${old_insertion_marker_end}$/${insertion_marker_end}/" "$file"
# 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