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

using multi-stage build #27

Merged
merged 5 commits into from Sep 6, 2017

Conversation

Projects
None yet
3 participants
@sevenEng
Contributor

sevenEng commented Aug 27, 2017

sevenEng added some commits Aug 27, 2017

@mor1

This comment has been minimized.

Show comment
Hide comment
@mor1

mor1 Aug 29, 2017

Dumb question-- why the separate Dockerfile-dev and Dockerfile?

mor1 commented Aug 29, 2017

Dumb question-- why the separate Dockerfile-dev and Dockerfile?

@sevenEng

This comment has been minimized.

Show comment
Hide comment
@sevenEng

sevenEng Aug 29, 2017

Contributor

Dockerfile-dev is actually a copy-past of previous Dockerfile not using multi-stage build, the name really doesn't mean anything

Contributor

sevenEng commented Aug 29, 2017

Dockerfile-dev is actually a copy-past of previous Dockerfile not using multi-stage build, the name really doesn't mean anything

@mor1

This comment has been minimized.

Show comment
Hide comment
@mor1

mor1 Aug 29, 2017

Ah-- delete it if it's dead?

mor1 commented Aug 29, 2017

Ah-- delete it if it's dead?

@Toshbrown

This comment has been minimized.

Show comment
Hide comment
@Toshbrown

Toshbrown Sep 5, 2017

Contributor

CI builds failing

[ERROR] Bad checksum for
/home/opam/.opam/packages.dev/ppx_tools.5.0/f70ca1d07d565989a0041e312e8efc00ff2ad87e:
- 1e18ffaa90d51cba9e6d5eb7bd186637 [expected result]
- 5e1a86ff323fe772d69ca2d25abc2c9d [actual result]
This may be fixed by running opam update.

Contributor

Toshbrown commented Sep 5, 2017

CI builds failing

[ERROR] Bad checksum for
/home/opam/.opam/packages.dev/ppx_tools.5.0/f70ca1d07d565989a0041e312e8efc00ff2ad87e:
- 1e18ffaa90d51cba9e6d5eb7bd186637 [expected result]
- 5e1a86ff323fe772d69ca2d25abc2c9d [actual result]
This may be fixed by running opam update.

@sevenEng

This comment has been minimized.

Show comment
Hide comment
@sevenEng

sevenEng Sep 5, 2017

Contributor

moved all the dependencies to another container seveneng/databox-ocaml-base, maybe worth transferring it under me-box?
@jptmoore I think maybe this could also save you some hustle when trying to dockerize some components.

Contributor

sevenEng commented Sep 5, 2017

moved all the dependencies to another container seveneng/databox-ocaml-base, maybe worth transferring it under me-box?
@jptmoore I think maybe this could also save you some hustle when trying to dockerize some components.

@mor1

This comment has been minimized.

Show comment
Hide comment
@mor1

mor1 Sep 5, 2017

Not sure breaking this out is a good idea.
AIUI it's essentially caching the pre-install of a set of packages needed to support a couple of arbitrary packages (export-service and store-timeseries). Is there genuinely a set of OPAM package dependencies that we expect all Databox components to have?
Also AIUI beware of doing things like apk update in a separate RUN command from subsequent RUN commands. The way image layer caching works, when you rebuild you're likely not to rerun the apk update line. This is unlikely to be what you want I think.

mor1 commented Sep 5, 2017

Not sure breaking this out is a good idea.
AIUI it's essentially caching the pre-install of a set of packages needed to support a couple of arbitrary packages (export-service and store-timeseries). Is there genuinely a set of OPAM package dependencies that we expect all Databox components to have?
Also AIUI beware of doing things like apk update in a separate RUN command from subsequent RUN commands. The way image layer caching works, when you rebuild you're likely not to rerun the apk update line. This is unlikely to be what you want I think.

@sevenEng

This comment has been minimized.

Show comment
Hide comment
@sevenEng

sevenEng Sep 5, 2017

Contributor

True, I don't think we expect all Databox components to depend on such set of packages. For now, maybe only export-service and store-timeseries could benefit from this. What I want it to be, is a union of all the dependencies that components developed in OCaml are in need of. To serve as a caching of pre-install deps, this largely cuts down the build time, both locally and in the docker hub too, in the case of export-service, it's the time of downloading and compiling ~80 deps and for store-timeseries, it's ~120 deps.

If moving this under me-box/ is not a good idea, I think I could keep this under my own namespace then. I find it smooth the image building process quite a lot. (that or my machine is just too old and slow 😓 )

And thanks for the reminder of apk update line, I'll update it.

Contributor

sevenEng commented Sep 5, 2017

True, I don't think we expect all Databox components to depend on such set of packages. For now, maybe only export-service and store-timeseries could benefit from this. What I want it to be, is a union of all the dependencies that components developed in OCaml are in need of. To serve as a caching of pre-install deps, this largely cuts down the build time, both locally and in the docker hub too, in the case of export-service, it's the time of downloading and compiling ~80 deps and for store-timeseries, it's ~120 deps.

If moving this under me-box/ is not a good idea, I think I could keep this under my own namespace then. I find it smooth the image building process quite a lot. (that or my machine is just too old and slow 😓 )

And thanks for the reminder of apk update line, I'll update it.

@mor1

This comment has been minimized.

Show comment
Hide comment
@mor1

mor1 Sep 6, 2017

mor1 commented Sep 6, 2017

@Toshbrown Toshbrown merged commit 63a93f9 into master Sep 6, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@Toshbrown Toshbrown deleted the multi-stage branch Sep 6, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment