Skip to content
This repository was archived by the owner on May 15, 2025. It is now read-only.

Add formatting check for shell scripts #106

Merged
merged 32 commits into from
Dec 16, 2023
Merged

Add formatting check for shell scripts #106

merged 32 commits into from
Dec 16, 2023

Conversation

matifali
Copy link
Member

@matifali matifali commented Nov 28, 2023

Context

This pull request adds a formatting check for shell scripts. It ensures that the shell scripts follow a consistent formatting style. This helps improve code readability and maintainability. The formatting check is performed using the Prettier plugin for shell scripts.

Possible issues

We must be cautious with these scripts as we populate them with variables from terraform using templatefile function. This expects a map of vars passed for all variables used as ${VAR}. To use the variables local to the script, we need to escape them with $${LOCAL_VAR}; otherwise, terraform tries to replace them and errors out as it can not find a variable with that name in the map.

We also need a method to test all module scripts using automated tests.

Suggested by @mafredi

@matifali matifali marked this pull request as ready for review November 28, 2023 13:19
@mafredri
Copy link
Member

A few sh script still seem unformatted (.sample, new.sh).

We must be cautious with these scripts as we populate them with variables from terraform using templatefile function.

I would suggest establishing a consistent practice for this. Perhaps we always prefix the inputs from terraform as TF_, TFVAR_, _MY_VAR, etc and begin each script with transferring them to shell vars:

#!/usr/bin/env

MY_VAR1=${_MY_VAR1}
MY_VAR2=${_MY_VAR2}

echo $${MY_VAR2}

Then we can add linting (basically just a script?) that after the initial assignment block, we disallow ${} to prevent errors and you must either use $${MY_VAR1} or $MY_VAR1.

Or actually, I'd like to turn that upside-down and have us post-process scripts to turn ${} into $${} automatically. This way we can write scripts normally without taking into account terraform syntax and have shell tools work as expected (e.g. shellcheck).

Also, adding shellcheck for checking script correctness could still be a big win, in addition to this.

@matifali
Copy link
Member Author

@mafredri You mean we run shellcheck on original scripts. Where do we post-process?

@matifali
Copy link
Member Author

I added shellcheck and solved the errors while suppressing warnings, info, and style checks.

@mafredri
Copy link
Member

@mafredri You mean we run shellcheck on original scripts.

Yes, shellcheck on original scripts. However, I'm pretty sure $${} breaks shellcheck, which is why we need either:

  1. Original scripts without $${} (requires a processing step so that files downloaded by terraform contain the ${} -> $${} transformation)
  2. Original scripts with $${}, copy scripts to temporary directory where we transform $${} -> ${} and run shellcheck against copies

These are just the two options I came up with, maybe there's more?

Where do we post-process?

That's a good question. I haven't looked at how these files end up in terraform so I don't know the answer to that off the top of my head.

@matifali
Copy link
Member Author

Yes, shellcheck on original scripts. However, I'm pretty sure $${} breaks shellcheck, which is why we need either:

yes. shellcheck complains that these variables are not used.

MY_VAR1=${_MY_VAR1}
MY_VAR2=${_MY_VAR2}

for this, I get self-assignment

@mafredri
Copy link
Member

I imagine we can use the # shellcheck disable=SCxxx and # shellcheck enable=SCxxx comments around the tf var assignments.

However, if the shellcheck integration is causing too much trouble, we can punt it for later.

@matifali
Copy link
Member Author

Removed shellcheck for now

@matifali matifali merged commit 1e3bd2b into main Dec 16, 2023
@matifali matifali deleted the shfmt branch December 16, 2023 14:39
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants