8000 Follow-up fixes on multi-stage moby's Dockerfile by vdemeester · Pull Request #36425 · moby/moby · GitHub
[go: up one dir, main page]
Skip to content

Follow-up fixes on multi-stage moby's Dockerfile #36425

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Feb 28, 2018
Merged

Follow-up fixes on multi-stage moby's Dockerfile #36425

merged 2 commits into from
Feb 28, 2018

Conversation

vdemeester
Copy link
Member
  • Do not copy golang in itself (runtime-dev is already base on golang)
  • Use golang official image as base (instead of debian:stretch and installing go manually)

This is gonna affect the cache quite a lot (because well, it's updating the base image for almost everything 😅 )

Signed-off-by: Vincent Demeester vincent@sbr.pm

@vdemeester
Do not copy golang in itself
30498c7 
Signed-off-by: Vincent Demeester <vincent@sbr.pm>
@vdemeester vdemeester mentioned this pull request Feb 27, 2018
Merged
boaz0
boaz0 approved these changes Feb 27, 2018
View reviewed changes
Copy link
Contributor
@boaz0 boaz0 left a comment

Choose a reason for hiding this comment

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

LGTM 👏

@boaz0
Copy link
Contributor
boaz0 commented Feb 27, 2018 •
edited
Loading

CI failed. On windows:

ERROR: Failed 'You cannot call a method on a null-valued expression.' at 02/27/2018 08:57:28

I have no idea what it means, though.

In janky and experimental it's due to flaky tests: DockerSwarmSuite.TestAPISwarmServicesPlugin and DockerSwarmSuite.TestAPISwarmManagerRestore

thaJeztah
thaJeztah reviewed Feb 27, 2018
View reviewed changes
Dockerfile Outdated
Copy link
Member

Choose a reason for hiding this comment

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

Wondering if we should use:

ARG GO_VERSION=1.9.3
FROM golang:$GO_VERSION

Which would also allow testing on different versions if we'd want to

Copy link
Member

Choose a reason for hiding this comment

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

Actually this means we can't test go rc's

Copy link
Member

Choose a reason for hiding this comment

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

Should work, Docker Hub also pushes images for rc's

Copy link
Member Author

Choose a reason for hiding this comment

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

@cpuguy83 to test RC's we could also "temporarly" refer to other images we build too, couldn't we ? 👼

Copy link
Member

Choose a reason for hiding this comment

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

You can't do anything out of the norm! ;)

Actually didn't realize there were rc's on hub. That's great.

Copy link
Member

Choose a reason for hiding this comment

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

We have a long tradition of pushing the RCs too: 😇

$ docker-hub-list-tags.sh library/golang | grep rc | grep -vE 'onbuild|wheezy|windowsservercore|nanoserver|cross'
rc
1.10-rc
1.10rc2
rc-stretch
1.10-rc-stretch
1.10rc2-stretch
rc-alpine
1.10-rc-alpine
1.10rc2-alpine
rc-alpine3.7
1.10-rc-alpine3.7
1.10rc2-alpine3.7
1.10rc1
1.10rc1-alpine
1.10rc1-alpine3.7
1.10rc1-stretch
1.9-rc-alpine
1.9rc2-alpine
rc-alpine3.6
1.9-rc-alpine3.6
1.9rc2-alpine3.6
1.9-rc
1.9rc2
1.9-rc-stretch
1.9rc2-stretch
1.9rc1-alpine
1.9rc1-alpine3.6
1.9rc1
1.9rc1-stretch
1.8-rc-alpine
1.8rc3-alpine
1.8-rc-stretch
1.8rc3-stretch
1.8-rc
1.8rc3
1.8rc2-alpine
1.8rc2
1.8rc1
1.8rc1-alpine
1.7rc6-alpine
1.7rc6
1.7rc5-alpine
1.7rc5
1.7rc4-alpine
1.7rc4
1.7rc3
1.7rc3-alpine
1.7rc2-alpine
1.7rc2
1.7rc1-alpine
1.7rc1
1.6rc2-alpine
1.6rc2
1.6rc1-alpine
1.6rc1
1.5rc1
1.4rc2
1.4rc1

(back to Go 1.4rc1 -- docker-library/golang#26; 2014! 😄)

thaJeztah
thaJeztah reviewed Feb 27, 2018
View reviewed changes
Dockerfile Outdated
Copy link
Member

Choose a reason for hiding this comment

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

Guess it doesn't make a difference for us, but the order is slightly different in the official Golang images (just leaving it here);

ENV GOPATH /go
ENV PATH $GOPATH/bin:/usr/local/go/bin:$PATH

cpuguy83
cpuguy83 approved these changes Feb 27, 2018
View reviewed changes
Copy link
Member
@cpuguy83 cpuguy83 left a comment

Choose a reason for hiding this comment

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

LGTM

@cpuguy83
Copy link
Member

Except CI failures. :(

@vdemeester
Copy link
Member Author
vdemeester commented Feb 27, 2018 •
edited
Loading

Oh windowsRS1 failure is real... the windowsRS1 CI script looks for ENV GO_VERSION…

    $goVersionDockerfile=$(Get-Content ".\Dockerfile" | Select-String "ENV GO_VERSION").ToString().Split(" ")[2]

I can either keep that ENV line for the time being, or update the script (but my powershell level is not that high).

@dnephin
Copy link
Member
dnephin commented Feb 27, 2018 •
edited
Loading

I think you can do $goVersionDockerfile=$(go version).ToString().Split(" ")[2], but also not super familiar with powershell. Oh, and that might have the go prefix on the version as well.

dnephin
dnephin approved these changes Feb 27, 2018
View reviewed changes
Copy link
Member
@dnephin dnephin left a comment

Choose a reason for hiding this comment

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

LGTM

thaJeztah
thaJeztah reviewed Feb 27, 2018
View reviewed changes
Dockerfile Outdated
Copy link
Member

Choose a reason for hiding this comment

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

Actually, just realised that we use the GO_VERSION env-var as well in the scripts to generate the "builder" images;

moby/contrib/builder/deb/ppc64le/generate.sh

Line 89 in 4bdb6ed

awk '$1 == "ENV" && $2 == "GO_VERSION" { print; exit }' ../../../../Dockerfile.ppc64le >> "$version/Dockerfile"

Copy link
Member Author

Choose a reason for hiding this comment

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

ohhh 😓 ok I think I have an idea of what to do 👼

@vdemeester
Use golang official image instead debian stretch
572cb66 
Signed-off-by: Vincent Demeester <vincent@sbr.pm>
@codecov
Copy link
codecov bot commented Feb 28, 2018 •
edited
Loading

Codecov Report

❗ No coverage uploaded for pull request base (master@f571eb1). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master   #36425   +/-   ##
=========================================
  Coverage          ?   34.95%           
=========================================
  Files             ?      613           
  Lines             ?    45375           
  Branches          ?        0           
=========================================
  Hits              ?    15859           
  Misses            ?    27422           
  Partials          ?     2094

dnephin
dnephin approved these changes Feb 28, 2018
View reviewed changes
Copy link
Member
@dnephin dnephin left a comment

Choose a reason for hiding this comment

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

LGTM

thaJeztah
thaJeztah approved these changes Feb 28, 2018
View reviewed changes
Copy link
Member
@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM

@thaJeztah thaJeztah merged commit 78efc2f into moby:master Feb 28, 2018
@vdemeester vdemeester deleted the dockerfile-adjusts branch February 28, 2018 22:11
@thaJeztah thaJeztah mentioned this pull request Apr 13, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cpuguy83 cpuguy83 cpuguy83 approved these changes

@thaJeztah thaJeztah thaJeztah approved these changes

@AkihiroSuda AkihiroSuda AkihiroSuda approved these changes

@tianon tianon Awaiting requested review from tianon

@tonistiigi tonistiigi Awaiting requested review from tonistiigi

+2 more reviewers

@dnephin dnephin dnephin approved these changes

@boaz0 boaz0 boaz0 approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
status/2-code-review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@vdemeester @boaz0 @cpuguy83 @dnephin @tianon @thaJeztah @AkihiroSuda @GordonTheTurtle
0