-
-
Notifications
You must be signed in to change notification settings - Fork 6.3k
Docker multi-stage #2927
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
Docker multi-stage #2927
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2927 +/- ##
=========================================
+ Coverage 35.89% 35.9% +0.01%
=========================================
Files 286 286
Lines 41312 41312
=========================================
+ Hits 14830 14835 +5
+ Misses 24300 24297 -3
+ Partials 2182 2180 -2
Continue to review full report at Codecov.
|
docker/Makefile
Outdated
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.
remove -ti?
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.
make generate can use go get that can ask the user for git access maybe ? Don't really know, I kept it as before only if some people use it for building the binary inside docker (if go is not available on host). But looking more at it we could totally remove docker-build target ?
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.
And even if this target will stay in Makefile, I think it would be reasonable to switch it to golang:1.9-alpine3.6 with content trust enabled as well.
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.
@0rzech for docker-build it is not possible golang:1.9-alpine3.6 is missing some build tools. I get them in the build stage of the Dockerfile.
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.
@sapk You're right. I forgot it misses gcc, git, make and musl-dev.
|
@appleboy maybe this should be in v1.4.0? |
|
@lunny OK. changed. |
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.
The .dockerignore file should not be removed, but repurposed to exclude anything not needed to compile the binary. Otherwise everything will be unnecessarily sent to Docker build context, thus will reduce build performance.
|
@0rzech I know this is intended to be able to build any version/tag/commit via build arg. |
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.
|
@0rzech good catch, I will clean it up and rebase on your PR(that has been merge) for content trust. |
4b009e9 to
2b2a0e4
Compare
|
@0rzech netgo tag removed. |
Dockerfile
Outdated
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 it is always better to sort packages alphabetically. Not a blocker.
Dockerfile
Outdated
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.
AFAIR, when you don't write RUN set -eu; <some other commands>, each run will exit without error. The build will fail anyways of course, but stdout will be polluted with more errors due to each subsequent command printing fails instead of first failed only. Not a blocker.
|
@sapk Thanks. I've made two more suggestions, but it is your choice whether to apply them or not. |
0rzech
left a comment
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.
LGTM
Dockerfile
Outdated
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 way gitea binary will become writable for user git. This is out of scope for this PR - I'm just writing it for future reference. Not a blocker.
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.
Are you sure ? I haven't tested but that should be root the owner and permissions should be the same as go build result.
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'm sure. It's because of this.
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.
Ok that should be fix but it was already the case before and not introduce by this PR.
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 agree. This is exactly why I wrote:
This is out of scope for this PR - I'm just writing it for future reference. Not a blocker.
;)
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.
Sorry misread, and thinking that specific line introduce that XD.
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.
No problem. ;)
379830c to
f13f14f
Compare
|
LGTM |
Dockerfile
Outdated
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.
It is a bad practice not to fix on specific version number. Besides, golang:alpine is the same as golang:1.9-alpine3.6. You can change it to alpine:3.7 and install go package with apk from Alpine's community repository.
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 know, I hesitate on that part and thinking that we it's not that bad since it is only for build.
You are rigth, I will fix to 3.7 and use in go alpine packages (1.9.2) and when golang:1.10-alpine-3.7 is release I will update. It's too bad that official image doesn't offer a golang:1.9-alpine-3.7 tag.
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.
A golang:1.9-alpine-3.7 tag should be release soon. ^^
0rzech
left a comment
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.
LGTM
Dockerfile
Outdated
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.
We wait for something, not wait of. ;) Not a blocker.
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.
Should be release monday on docker hub. Could be merge before and fix after. ^^
|
@sapk please resolve conflicts |
3fa0761 to
4fddeff
Compare
|
Codacy is complaining about maintainers because doesn't contains email. but I don't think we have a email address for it. |
|
@sapk please use |
10ee7df to
fb817e3
Compare
|
@lafriks Done + squashed and rebased |
fb817e3 to
94ca7e0
Compare
|
@sapk still fails |
|
@lafriks I can't figure out what codacy wants but it doesn't validate the format documented here : https://docs.docker.com/engine/reference/builder/#maintainer-deprecated. |
|
Maybe it does not understand new format |
Permit to do directly
docker build .in repo root. since binary is build at first stage and COPY in the exported image. Resolve #2879In addition :
I kept
make docker-buildif some people use docker to build the binary.