Follow-up fixes on multi-stage moby's Dockerfile by vdemeester · Pull Request #36425 · moby/moby

vdemeester · 2018-02-27T08:32:11Z

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

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

boaz0

LGTM 👏

boaz0 · 2018-02-27T11:23:52Z

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 · 2018-02-27T11:44:28Z

Dockerfile

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

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

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

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

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

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

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 · 2018-02-27T11:48:59Z

Dockerfile

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

LGTM

cpuguy83 · 2018-02-27T12:27:34Z

Except CI failures. :(

vdemeester · 2018-02-27T12:49:09Z

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 · 2018-02-27T18:22:10Z

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

LGTM

thaJeztah · 2018-02-27T22:59:25Z

Dockerfile

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"

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

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

codecov · 2018-02-28T08:05:07Z

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

LGTM

thaJeztah

LGTM

Do not copy golang in itself

30498c7

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

vdemeester requested review from cpuguy83, dnephin, thaJeztah, tianon and tonistiigi February 27, 2018 08:32

GordonTheTurtle added the status/0-triage label Feb 27, 2018

vdemeester force-pushed the dockerfile-adjusts branch from 62fc030 to 4ef3925 Compare February 27, 2018 08:56

vdemeester added status/2-code-review and removed status/0-triage labels Feb 27, 2018

vdemeester mentioned this pull request Feb 27, 2018

Updates the Dockerfile to use multi-stage #35100

Merged

AkihiroSuda approved these changes Feb 27, 2018

View reviewed changes

boaz0 approved these changes Feb 27, 2018

View reviewed changes

thaJeztah reviewed Feb 27, 2018

View reviewed changes

cpuguy83 approved these changes Feb 27, 2018

View reviewed changes

vdemeester added rebuild/janky labels Feb 27, 2018

GordonTheTurtle removed rebuild/experimental labels Feb 27, 2018

dnephin approved these changes Feb 27, 2018

View reviewed changes

thaJeztah reviewed Feb 27, 2018

View reviewed changes

Use golang official image instead debian stretch

572cb66

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

vdemeester force-pushed the dockerfile-adjusts branch from 4ef3925 to 572cb66 Compare February 28, 2018 08:05

vdemeester added rebuild/janky labels Feb 28, 2018

GordonTheTurtle removed rebuild/janky labels Feb 28, 2018

dnephin approved these changes Feb 28, 2018

View reviewed changes

thaJeztah approved these changes Feb 28, 2018

View reviewed changes

thaJeztah merged commit 78efc2f into moby:master Feb 28, 2018

vdemeester deleted the dockerfile-adjusts branch February 28, 2018 22:11

thaJeztah mentioned this pull request Apr 13, 2018

Refactor code in cmd/dockerd/daemon.go #36845

Merged

Conversation

vdemeester commented Feb 27, 2018

Uh oh!

boaz0 left a comment

Choose a reason for hiding this comment

Uh oh!

boaz0 commented Feb 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cpuguy83 left a comment

Choose a reason for hiding this comment

Uh oh!

cpuguy83 commented Feb 27, 2018

Uh oh!

vdemeester commented Feb 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dnephin commented Feb 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dnephin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Feb 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

dnephin left a comment

Choose a reason for hiding this comment

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

boaz0 commented Feb 27, 2018 •

edited

Loading

vdemeester commented Feb 27, 2018 •

edited

Loading

dnephin commented Feb 27, 2018 •

edited

Loading

codecov bot commented Feb 28, 2018 •

edited

Loading