reactioncommerce / docker-base Goto Github PK

• It looks like https://github.com/just-containers/s6-overlay solves the volume owner problem in much the same general approach we do, but they read a configuration file for the directories. Interesting to study but at the moment I think fix-volumes.sh is better for us since it needs no explicit configuration. If, however, we hit issues and we think derived images will need to explicitly list their volume mounts, we might look at s6-overlay specifically their fix-attrs script

In support of a better dev experience, Install yalc (https://www.npmjs.com/package/yalc) into the container by default to be used by the package link, unlink and update scripts reactioncommerce/reaction#6266

One of the problem I faced it was I had to create another image to support npm private packages installation. So I think it would be useful change this line

for something like:

ONBUILD COPY --chown=node:node package-lock.json LICENSE* .npmrc ./

That way people can define the file .npmrc like:
engine-strict=true
//registry.npmjs.org/:_authToken=${NPM_TOKEN}

so when folks try to build a custom image they can do it by using something like this:
docker build -t $IMG:$TAG --build-arg NPM_TOKEN=TOKEN .

Hey Pete, everytime fix-volumes.sh runs it does a chown -R root:root .
all the files in my filesystem become owned by root
Fixing volume /app (before=1000:1000 after=0:0)…✓
I know that's what it says it's doing, too, but it's not expected that it would make the files on my
host owned by root, right?

The sharp binaries don't get included in the production build for 3.0.

Python is required to build libvips.

As I understand it /app is pretty much the canonical mount path. The Docker docs use this path as an example in their Best practices for writing Dockerfiles page, and it is used in an overwhelming number of blog posts and articles. We use it in many of our projects but not our base image.

This will be an easy change but needs to be done as new revisions of our images, not editing the existing dockerfile revisions.

Neither @rosshadden or I are clear on why it's necessary to keep duplicate copies of versioned files in here. Could we add to the README why that is being done?

Not sure if how we are using the "-v" suffixes aligns exactly with semver build metadata, but in the spirit of using external standards, I'd point us to point 10 of semver. And I also just think the "v" should be only used strictly as a prefix and everything after it is the version, so having another random "v" in there is weird.

The entrypoint script in the 12.14 base image requires the app to be mounted at /usr/local/src/app, and only runs npm install if this is the case.

The earlier images all ran npm install no matter what the mount path was, but after investigating why it turns out it was likely an unexpected bug. This is the relevant code from the earlier images:

if [[ -f /usr/local/src/app/yarn.lock ]]; then
  echo "(Using Yarn because there is a yarn.lock file)"
  su-exec node yarn install
else
  su-exec node npm install --no-audit
fi

It's looking for yarn.lock in the required mount path, and if not found it runs npm install regardless of where the mount path is. Based on this code it seems the expectation was to only install npm dependencies at the one mount path as well, but some of our other projects were relying on this. Thus when it was "fixed" we had regressions. https://xkcd.com/1172/

Ultimately the entrypoint script should not care where you mount your application. We should make it mount-agnostic.