[AIRFLOW-3906] Add npm compile to docker file#4724
Conversation
Codecov Report
@@ Coverage Diff @@
## master #4724 +/- ##
==========================================
+ Coverage 74.29% 74.44% +0.15%
==========================================
Files 450 450
Lines 28970 28970
==========================================
+ Hits 21522 21567 +45
+ Misses 7448 7403 -45
Continue to review full report at Codecov.
|
|
@verdan PTAL |
|
Do we want to add the node runtime to the image, or just the compiled assets? If just the assets, this might be a good use case for multi-stage builds. |
|
@jmcarp You know this exists? https://cwiki.apache.org/confluence/display/AIRFLOW/AIP-10+Multi-layered+official+Airflow+image @Fokko I this is ready for merge now |
zhongjiajie
left a comment
There was a problem hiding this comment.
Could we set env in L19 rather than L55?
|
|
||
| WORKDIR /opt/airflow/airflow/www | ||
| RUN npm install \ | ||
| && npm run prod |
There was a problem hiding this comment.
-1 to doing it this way: Having Node and all of the node_modules in the final image isn't great.
Instead we should use a multi-stage docker file so this line becomes:
COPY --from=assert-builder /opt/airflow/airflow/www/static/dist /opt/airflow/airflow/www/static/dist
And then earlier in the docker file we have (i.e. right at the start)
FROM node as asset-builder
COPY ./airflow/www/package*json /opt/airflow/airflow/www/package.json
RUN npm install
COPY ./airflow/www/static/ /opt/airflow/airflow/www/static/
RUN npm run prod
Fokko
left a comment
There was a problem hiding this comment.
Thanks @ffinfo for picking this up.
To avoid a scope creep, I'll merge this one, and I've created a new ticket for turning this into a multi stage build: https://issues.apache.org/jira/browse/AIRFLOW-3976
I would like merge this so I have a working UI again.
Make sure you have checked all steps below.
Jira
Description
Adding nom, compile step to Dockerfile
Tests
Commits
Documentation
Code Quality
flake8