Add shellcheck for bash completion by jphuynh · Pull Request #266 · docker/cli

jphuynh · 2017-06-29T13:18:53Z

Signed-off-by: Jean-Pierre Huynh jean-pierre.huynh@ounet.fr

This is the equivalent of the PR from the moby project before cli was relocated here (See: moby/moby#33260).

A new docker image has been added because of the lack of a precompiled version of shellcheck on Alpine. Installing the dependencies and compiling from source would take more build time.

Eventually it would be nice to merge the validate and shellcheck jobs to use only Dockerfile.validate instead of Dockerfile.dev

dnephin

For some reason only 2 of the 5 CI jobs ran. It looks like it doesn't have any workflows: https://circleci.com/gh/docker/workflows/cli/tree/pull%2F266

Did you setup circleCI to test your fork? I think that caused problems for me in the past.

If you disable it for your fork and force push, it should trigger a new build.

dnephin · 2017-06-29T21:13:30Z

@@ -0,0 +1,9 @@
+FROM debian:stretch-slim


Minor: could we name this file Dockerfile.shellcheck or Dockerfile.shellvalidate so that it's clear that it's different from "go validation"

Yep works for me.

dnephin · 2017-06-29T21:13:49Z

 DEV_DOCKER_IMAGE_NAME = docker-cli-dev
 LINTER_IMAGE_NAME = docker-cli-lint
 CROSS_IMAGE_NAME = docker-cli-cross
+VALIDATE_IMAGE_NAME = docker-cli-validate


s/docker-cli-validate/docker-cli-shell-validate

dnephin · 2017-06-29T21:15:05Z

+# Maintain an array of files to shellcheck not the best solution but will do for the time being
+FILES=()
+FILES+=("contrib/completion/bash/docker")
+FILES+=("scripts/validate/shellcheck")


Shouldn't we also validate everything under scripts/ ?

We should but the intent of this initial PR is to introduce shellcheck validation. I don't want it to end up like my original PR on the Moby project where it started to be very difficult to review and merge.

The idea is once we have that integrated to the build, we start adding the rest of scripts/

codecov-io · 2017-06-29T21:44:18Z

Codecov Report

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

@@           Coverage Diff           @@
##           master     #266   +/-   ##
=======================================
  Coverage   48.85%   48.85%           
=======================================
  Files         186      186           
  Lines       12413    12413           
=======================================
  Hits         6064     6064           
  Misses       5975     5975           
  Partials      374      374

dnephin · 2017-06-29T21:44:57Z

@@ -0,0 +1,9 @@
+FROM debian:stretch-slim


Looks like there are some warnings related to koalaman/shellcheck#785, but probably not an issue.

Could we use debian:jessie here?

I noticed that as well. The problem with debian:jessie is the version of Shellcheck. (See: moby/moby#33260 (comment)).

I would have to recompile from source which involves cabal-install. I'm not sure it's worth the effort but I understand those warnings annoy me as well :)

dnephin · 2017-06-29T21:45:54Z

+    apt-get -y install make shellcheck && \
+    apt-get clean
+
+WORKDIR /tmp


Minor: could we use /go/src/github.com/docker/cli here too? It just makes it easier to re-use MOUNTS in the docker.Makefile if the code is mounted at the same place in all conatiners.

dnephin

LGTM

dnephin · 2017-06-30T15:37:52Z

cc @albers

dnephin · 2017-07-06T14:47:26Z

looks like this needs a rebase

vdemeester

LGTM 🐸

dnephin · 2017-07-06T18:06:46Z

I think we can merge this, but I'd like to give @albers a chance to take a look at it first, since he's the primary contributor to the bash completion.

albers · 2017-07-07T08:38:07Z

@jphuynh How can I invoke shellcheck? I tried make -f docker.Makefile shellcheck but got error messages:

$ make -f docker.Makefile shellcheck
[...]
docker run -ti --rm -v "/home/user/go/src/github.com/docker/cli":/go/src/github.com/docker/cli docker-cli-shell-validate make shellcheck
scripts/validate/shellcheck
shellcheck: unable to decommit memory: Invalid argument
shellcheck: unable to decommit memory: Invalid argument
shellcheck: unable to decommit memory: Invalid argument

exit code was 0, though.

edit: looks like the messages are just warnings that can be ignored. If I introduce a problem, shellcheck reports it when invoked as mentioned above.

albers

I'm not familiar with the build and CI scripts, so my review is just about bash completion.

I found some minor nits, please update.

albers · 2017-07-07T08:09:54Z

 				esac
 			done
-			__docker_complete_containers_removable
+			__docker_complete_containers_removable "$@"


This addition does not make sense.

albers · 2017-07-07T08:11:00Z

 			;;
 		*)
-			__docker_complete_containers_stopped
+			__docker_complete_containers_stopped "$@"


Same as above: why did you add this?

TBH I didn't even check. It's coming from my previous PR and are leftovers of an attempt to automate certain rules on my side. It didn't work really great.

albers · 2017-07-07T08:11:16Z

-			if [ $cword -eq $counter ]; then
-				__docker_complete_containers_unpauseable
+			if [ "$cword" -eq "$counter" ]; then
+				__docker_complete_containers_unpauseable "$@"


albers · 2017-07-07T08:13:33Z

 			;;
 		--storage-driver|-s)
-			COMPREPLY=( $( compgen -W "aufs btrfs devicemapper overlay overlay2 vfs zfs" -- "$(echo $cur | tr '[:upper:]' '[:lower:]')" ) )
+			COMPREPLY=( $( compgen -W "aufs btrfs devicemapper overlay  overlay2 vfs zfs" -- "$(echo "$cur" | tr '[:upper:]' '[:lower:]')" ) )


Please remove the extra blank (probably came back in the rebase)

albers · 2017-07-07T08:17:57Z

+			if [ "$cword" -eq "$counter" ]; then
 				__docker_complete_networks
-			elif [ $cword -eq $(($counter + 1)) ]; then
+			elif [ "$cword" -eq $((counter + 1)) ]; then


To be consistent, the second expression should be quoted as well (see line 1101).

albers · 2017-07-07T08:25:39Z


 _docker_rm() {
-	_docker_container_rm
+	_docker_container_rm "$@"


see above, please remove.

albers · 2017-07-07T08:26:03Z


 _docker_start() {
-	_docker_container_start
+	_docker_container_start "$@"


see above, please remove.

albers · 2017-07-07T08:26:09Z


 _docker_unpause() {
-	_docker_container_unpause
+	_docker_container_unpause "$@"


see above, please remove.

albers · 2017-07-07T09:37:22Z

 	esac

 	local repo_print_command
+	# shellcheck disable=SC2016


I think SC2016 is so basic that we should not warn about it.
We have several computed awk expressions that all produce these warnings. Please add SC2016 to the general exclusions.

albers · 2017-07-07T09:38:35Z

@@ -1,4 +1,8 @@
 #!/usr/bin/env bash
+# shellcheck disable=SC2155


Please add a comment where to find documentation on shellcheck warnings: https://github.com/koalaman/shellcheck/wiki/SCXXXX

dnephin · 2017-07-13T15:05:47Z

Now that #206 is merged this can be updated to use the shared engine (see the new circle.yml config)

Signed-off-by: Jean-Pierre Huynh <jean-pierre.huynh@ounet.fr>

albers

Great work, @jphuynh!
Found only minor nits, please update.

albers · 2017-07-14T06:52:37Z

-			if [ $cword -eq $counter ]; then
-				__docker_complete_services_and_tasks
+			if [ "$cword" -eq "$counter" ]; then
+				__docker_complete_services_and_tasks "$@"


Please remove the argument

albers · 2017-07-14T07:59:19Z

 __docker_complete_containers_running() {
 	__docker_complete_containers "$@" --filter status=running
 }
-


Please do not remove this blank line.

Signed-off-by: Jean-Pierre Huynh <jean-pierre.huynh@ounet.fr>

albers

LGTM
Thanks very much for your patience and repeated re-work.

Some of the warnigs in bash completion can be removed through code changes, but this is out of scope of this PR. I will take care of this in a follow-up.

@thaJeztah I'd like to merge this but I haven't got permission to do so. Can I please get the required privileges? After all, this repo is my primary field of work.

[17.10] Backport version fix for Windows manifest lists

GordonTheTurtle added the status/0-triage label Jun 29, 2017

dnephin added status/1-design-review and removed status/0-triage labels Jun 29, 2017

dnephin reviewed Jun 29, 2017

View reviewed changes

dnephin mentioned this pull request Jun 30, 2017

Add bash completion for swarm ca #268

Merged

dnephin approved these changes Jun 30, 2017

View reviewed changes

dnephin added status/2-code-review and removed status/1-design-review labels Jun 30, 2017

dnephin requested a review from vdemeester July 6, 2017 14:47

dnephin requested a review from thaJeztah July 6, 2017 14:47

jphuynh force-pushed the shellcheck branch from f615e18 to e9f88de Compare July 6, 2017 16:19

vdemeester approved these changes Jul 6, 2017

View reviewed changes

albers requested changes Jul 7, 2017

View reviewed changes

albers mentioned this pull request Jul 9, 2017

Don't use extglob in bash/docker-machine.bash docker-archive-public/docker.machine#4141

Merged

jphuynh and others added 4 commits July 13, 2017 22:24

Add shellcheck for bash completion

b2fc35a

Signed-off-by: Jean-Pierre Huynh <jean-pierre.huynh@ounet.fr>

Reword to remove ambiguity with go validate

2e7d24c

Signed-off-by: Jean-Pierre Huynh <jean-pierre.huynh@ounet.fr>

Update dockerfile path for CicleCI build

501cf0e

Signed-off-by: Jean-Pierre Huynh <jean-pierre.huynh@ounet.fr>

Fix code location when mounting to docker-cli-shell-validate

ffe8509

Signed-off-by: Jean-Pierre Huynh <jean-pierre.huynh@ounet.fr>

jphuynh force-pushed the shellcheck branch 2 times, most recently from 274f02d to f708621 Compare July 13, 2017 21:36

albers requested changes Jul 14, 2017

View reviewed changes

jphuynh force-pushed the shellcheck branch from f708621 to a1d3f7b Compare July 14, 2017 08:21

Fix inconsistencies

d7748e1

Signed-off-by: Jean-Pierre Huynh <jean-pierre.huynh@ounet.fr>

jphuynh force-pushed the shellcheck branch from a1d3f7b to d7748e1 Compare July 14, 2017 08:24

albers approved these changes Jul 18, 2017

View reviewed changes

vdemeester merged commit b75596e into docker:master Jul 18, 2017

GordonTheTurtle added this to the 17.07.0 milestone Jul 18, 2017

jphuynh deleted the shellcheck branch July 21, 2017 10:33

albers mentioned this pull request Jul 26, 2017

Add file completion to bash completion for secret create #383

Merged

thaJeztah added the area/completion label Aug 3, 2018

albers mentioned this pull request Aug 3, 2018

Fix shellcheck warnings #1271

Merged

nobiit pushed a commit to nobidev/docker-cli that referenced this pull request Nov 19, 2025

Merge pull request docker#266 from johnstep/windows-manifest-lists

6f56735

[17.10] Backport version fix for Windows manifest lists

		@@ -1,4 +1,8 @@
		#!/usr/bin/env bash
		# shellcheck disable=SC2155

Conversation

jphuynh commented Jun 29, 2017

Uh oh!

dnephin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov-io commented Jun 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dnephin left a comment

Choose a reason for hiding this comment

Uh oh!

dnephin commented Jun 30, 2017

Uh oh!

dnephin commented Jul 6, 2017

Uh oh!

vdemeester left a comment

Choose a reason for hiding this comment

Uh oh!

dnephin commented Jul 6, 2017

Uh oh!

albers commented Jul 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

albers left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dnephin commented Jul 13, 2017

Uh oh!

albers left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

albers left a comment

Choose a reason for hiding this comment

Uh oh!

codecov-io commented Jun 29, 2017 •

edited

Loading

albers commented Jul 7, 2017 •

edited

Loading