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

Dockerfile optimized #275

Merged
merged 6 commits into from
Sep 9, 2019
Merged

Conversation

se7entyse7en
Copy link
Contributor

@se7entyse7en se7entyse7en commented Sep 3, 2019

Closes #273.
This doesn't address #272.

The Dockerfile has been modified in order to reduce the final image size. This is achieved by using multi-stage build. There are two builders:

  1. for static files,
  2. for python wheels.

The first builder compiles the static files and permits the final image to avoid installing nodejs for the production version. This also improves the usage of docker caching for this step that is really heavy.

The second builder builds the python wheels to be used in the final image. This permits to use the slim image version.

The final image depends on the build arg DEV_BUILD: if DEV_BUILD is true, then also os dev dependencies are installed, otherwise, they're not. The Makefile provides the build-dev target.

The production image size has decreased from 1.76GB to 945MB (decrease >800MB, -46%).
The development image size has decreased from 1.76GB to 1.1GB (decrease >650MB, -37.5%).


  • I have updated the CHANGELOG file according to the conventions in keepachangelog.com
  • This PR contains changes that do not require a mention in the CHANGELOG file

Signed-off-by: Lou Marvin Caraig <[email protected]>
&& pip install -r requirements.txt -r requirements-dev.txt -r requirements-extra.txt \
&& rm -rf /root/.cache/pip
&& pip install --no-index --find-links=/wheels \
-r requirements.txt -r requirements-dev.txt -r requirements-extra.txt \
Copy link
Contributor

Choose a reason for hiding this comment

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

requirements-dev are needed only for dev :)
(except db drivers which we should move to another file)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yup, this doesn't address #272. The reason is that as you said we should also reorg requirements files such as whether to store our requirements in a requirements-build.txt or whatever where we store db drivers and pyHive. I wanted to separate this PR which is more general.


COPY superset/assets superset/assets

RUN cd superset/assets \
Copy link
Contributor

Choose a reason for hiding this comment

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

This also improves the usage of docker caching for this step that is really heavy.

if you want to improve it further you can do:

# install deps and cache them
COPY superset/assets/package*.json superset/assets
RUN npm ci

# build
COPY superset/assets superset/assets
RUN npm run build

It will use docker cache instead of downloading and installing dependencies most of the time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! Done here.

@@ -14,7 +14,34 @@
# See the License for the specific language governing permissions and
# limitations under the License.
#
FROM python:3.6-jessie
FROM node:12 AS static
Copy link
Contributor

@smacker smacker Sep 5, 2019

Choose a reason for hiding this comment

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

node version in build stage and in the final image aren't the same. Most probably it's not important but anyway better make them match.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch! Fixed here.

Copy link
Contributor

@carlosms carlosms left a comment

Choose a reason for hiding this comment

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

awesome

@se7entyse7en se7entyse7en merged commit aa02fc2 into src-d:master Sep 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[idea] Different docker images for prod and dev
3 participants