changed: run grunt only when building Docker image #317
Conversation
Dockerfile
Outdated
RUN npm install -g grunt-cli pm2 | ||
|
||
COPY . ${ENKETO_SRC_DIR} | ||
|
||
RUN npm install --production | ||
RUN npm install |
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.
I believe that this will also install dev dependencies, which we probably don't want in prod. This might be better?
RUN npm install
RUN grunt
RUN npm prune --production
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 review! I'll do this as RUN npm install && grunt && npm prune --production
to shrink the resulting image a bit. With everything on one line (and one Docker layer), this step ends up adding 602 MB according to docker history
, vs. 645 MB for npm install
, 26.5 MB for grunt
, and an additional 584 bytes (lol) for the file deletion markers added by npm prune --production
.
@jnm This PR looks OK to me spiritually. I want to hold off on a final review and merge because I'm waiting for a potentially related PR to Central. |
Hi @yanokwa, sounds fair enough. FYI, your ODK Central build will begin to fail at some point if you upgrade Enketo. I'm not sure what changed or when, but the build works okay without
However, on 2.8.1:
I've basically been getting away with this through dumb luck and didn't notice until a developer started pulling their hair out while trying to run a local instance of Enketo. |
Agreed that things break starting with 2.8.x because of the changes. I ran into the problem earlier and we build the Enketo docker file differently with Central* now. *dead link; please see https://github.com/getodk/central/blob/d5d430ae204001d23528e092d56b0586e4512394/enketo.dockerfile#L19 |
cd415a8
to
c746044
Compare
Rebased and resolved merge conflict |
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.
@jnm Sorry about the delay and the need to rebase. Sequencing Enketo releases is tricky business!
You open to adding the npm prune --production
changes we discussed? Also, any opposition to node 14, since 12 will be EOL in April.
Ah! I rebased away my addition of the Node 14 sounds great. Will include both changes here momentarily. |
…not at each container startup when applicable changes are detected. Closes #161; see #163.
c746044
to
adfd7e6
Compare
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.
And just to be explicit, before we merge, you've tried this build with KoBo and it works?
@eyelidlessness This looks good to me, but wanted a final sanity check from you. |
Yes, locally. Our big instances are still running 2.8.1. |
I don't know whether it's because I bungled up somewhere, but I'm getting a 404 on request to (Never-ending blue loading circle on opening a Kobo form) |
configuration. These volumes aren't necessary now that `grunt` is run only when the Docker image is built, not at container start up, and the mounts actually shadow the pre-built JS bundle leading to a never-ending loading spinner and a 404 fetching `enketo-webform.js`
Thank you, @amks1. I needed to remove obsolete volumes from the sample |
Thanks for all the great work on this! I haven't had a chance to personally verify it, but LGTM. If we find issues, we can follow on with more PRs. |
…not at each container startup when applicable changes are detected.
Closes enketo/enketo#1135; see enketo/enketo#1133.
Also fixes the following error seen during container start up (fixes enketo/enketo#1044 and obviates #338):