Add Thrift 0.12#16
Conversation
|
@ahawkins Could you check this PR? |
|
If you could tell me what the desired artifacts are for this container, I can probably help you optimize it down to something smaller. For example if all you need is the thrift compiler then you can further tune the build just to build the compiler. |
Yes, I need thrift command only. |
|
Try The Apache Thrift project is considering adding a Dockerfile to the project that will provide /usr/bin/thrift similar to what is done here, and making it available on apache/thrift in docker hub. |
0.12/Dockerfile
Outdated
| && make install \ | ||
| && cd / \ | ||
| && rm -rf /usr/src/thrift \ | ||
| && curl -k -sSL "https://storage.googleapis.com/golang/go1.4.linux-amd64.tar.gz" -o go.tar.gz \ |
There was a problem hiding this comment.
Apache Thrift 0.12.0 supports go 1.7 or later, I believe... what is go 1.4 doing in this repo if the goal is to provide a compiler? All that's needed is C++ for that.
There was a problem hiding this comment.
This line is copied from 0.11/Dockerfile.
There is no reason no more.
There was a problem hiding this comment.
You can remove go from the container, and you may be able to remove most of the "buildDeps" that were installed in order to build the compiler.
0.12/Dockerfile
Outdated
| && rm go.tar.gz \ | ||
| && cp go/bin/gofmt /usr/bin/gofmt \ | ||
| && rm -rf go \ | ||
| && apt-get purge -y --auto-remove $buildDeps |
There was a problem hiding this comment.
You could also add:
# Clean up
RUN rm -rf /var/cache/apt/* && \
rm -rf /var/lib/apt/lists/* && \
rm -rf /tmp/* && \
rm -rf /var/tmp/*
To clean up apt caches and other temporary stuff.
| @@ -0,0 +1,41 @@ | |||
| FROM ubuntu:18.04 | |||
There was a problem hiding this comment.
This is a large image to start from. Why not use buildpack-deps:bionic-scm ?
There was a problem hiding this comment.
A image using buildpack-deps is bigger than using ubuntu.
$ sudo docker images
REPOSITORY TAG IMAGE ID CREATED SIZE
tmp2 latest d1cc5d0e893c 10 seconds ago 317 MB # <= FROM buildpack-deps
tmp latest 2ca50fa19b8a 5 minutes ago 151.9 MB # <= FROM ubuntu:18.04
docker.io/ubuntu 18.04 20bb25d32758 6 hours ago 87.47 MB
docker.io/buildpack-deps bionic-scm 4b15e4cfafaf 3 weeks ago 247.9 MB
| && rm -rf go \ | ||
| && apt-get purge -y --auto-remove $buildDeps | ||
|
|
||
| CMD [ "thrift" ] |
There was a problem hiding this comment.
It would be better if this container did not run thrift as root.
There's a good recipe for running inside a container as a regular user in the Boost Docker Development Environment.
jeking3
left a comment
jeking3
left a comment
There was a problem hiding this comment.
I see a number of things that can be optimized and secured here...
|
Shouldn't there be CI here? |
Co-Authored-By: yuokada <callistoiv+git@gmail.com>
|
CI is repository owner task. @ahawkins Could you tell me your policy? |
|
@ahawkins , are you able to review/merge this fix? We could really use 0.12 in the Jaeger build, but we are blocked by this issue here. |
|
@yuokada Thanks for the PR. Sorry for the delay.
Yup there should be. I need to add something. |
4 participants
Overview
Resolve #17