-
Notifications
You must be signed in to change notification settings - Fork 127
[WIP] Reduce run layers, fix linter errors #1
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the cleanup. We should strike a balance of reducing bloat but also making a Docker file for noobs that's easy to follow.
Left some initial comments. What more do you thin we can do?
builder/Dockerfile
Outdated
# Install utilities | ||
RUN apt-get update && apt-get install -y $deps \ | ||
--no-install-recommends \ | ||
&& curl -sSL https://deb.nodesource.com/setup_6.x | bash - \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is getting gnarly. Can we keep the && on the previous lines so it's easier to parse each command?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The && style on the bottom line is the notation in the Best Practices doc (https://docs.docker.com/engine/userguide/eng-image/dockerfile_best-practices/), but the && on previous line is a-okay by me. It's on my list to make consistent in this PR.
builder/Dockerfile
Outdated
&& echo "deb [arch=amd64] https://dl.google.com/linux/chrome/deb/ stable main" > /etc/apt/sources.list.d/google-chrome.list \ | ||
&& apt-get update && apt-get install -y \ | ||
google-chrome-stable \ | ||
nodejs \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
apt-get install -y google-chrome-stable nodejs --no-install-recommends
on it's own line
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is just the style in the RUN section (see https://docs.docker.com/engine/userguide/eng-image/dockerfile_best-practices/#run), I've used both in practice. Can go single.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's cool. Then, let's stick w/ Docker's style.
builder/Dockerfile
Outdated
nodejs \ | ||
--no-install-recommends \ | ||
&& npm --global install yarn \ | ||
&& apt-get purge --auto-remove -y curl gnupg \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you add comments on these last two lines?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
builder/Dockerfile
Outdated
sudo adduser chromeuser sudo | ||
# Add Chrome as a user | ||
RUN groupadd -r chrome && useradd -r -g chrome -G audio,video chrome \ | ||
&& mkdir -p /home/chrome/Downloads && chown -R chrome:chrome /home/chrome |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do we need mkdir -p /home/chrome/Downloads
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's my bad; we won't need downloads in this case. Will remove.
Agreed; the balance needs to be there and I believe I can make this a little more cleaner for people just coming into it. Should have some time this evening, I'll push another commit with revisions for you to have a look at. |
This is first pass to reduce some of the layers in the docker build process to help reduce build problems. In particular we don't want to use
sudo
and we don't want to runapt-get upgrade
.This currently builds and runs, but I'll probably work on this pull some more before it might be considered for merge.