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

Run linter against shell scripts #3932

Merged
merged 10 commits into from
Jun 18, 2021
Merged

Run linter against shell scripts #3932

merged 10 commits into from
Jun 18, 2021

Conversation

xlgmokha
Copy link
Contributor

@xlgmokha xlgmokha commented Jun 17, 2021

Why is this needed?

To ensure that we follow the best practices for shell scripting described in
the Shellcheck wiki. e.g. SC2006.

What does this do?

It adds a ./bin/lint script that can be used to run the shellcheck linter
on your machine. It also adds a job to the CI workflow to ensure that the
linter checks are passing.

xref: #3917 (comment)

Screenshots

Before:

モ ./bin/lint

In ./bin/docker-dev-shell line 14:
OPTS=`getopt -o hr --long help,rebuild -n 'parse-options' -- "$@"`
^--^ SC2034: OPTS appears unused. Verify use (or export if used externally).
     ^-- SC2006: Use $(...) notation instead of legacy backticked `...`.

Did you mean:
OPTS=$(getopt -o hr --long help,rebuild -n 'parse-options' -- "$@")


In ./bin/docker-dev-shell line 15:
if [ $? != 0 ]; then
     ^-- SC2181: Check exit code directly with e.g. 'if mycmd;', not indirectly with $?.


In ./bin/docker-dev-shell line 61:
echo $RUNNING
     ^------^ SC2086: Double quote to prevent globbing and word splitting.

Did you mean:
echo "$RUNNING"


In ./hex/helpers/build line 18:
case `uname` in
     ^-----^ SC2006: Use $(...) notation instead of legacy backticked `...`.

Did you mean:
case $(uname) in

For more information:
  https://www.shellcheck.net/wiki/SC2034 -- OPTS appears unused. Verify use (...
  https://www.shellcheck.net/wiki/SC2086 -- Double quote to prevent globbing ...
  https://www.shellcheck.net/wiki/SC2006 -- Use $(...) notation instead of le...

After:

モ ./bin/lint
~/src/github.com/xlgmokha/dependabot-core [turtles-in-a-half-shellcheck]
モ echo $?
0

Type of change

  • Bug fix (non-breaking change which fixes an issue)

```plaintext
In ./hex/helpers/build line 18:
case `uname` in
     ^-----^ SC2006: Use $(...) notation instead of legacy backticked `...`.

Did you mean:
case $(uname) in
```

https://github.com/koalaman/shellcheck/wiki/SC2006
```plaintext
In ./bin/docker-dev-shell line 61:
echo $RUNNING
     ^------^ SC2086: Double quote to prevent globbing and word splitting.
```

https://github.com/koalaman/shellcheck/wiki/SC2086
```plaintext
In ./bin/docker-dev-shell line 14:
OPTS=`getopt -o hr --long help,rebuild -n 'parse-options' -- "$@"`
^--^ SC2034: OPTS appears unused. Verify use (or export if used externally).
     ^-- SC2006: Use $(...) notation instead of legacy backticked `...`.
```

https://github.com/koalaman/shellcheck/wiki/SC2006
OPTS=`getopt -o hr --long help,rebuild -n 'parse-options' -- "$@"`
# shellcheck disable=SC2034
OPTS=$(getopt -o hr --long help,rebuild -n 'parse-options' -- "$@")
# shellcheck disable=SC2181
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added the ignores above to skip the following:

モ ./bin/lint

In ./bin/docker-dev-shell line 14:
OPTS=$(getopt -o hr --long help,rebuild -n 'parse-options' -- "$@")
^--^ SC2034: OPTS appears unused. Verify use (or export if used externally).


In ./bin/docker-dev-shell line 15:
if [ $? != 0 ]; then
     ^-- SC2181: Check exit code directly with e.g. 'if mycmd;', not indirectly with $?.

For more information:
  https://www.shellcheck.net/wiki/SC2034 -- OPTS appears unused. Verify use (...
  https://www.shellcheck.net/wiki/SC2181 -- Check exit code directly with e.g...

@xlgmokha xlgmokha marked this pull request as ready for review June 17, 2021 21:44
@xlgmokha xlgmokha requested a review from a team as a code owner June 17, 2021 21:44
@xlgmokha xlgmokha enabled auto-merge June 17, 2021 22:18
Copy link
Member

@jurre jurre left a comment

Choose a reason for hiding this comment

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

I had one question about installing the new dependency in the dev container

bin/lint Show resolved Hide resolved
@xlgmokha xlgmokha merged commit cb5d1ac into dependabot:main Jun 18, 2021
@jurre
Copy link
Member

jurre commented Jun 18, 2021

Ah, ok automerge was enabled. I suppose we can add shellcheck to the dev container in a separate PR

@xlgmokha xlgmokha deleted the turtles-in-a-half-shellcheck branch June 18, 2021 18:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants