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

Update all Dockerfiles to use multi-stage builds #370

Merged
merged 3 commits into from
Sep 16, 2019

Conversation

taharah
Copy link
Contributor

@taharah taharah commented Sep 13, 2019

Signed-off-by: Trevor Wood [email protected]

Fixes issue #367

Proposed Changes

  • Updates all Dockerfiles to use multi-stage builds
  • Optimized builds while preserving backwards compatibility

@yoshi-1224
Copy link
Contributor

yoshi-1224 commented Sep 13, 2019

@taharah This is a very minor thing, but could you update the message on build.sh for helm binding such that in cases where go is not available and go build did not run, rather than

[WARN] go is not available on this system -- skipping Helm binding build!
libtemplate.so built successfully

we output something like

[WARN] go is not available on this system -- skipping Helm binding build!
libtemplate.so built successfully or already exists

Just so that we or other developers don't get confused? Sorry I didn't do this in the previous PR.

Copy link
Contributor

@uberspot uberspot left a comment

Choose a reason for hiding this comment

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

Let's do another pass here and try to simplify these Docker images so maintenance is easier. :)
The initial PR looks good. 👍

Signed-off-by: Trevor Wood <[email protected]>
@uberspot
Copy link
Contributor

uberspot commented Sep 16, 2019

All looks good now, thank you for fixing the comments. :)
One minor missing dependency fails the travis build still

generating ./helm_binding.py
(already up-to-date)
chmod: cannot access './scripts/pyinstaller.sh': No such file or directory

Which I assume is because we also have to COPY ./scripts/ ./scripts/ in the dockerfiles?

Copy link
Contributor

@uberspot uberspot left a comment

Choose a reason for hiding this comment

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

LGTM! Would be good to get another approval from someone and then merge.
@ramaro or @yoshi-1224 ? :)

@uberspot uberspot requested a review from ramaro September 16, 2019 12:52
@uberspot uberspot merged commit b6f373e into kapicorp:master Sep 16, 2019
@uberspot
Copy link
Contributor

Btw, average build time dropped from ~18-19 minutes to 12-13. 👍
https://travis-ci.org/deepmind/kapitan/builds/
Also the resulting image is significantly smaller:

$ docker images | grep kapitan
deepmind/kapitan                        0.24.1                         b5a2f28f3dff        8 minutes ago       332MB
....
deepmind/kapitan                        0.24.0                         a955dc230ec1        2 weeks ago         857MB

Nice work! 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants