Skip to content
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

Docker: build frontend dependencies with node and yarn #1113

Merged
merged 6 commits into from
Mar 28, 2018

Conversation

ArthurHoaro
Copy link
Member

@ArthurHoaro ArthurHoaro commented Mar 26, 2018

Based on #1072 - please only check the latest commit.

Tested locally, it seems to work fine.
Currently, it can be tested by replacing the code retrieval with:

+RUN curl -L https://github.com/ArthurHoaro/Shaarli/archive/feature/modern-front-end.tar.gz | tar xzf - \
+    && mv Shaarli-feature-modern-front-end shaarli \

@ArthurHoaro ArthurHoaro added in review docker containers & cloud labels Mar 26, 2018
@ArthurHoaro ArthurHoaro added this to the 0.10.0 milestone Mar 26, 2018
@ArthurHoaro ArthurHoaro self-assigned this Mar 26, 2018
@ArthurHoaro ArthurHoaro requested a review from virtualtam March 26, 2018 18:23
@ArthurHoaro ArthurHoaro force-pushed the docker/node-yarn-webpack branch from 86bc344 to a9bad6e Compare March 26, 2018 18:24
@@ -8,6 +8,16 @@ RUN curl -L https://github.com/shaarli/Shaarli/archive/master.tar.gz | tar xzf -
&& composer --prefer-dist --no-dev install

# Stage 2:
# - Frontend dependencies
FROM node:latest as node
RUN echo `pwd`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

debug?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🙊

@@ -8,6 +8,16 @@ RUN curl -L https://github.com/shaarli/Shaarli/archive/master.tar.gz | tar xzf -
&& composer --prefer-dist --no-dev install

# Stage 2:
# - Frontend dependencies
FROM node:latest as node
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestions:

  • use an Alpine-based image for size & speed
  • pin the major (and maybe minor?) NodeJS version

e.g. FROM node:9.9-alpine

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, good idea, the size ratio is 1/10:

node                 9.9-alpine            3e60aa6db49b        5 days ago           68.4MB
node                 latest              4885ab8871c2        5 days ago           673MB

@ArthurHoaro ArthurHoaro force-pushed the docker/node-yarn-webpack branch from a9bad6e to 94abe0a Compare March 27, 2018 17:05
Copy link
Member

@virtualtam virtualtam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Builds and runs fine!

There's a Node warning that can be easily fixed:

yarn install v1.5.1
warning package.json: No license field
warning No license field
[1/4] Resolving packages...
[2/4] Fetching packages...
info [email protected]: The platform "linux" is incompatible with this module.
info "[email protected]" is an optional dependency and failed compatibility check. Excluding it from installation.
[3/4] Linking dependencies...
[4/4] Building fresh packages...
Done in 22.29s.
yarn run v1.5.1
warning package.json: No license field

@ArthurHoaro ArthurHoaro merged commit ed6d1a7 into shaarli:master Mar 28, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docker containers & cloud in review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants