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.

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
Changes from 1 commit
Commits
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
quotes
MaxymVlasov committed Aug 30, 2024
commit a76f03bfc4978741f37f79a6b4a86dc72078a78d
4 changes: 2 additions & 2 deletions README.md
Original file line number Diff line number Diff line change
@@ -586,8 +586,8 @@ Unlike most other hooks, this hook triggers once if there are any changed files

```bash
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

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

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

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/'
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/'
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/'
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/'
```

```yaml
6 changes: 2 additions & 4 deletions hooks/terraform_docs.sh
Original file line number Diff line number Diff line change
@@ -46,10 +46,8 @@ function replace_old_markers {
else
SED_CMD=(sed -i '')
fi
# shellcheck disable=SC2068
${SED_CMD[@]} -e "s/^${old_insertion_marker_begin}$/${insertion_marker_begin}/" "$file"
# shellcheck disable=SC2068
${SED_CMD[@]} -e "s/^${old_insertion_marker_end}$/${insertion_marker_end}/" "$file"
"${SED_CMD[@]}" -e "s/^${old_insertion_marker_begin}$/${insertion_marker_begin}/" "$file"
"${SED_CMD[@]}" -e "s/^${old_insertion_marker_end}$/${insertion_marker_end}/" "$file"
}

#######################################################################
Loading