Skip to content

Fix shellcheck warnings#1271

Merged
thaJeztah merged 1 commit into
docker:masterfrom
albers:completion-shellcheck
Aug 3, 2018
Merged

Fix shellcheck warnings#1271
thaJeztah merged 1 commit into
docker:masterfrom
albers:completion-shellcheck

Conversation

@albers
Copy link
Copy Markdown
Collaborator

@albers albers commented Aug 3, 2018

Fixed some problems identified by shellcheck.

Signed-off-by: Harald Albers <github@albersweb.de>
@albers albers force-pushed the completion-shellcheck branch from 6a0194f to e587ec2 Compare August 3, 2018 13:28
@codecov-io
Copy link
Copy Markdown

Codecov Report

Merging #1271 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #1271   +/-   ##
=======================================
  Coverage   54.29%   54.29%           
=======================================
  Files         268      268           
  Lines       17855    17855           
=======================================
  Hits         9695     9695           
  Misses       7550     7550           
  Partials      610      610

Copy link
Copy Markdown
Contributor

@silvin-lubecki silvin-lubecki left a comment

Choose a reason for hiding this comment

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

LGTM. I don't know shellcheck, but do you think we could add a CI check which fails if it complains?

@albers
Copy link
Copy Markdown
Collaborator Author

albers commented Aug 3, 2018

@silvin-lubecki We already have this check, it's the ci/circleci: shellcheck check that's always executed in this repo. You can manually execute it with make -f docker.Makefile shellcheck.

Not all shellcheck recommendations make sense in the context of bash completion, e.g. we construct awk commands that contain a literal $1. Shellcheck warns us that in '$1' the $1 won't be expanded and we should use double quotes instead, which is not what we want. So we had to add some shellcheck directives in order to suppress the false positives.

When the checks were introduced in #266, quite a lot of exceptions were added in order to get it running. In this PR I remove some of them.

Copy link
Copy Markdown
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!!

@thaJeztah thaJeztah merged commit b5768be into docker:master Aug 3, 2018
@GordonTheTurtle GordonTheTurtle added this to the 18.09.0 milestone Aug 3, 2018
@albers albers deleted the completion-shellcheck branch August 4, 2018 08:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants