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

Add shellcheck support (config, workflow to detect/not fix) and address some warnings #2584

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

yarikoptic
Copy link
Contributor

More about shellcheck https://www.shellcheck.net/ .

CI workflow has 'permissions' set only to 'read' so also should be safe.

Individual commits should have more of description. One of the "largest" changes is explicit specification to use bash instead of some generic POSIX shell due to use of echo -e which according to shellcheck is not guaranteed to be supported by a POSIX shell... e.g. in dash:

❯ dash
$ echo -e "123"
-e 123

TODOs

  • verify that CI detects warning and possibly remove custom action

… defined in POSIX sh (according to shellcheck)

Also made script executable since it is a script
left unused YELLOW to simplify possible use later.
That command construct looks reliable enough to not care much
I guess it is unlikely that names would have spaces in them but
shellcheck generally right on such a recommendation and with mapfile
it would be more robust.
…nings

=== Do not change lines below ===
{
 "chain": [],
 "cmd": "sed -i -e '/# shellcheck disable.*/d' scripts/aws-force-destroy.sh",
 "exit": 0,
 "extra_inputs": [],
 "inputs": [],
 "outputs": [],
 "pwd": "."
}
^^^ Do not change lines above ^^^
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: New 🚦
Development

Successfully merging this pull request may close these issues.

1 participant