-
Notifications
You must be signed in to change notification settings - Fork 0
[demo] Optimize the Dockerfile #5
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
6d13464 to
3311ba9
Compare
|
|
||
| # Install packages common to dev/prod | ||
| RUN apt-get -y update -qq && \ | ||
| gem install bundler --version 2.5.22 |
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 bundle install shows a warning that the lockfile was actually built with 2.1.4. I would suggest getting out of the image's way and just re-installing with whatever bundler version it ships with, provided that meets the app's needs.
| gem install bundler --version 2.5.22 | ||
|
|
||
| # Ignore the system's platform and only install native Ruby versions | ||
| ENV BUNDLE_FORCE_RUBY_PLATFORM=true |
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.
Setting these in environment variables is arguably a bit cleaner—I don't actually have a strong preference here. I do wonder how you all want to handle freezing the Gemfile.lock, as that has implications for when you actually want to perform updates. I think with a frozen bundle, to update dependencies you'd need to hop into the container just before the bundle-install, unfreeze, perform the update, and extract/commit the Gemfile/.lock changes. Seems like a lot of steps—not sure the best way to optimize that.
CC @BerkeleyLibrary/apps
| # Install Gems to the container's system-wide location | ||
| ENV BUNDLE_SYSTEM=true | ||
| # Prepend BFS script to PATH so you don't have to prefix with /opt/app/bin. | ||
| ENV PATH=/opt/app/bin:$PATH |
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.
It's been pretty standard practice to add our bin scripts to the front of the path. How does everyone feel about this?
| # Prepend BFS script to PATH so you don't have to prefix with /opt/app/bin. | ||
| ENV PATH=/opt/app/bin:$PATH | ||
|
|
||
| WORKDIR /opt/app |
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 workdir is common to both subsequent stages so it makes sense to set it in the base stage.
| @@ -1,58 +1,58 @@ | |||
| FROM ruby:3.3-slim AS base | |||
| USER root | |||
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.
ruby:3.3-slim runs as root by default so this directive wasn't necessary.
$ docker run --rm ruby:3.3-slim whoami
root| usermod -u 40061 -g bfs -G alma -l bfs default && \ | ||
| find / -user 1001 -exec chown -h bfs {} \; || true && \ | ||
| mkdir -p /opt/app && \ | ||
| chown -R bfs:bfs /opt/app |
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 gist of this whole diff is that it sets up the alma group/user (in that order), then the bfs group/user, and finally scaffolds the application directory and bfs's sshkey directory with the proper (tight) permissions.
Also note the alma user is not marked as a "system" user (-r is omitted). It's not necessary, and marking alma as system throws a warning about its uid exceeding SYS_MAX_UID.
|
|
||
| USER bfs | ||
| build-essential \ | ||
| make |
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.
Command continuations should be indented from the previous line so it's visually obvious that they're part of it, and not new commands.
|
|
||
| # Verify the installation | ||
| RUN bundle check && \ | ||
| bfs help |
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 previous bundle install --local was, I think, overkill for what we're trying to check here. We just want to know that the dependencies are installed and the app can run, and this combination of bundle check and running the help command should accomplish that.
Dockerfile
Outdated
|
|
||
| COPY --chown=bfs . /opt/app | ||
| useradd -u 40061 -g bfs -G alma -m bfs && \ | ||
| install -d -o bfs -g bfs -m 0700 /opt/app /home/bfs/.ssh && |
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 results in the correct config for that user:
$ docker run --rm --entrypoint id ucblib/bfs
uid=40061(bfs) gid=40061(bfs) groups=40061(bfs),40054(alma)
No description provided.