Add Dockerlint to the CI build

[toland]

Verify the Docker files using Dockerlint as part of the CI build.

https://github.com/RedCoolBeans/dockerlint

[bernardd]

Maybe I'm using it wrong, but dockerlint seems kind of...not great. Running it over Dockerfile.release yields:

ERROR: COPY contains undefined ARG or ENV variable on line 54
ERROR: Empty / bogus instruction is invalid on line 12

Now unless I'm very much mistaken, the ARG specified on line 54 is defined on line 53, and the "empty instruction"'s only crime is that it has a \ at the end of the line, meaning its contents are on the next few lines.

At this point I'm not convinced it's going to be much help.

[toland]

In the fine tradition of linting programs everywhere (cough dialyzer cough), the error messages are...not particularly enlightening. However, both issues are potentially legitimate.

Now unless I'm very much mistaken, the ARG specified on line 54 is defined on line 53

It is declared on line 53 but not defined. We define the ARG using a command-line switch, but Dockerlint can't know that. Adding a default, and thus defining the ARG, makes this message go away.

The situation is roughly analogous to this C program:

int main() {
  int i;
  return i;
}

Any linter or compiler worth its salt should warn about the use of i.

the "empty instruction"'s only crime is that it has a \ at the end of the line

It isn't warning about the command on line 12, it is warning about the comment on line 11. I assume the line number difference is due to how it is parsed. The reason for this warning is that a future version of Docker will return an error in that situation. docker build issues this warning:

[WARNING]: Empty continuation line found in:
    ENV REFRESHED_AT=2018-01-10     LANG=en_US.UTF-8     HOME=/opt/app     SHELL=/bin/sh     TERM=xterm     WOCKY_ENV=prod     WOCKY_INST=testing     WOCKY_HOST=localhost     REPLACE_OS_VARS=true
[WARNING]: Empty continuation lines will become errors in a future release.

Or it used to, at least. I can't reproduce the warning with the version of Docker currently on my laptop. At any rate, removing the comments makes the message go away.

[toland]

Interestingly, someone has opened a ticket on the dockerlint repo claiming that the behavior in the first example is a bug. See RedCoolBeans/dockerlint#52. The maintainer's response was...not what I would have expected.

[bernardd]

The situation is roughly analogous to this C program:

It's not really, though. The whole point of ARG is that you can supply it at runtime. That's not the case with i. Docker itself will output a warning if you don't supply it. Seems to me a perfectly reasonable use case to deliberately leave it out when there's no sensible default and any user is required to supply it.

At any rate, removing the comments makes the message go away.

Too bad if we wanted to document behaviour, I guess ;)

[bernardd]

The maintainer's response was...not what I would have expected.

I have no idea what he was even getting at there...the patch looks utterly unrelated.

[toland]

Docker itself will output a warning if you don't supply it.

Are you sure about that?

~/Projects/dockertest 1187ms
❯ cat Dockerfile
FROM alpine:3.7

ARG FOO

RUN [[ -n "$FOO" ]]

~/Projects/dockertest 1287ms
❯ docker --version
Docker version 18.03.1-ce, build 9ee9f40

~/Projects/dockertest
❯ docker build . -f Dockerfile --build-arg FOO=bar
Sending build context to Docker daemon  2.048kB
Step 1/3 : FROM alpine:3.7
 ---> 3fd9065eaf02
Step 2/3 : ARG FOO
 ---> Using cache
 ---> c266ec944ae5
Step 3/3 : RUN [[ -n "$FOO" ]]
 ---> Using cache
 ---> e6a4809208dd
Successfully built e6a4809208dd

~/Projects/dockertest
❯ docker build . -f Dockerfile
Sending build context to Docker daemon  2.048kB
Step 1/3 : FROM alpine:3.7
 ---> 3fd9065eaf02
Step 2/3 : ARG FOO
 ---> Using cache
 ---> c266ec944ae5
Step 3/3 : RUN [[ -n "$FOO" ]]
 ---> Running in 7fcc3e9b47a9
The command '/bin/sh -c [[ -n "$FOO" ]]' returned a non-zero code: 1

Seems to me a perfectly reasonable use case to deliberately leave it out when there's no sensible default and any user is required to supply it.

It would be, if the user was required to supply it.

Too bad if we wanted to document behaviour, I guess ;)

Indeed. I can't figure out why Docker would deprecate that behavior. The only thing I can figure is that they think you won't need it once --squash becomes common since you can just use multiple RUN or ENV statements without the current negative performance implications.

[bernardd]

Are you sure about that?

No - I was (probably foolishly) basing it on the presumption that the docs here: https://docs.docker.com/engine/reference/builder/#arg were correct :)

Edit: Whoops - totally misread those docs. Turns out they warn for the reverse case - the user specifying an argument that isn't in the file. Nevermind.

[toland]

No worries. The Docker docs can be voluminous but not especially helpful.

[toland]

Too bad if we wanted to document behaviour, I guess ;)

It turns out that it was not complaining about the presence of the comment, but about the fact that there was no line continuation character at the end of the line. So, the documentation is fine; I didn't have to delete the comments.

[toland]

There is nothing to deploy here, so closing.