-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
Make frontend build in Docker image optional #4879
Conversation
Dockerfile
Outdated
|
||
COPY client /frontend/client | ||
COPY webpack.config.js /frontend/ | ||
RUN npm run build | ||
RUN if [ "x$skip_frontend_build" = "x" ] ; then npm run build; else mkdir /frontend/client/dist && touch /frontend/client/dist/{multi_org.html,index.html}; fi |
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.
Create dummy index.html
and multi_org.html
to make backend tests work.
REDASH_LOG_LEVEL: "INFO" | ||
REDASH_REDIS_URL: "redis://redis:6379/0" | ||
REDASH_DATABASE_URL: "postgresql://postgres@postgres/postgres" | ||
REDASH_RATELIMIT_ENABLED: "false" |
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 isn't directly related, but used the opportunity to reduce even more duplicates.
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 think we had to fix the scheduler container for this anyway in our recent rebase: mozilla@ce5e606
I think it's a reasonable proposal. On another note, my works care about no-include requirements_dev.txt for production build. Is it possible in this PR? By the way, Makefile is no longer update? #3038 |
No need for an arg, checking if file exists is enough. But I'm not sure about the usefulness of it. It saves the person to write a single command ( |
It would be more than just FROM redash/redash:preview
COPY requirements_ext.txt .
USER root
RUN pip install -r requirements_ext.txt
USER redash But I now realize that there is the devil in the details, since I think Maybe we should stop using webpack in favor of build-less packaging for the frontend assets like Snowpack? |
Snowpack doesn't require build during development, but you still need to build for production/deployment. |
* Add build arg to Dockerfile to control if we should build frontend assets * Move more env settings into the shared one. * Use build arg in docker-compose to skip frontend build. * CirlceCI: Skip building frontend assets in backend tests * Create dummy template files * Expand file names manually. * Add build arg to skip dev dependencies. * Update Dockerfile * Reverse logic of skip_dev_deps to what it should be.
What type of PR is this? (check all applicable)
Description
A different approach to making frontend build optional by using a build arg. While a bit more hacky, it's fully supported across different Docker versions and requires less changes.
Related Tickets & Documents
#4843, #4292