Add dockerfile for automated mentoring support #12

depsir · 2019-05-26T21:36:59Z

I created a first draft of dockerfile, inspired by the go-analyzer, having in mind the guidelines of automated-mentoring-support.
This pr is intended to fix #7

Some info about the choices made:

node version: lts
decided to run the analyzer as an unprivileged user
apk update to make the build with the latest dependencies and security fixes
two phase build, copying node_modules after yarn install --prod, to keep only runtime needed dependencies
Added certificates because it couldn't be bad
I changed /bin/bash to /bin/sh in the analyze.sh file since bash is not present in linux-alpine, probably it could be #!/usr/bin/env sh

Dockerfile

SleeplessByte

node version

LTS please 💯

go version had certificates, user and stuff. are they needed for something?

@tehsphinx might have more on that, but in general it is probably a good thing to add a special user (such as deploy or analyzer-user) that will have execution rights to node and whatsnot. The certificates I think is to make sure the certificates are in sync given:

COPY --from=builder /etc/ssl/certs/ca-certificates.crt /etc/ssl/certs/
COPY --from=builder /etc/passwd /etc/passwd

should I apk update?

Let's ask @ErikSchierboom and @tehsphinx

the two phase build should help keeping the final image small, so I wanted to copy only bin and dist. I had to copy node_modules too since the shell script needs the module esm. This could be fixed by copying only that module, or by creating an ad hoc package.json just to setup the final image (or we can just keep the build image as final image)

You may vendor package esm. Preferably by:

add it to devDependencies and NOT dependencies
copy it from node_modules during build / package

I changed /bin/bash to /bin/sh in the analyze.sh file since bash is not present in linux-alpine

Fine with me!

ErikSchierboom · 2019-05-27T10:05:35Z

Let's ask @ErikSchierboom

What exactly is the question? (sorry for not understanding)

NobbZ · 2019-05-27T10:27:41Z

What exactly is the question? (sorry for not understanding)

I think it is whether one should do an apk update in the containers.

Personally I do not hink its necessary. We can and should assume that FROM alpine:latest produces already a base image that us current (enough), honestly, it contains so little of base packages, that there usually is not much to update…

The index, which was the main reason for doing apk update in the "old days", is nowadays updated inmen when doing apk add --no-cache.

Doing apk update wrong might cause additional diskspace needed by the image.

SleeplessByte · 2019-05-27T11:25:19Z

@NobbZ thank you for your elaborate answer and yes @ErikSchierboom what he said was the question!

ZapAnton · 2019-05-27T13:26:21Z

Dockerfile

+
+# Create appuser
+#RUN adduser -D -g '' appuser && mkdir /go-analyzer
+RUN mkdir /javascript-analyzer


Since the WORKDIR command is used later, the RUN mkdir can be removed - WORKDIR creates the directory if it does not exist

ZapAnton · 2019-05-27T13:29:49Z

bin/analyze.sh

@@ -1,4 +1,4 @@
-#!/bin/bash
+#!/bin/sh


To make the script more cross-platform, perhaps the #!/usr/bin/env sh could be used here?

Will that not conflict when using multiple arguments (https://stackoverflow.com/a/16365367)?

Checked on my machine, no problems with passing several arguments to the script - $1 and $2 work as intended

Plus I am using the #!/usr/bin/env sh in my Docker image, and it works there as well

ZapAnton · 2019-05-27T13:39:07Z

.dockerignore

@@ -0,0 +1,3 @@
+test
+dist
+node_modules


Since the git commands are not used in the Dockerfile, it would make sense to add the .git directory here.
This way the Docker cache will not be invalidated when the git commands are run on the developer machine

ZapAnton · 2019-05-27T13:48:14Z

Dockerfile

+COPY --from=builder /javascript-analyzer/bin /opt/analyzer/bin
+COPY --from=builder /javascript-analyzer/dist /opt/analyzer/dist
+COPY --from=builder /javascript-analyzer/node_modules /opt/analyzer/node_modules
+RUN chmod +x /opt/analyzer/bin/analyze.sh


If storing executable scripts is OK for you, the chmod could be run on the repository file.
Docker saves the file permissions when they are copied.
Then this line can be removed.

This can be removed @depsir . This was fixed (#1)

ZapAnton · 2019-05-27T14:05:32Z

Dockerfile

+WORKDIR /javascript-analyzer
+
+# get the rest of the source code
+COPY . /javascript-analyzer


This can be simplified to the COPY . .. You are already in the javascript-analyzer directory, because of the previous WORKDIR command.

tehsphinx · 2019-05-27T14:21:54Z

About the certificates and the user:
I'm not a docker security expert but every time I see some example about a secure Go container from scratch it creates a user and adds certificates. So basically it is about running as a pretty much unprivileged user.

tehsphinx · 2019-05-27T14:25:36Z

About apk update: Probably both is fine:

Doing apk update AND using a 2 step build for the container (then it does not affect image size)
Leaving apk update out

Again: security. If we leave it out the builds are dependent on the underlying container to bring in security updates regularly.

SleeplessByte · 2019-05-27T14:33:06Z

I personally prefer to do the apk update then, @depsir

ErikSchierboom · 2019-05-27T15:41:16Z

Regarding the apk update, I'm generally in favor of updating all dependencies where possible.

SleeplessByte · 2019-05-27T21:04:30Z

About the certificates and the user:
I'm not a docker security expert but every time I see some example about a secure Go container from scratch it creates a user and adds certificates. So basically it is about running as a pretty much unprivileged user.

@depsir per this, can we also add the certificates :) ?

SleeplessByte

I've added suggestion inline which I'll apply -- let's see if this flies. Amazing work @depsir 🚀 🚀 🚀

.dockerignore

Dockerfile

bin/analyze.sh

@SleeplessByte

* Create CODE_OF_CONDUCT (originally @SleeplessByte exercism/javascript-analyzer#5) * Add dockerfile for automated mentoring support (originally @depsir exercism/javascript-analyzer#12) * Initial smoke for usage of jest (originally: @SleeplessByte exercism/javascript-analyzer#17) * Apply design and proper interface and structure (originally: @SleeplessByte exercism/javascript-analyzer#18) * Fixes issues in the README.md (originally: @zeckdude exercism/javascript-analyzer#19)

@SleeplessByte

* Create CODE_OF_CONDUCT (originally @SleeplessByte exercism/javascript-analyzer#5) * Add dockerfile for automated mentoring support (originally @depsir exercism/javascript-analyzer#12) * Initial smoke for usage of jest (originally: @SleeplessByte exercism/javascript-analyzer#17) * Apply design and proper interface and structure (originally: @SleeplessByte exercism/javascript-analyzer#18) * Fixes issues in the README.md (originally: @zeckdude exercism/javascript-analyzer#19)

depsir added 2 commits May 26, 2019 23:28

First draft of dockerfile

2e442de

Fix typo in example

9ac6634

tejasbubane reviewed May 27, 2019

View reviewed changes

Dockerfile Outdated Show resolved Hide resolved

tejasbubane reviewed May 27, 2019

View reviewed changes

Dockerfile Show resolved Hide resolved

SleeplessByte reviewed May 27, 2019

View reviewed changes

SleeplessByte assigned depsir May 27, 2019

SleeplessByte added the 🔧 chore label May 27, 2019

ZapAnton reviewed May 27, 2019

View reviewed changes

Merge branch 'master' into feature/dockerfile

ec14b91

Simplified dockerfile according to code review

528bdd9

Added certificates

5a31b62

depsir changed the title ~~[WIP] Add dockerfile for automated mentoring support~~ Add dockerfile for automated mentoring support May 29, 2019

SleeplessByte approved these changes May 30, 2019

View reviewed changes

.dockerignore Show resolved Hide resolved

Dockerfile Show resolved Hide resolved

bin/analyze.sh Show resolved Hide resolved

SleeplessByte merged commit cc01436 into exercism:master May 30, 2019

SleeplessByte mentioned this pull request Jun 4, 2019

Sync with javascript analyzer exercism/typescript-analyzer#2

Merged

		@@ -1,4 +1,4 @@
		#!/bin/bash
		#!/bin/sh

Uh oh!

Add dockerfile for automated mentoring support #12

Add dockerfile for automated mentoring support #12

Uh oh!

Conversation

depsir commented May 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

SleeplessByte left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ErikSchierboom commented May 27, 2019

Uh oh!

NobbZ commented May 27, 2019

Uh oh!

SleeplessByte commented May 27, 2019

Uh oh!

ZapAnton May 27, 2019

Choose a reason for hiding this comment

Uh oh!

ZapAnton May 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SleeplessByte May 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ZapAnton May 27, 2019

Choose a reason for hiding this comment

Uh oh!

ZapAnton May 27, 2019

Choose a reason for hiding this comment

Uh oh!

ZapAnton May 27, 2019

Choose a reason for hiding this comment

Uh oh!

ZapAnton May 27, 2019

Choose a reason for hiding this comment

Uh oh!

SleeplessByte May 27, 2019

Choose a reason for hiding this comment

Uh oh!

ZapAnton May 27, 2019

Choose a reason for hiding this comment

Uh oh!

tehsphinx commented May 27, 2019

Uh oh!

tehsphinx commented May 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SleeplessByte commented May 27, 2019

Uh oh!

ErikSchierboom commented May 27, 2019

Uh oh!

SleeplessByte commented May 27, 2019

Uh oh!

SleeplessByte left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

depsir commented May 26, 2019 •

edited

Loading

SleeplessByte left a comment •

edited

Loading

ZapAnton May 27, 2019 •

edited

Loading

SleeplessByte May 27, 2019 •

edited

Loading

tehsphinx commented May 27, 2019 •

edited

Loading