Skip to content

Upgrade shellcheck 0.6.0 - #1693

Merged
vdemeester merged 2 commits into
docker:masterfrom
thaJeztah:upgrade_shellcheck_0.6.0
Mar 8, 2019
Merged

Upgrade shellcheck 0.6.0#1693
vdemeester merged 2 commits into
docker:masterfrom
thaJeztah:upgrade_shellcheck_0.6.0

Conversation

@thaJeztah

Copy link
Copy Markdown
Member

Follow-up to #1538 (decided to keep it separate in case we needed to discuss the new exclusions)

As a bonus; changing to the official, alpine-based image makes this image substantially smaller;

current master:

docker-cli-shell-validate latest 91.2MB

with official shellcheck v0.4.6 image

docker-cli-shell-validate latest 22.4MB

with official shellcheck v0.6.0 image

docker-cli-shell-validate latest 12MB

@thaJeztah

Copy link
Copy Markdown
MemberAuthor

ping @albers@tianon PTAL

# - SC2119: Use foo "$@" if function's $1 should mean script's $1.
# - SC2155: Declare and assign separately to avoid masking return values.
# - SC2206: Quote to prevent word splitting, or split robustly with mapfile or read -a.
# - SC2207: Prefer mapfile or read -a to split command output (or quote to avoid splitting).

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Disabled these for this file to ignore errors, such as;

In contrib/completion/bash/docker line 314:
COMPREPLY=( $(compgen -W "$(__docker_volumes "$@")" -- "$current") )
^-- SC2207: Prefer mapfile or read -a to split command output (or quote to avoid splitting).
In contrib/completion/bash/docker line 347:
plugins=(${plugins[@]/$del/})
^-- SC2206: Quote to prevent word splitting, or split robustly with mapfile or read -a.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I verified: Yes, these warnings really don't make sense here.

@vdemeestervdemeester left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM 🐯

@codecov-io

Copy link
Copy Markdown

Codecov Report

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

@@ Coverage Diff @@## master #1693 +/- ##
=======================================
Coverage 56.12% 56.12% =======================================
Files 306 306 Lines 21025 21025 =======================================
Hits 11800 11800 Misses 8371 8371 Partials 854 854

@codecov-io

codecov-io commented Feb 26, 2019

Copy link
Copy Markdown

Codecov Report

Merging #1693 into master will decrease coverage by <.01%.
The diff coverage is n/a.

@@ Coverage Diff @@## master #1693 +/- ##
==========================================
- Coverage 56.12% 56.12% -0.01% 
==========================================
Files 306 306 Lines 21030 21025 -5 ==========================================
- Hits 11803 11800 -3 + Misses 8373 8371 -2 
Partials 854 854

@silvin-lubeckisilvin-lubecki left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

Comment threadscripts/validate/shellcheck
This patch switches the shellcheck image to use the official image
from Docker Hub.
Note that this does not yet update shellcheck to the latest version (v0.5.x);
Shellcheck v0.4.7 added some new checks, which makes CI currently fail, so will
be done in a follow-up PR. Instead, the v0.4.6 version is used in this PR, which
is closest to the same version as was installed in the image before this change;
```
docker run --rm docker-cli-shell-validate shellcheck --version
ShellCheck - shell script analysis tool
version: 0.4.4
license: GNU General Public License, version 3
website: http://www.shellcheck.net
```
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@thaJeztah
thaJeztahforce-pushed the upgrade_shellcheck_0.6.0 branch from 238c48e to a830734CompareFebruary 26, 2019 14:11
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@thaJeztah
thaJeztahforce-pushed the upgrade_shellcheck_0.6.0 branch from a830734 to ff107b3CompareFebruary 26, 2019 14:13
@thaJeztah

Copy link
Copy Markdown
MemberAuthor

ping @albers PTAL 🤗

@albersalbers left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM

@vdemeester
vdemeester merged commit 89dd14d into docker:masterMar 8, 2019
@GordonTheTurtleGordonTheTurtle added this to the 19.03.0 milestone Mar 8, 2019
@thaJeztah
thaJeztah deleted the upgrade_shellcheck_0.6.0 branch March 9, 2019 01:30
Sign up for freeto 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.

6 participants

@thaJeztah@codecov-io@vdemeester@albers@silvin-lubecki@GordonTheTurtle