Skip to content

Dockerfile: use GO_VERSION build-arg for overriding Go version#39548

Merged
thaJeztah merged 2 commits intomoby:masterfrom
thaJeztah:golang_arg
Jul 19, 2019
Merged

Dockerfile: use GO_VERSION build-arg for overriding Go version#39548
thaJeztah merged 2 commits intomoby:masterfrom
thaJeztah:golang_arg

Conversation

@thaJeztah
Copy link
Member

@thaJeztah thaJeztah commented Jul 17, 2019

This allows overriding the version of Go without making modifications in the
source code, which can be useful to test against multiple versions.

For example:

make GO_VERSION=1.13beta1 shell

@thaJeztah thaJeztah requested a review from tianon as a code owner July 17, 2019 12:07
@thaJeztah thaJeztah mentioned this pull request Jul 17, 2019
Merged
6 tasks
@thaJeztah
Copy link
Member Author

thaJeztah commented Jul 17, 2019

experimental failures are unrelated; https://jenkins.dockerproject.org/job/Docker-PRs-experimental/46008/console

13:22:49 FAIL: docker_api_swarm_service_test.go:34: DockerSwarmSuite.TestAPIServiceUpdatePort
13:49:37 FAIL: docker_cli_swarm_test.go:1535: DockerSwarmSuite.TestSwarmPublishDuplicatePorts

@thaJeztah
Copy link
Member Author

thaJeztah commented Jul 17, 2019

Janky failure is a flaky docker-py test

16:36:46 _______________________ ExecTest.test_exec_command_demux _______________________
16:36:46 /docker-py/tests/integration/api_exec_test.py:182: in test_exec_command_demux
16:36:46     assert next(exec_log) == (b'hello out\r\n', None)
16:36:46 E   AssertionError: assert ('hello out\r...rr\r\n', None) == ('hello out\r\n', None)
16:36:46 E     At index 0 diff: 'hello out\r\nhello err\r\n' != 'hello out\r\n'

kolyshkin
kolyshkin approved these changes Jul 18, 2019
View reviewed changes
Copy link
Contributor

@kolyshkin kolyshkin left a comment

Choose a reason for hiding this comment

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

LGTM

This is really nice to have; is there any place we can document it? Perhaps docs/contributing/set-up-dev-env.md and/or TESTING.md?

@tiborvass
Copy link
Contributor

tiborvass commented Jul 18, 2019

@cpuguy83 I think we can leave it as a go version for now and if we have the need to override the entire image, add another variable that defaults to golang:${GO_VERSION}-alpine.

@thaJeztah
Copy link
Member Author

thaJeztah commented Jul 18, 2019

Docker Hub made a whoopsie https://jenkins.dockerproject.org/job/Docker-PRs/54931/console

17:30:20 FAIL: docker_cli_swarm_test.go:342: DockerSwarmSuite.TestSwarmContainerEndpointOptions
17:30:20 
17:30:20 Creating a new daemon
17:30:20 [dc255ac69325d] waiting for daemon to start
17:30:20 [dc255ac69325d] waiting for daemon to start
17:30:20 [dc255ac69325d] daemon started
17:30:20 
17:30:20 assertion failed: error is not nil: exit status 125: Unable to find image 'busybox:glibc' locally
17:30:20 /usr/local/cli/docker: Error response from daemon: Get https://registry-1.docker.io/v2/library/busybox/manifests/glibc: received unexpected HTTP status: 500 Internal Server Error.
17:30:20 See '/usr/local/cli/docker run --help'.
17:30:20 
17:30:20 [dc255ac69325d] Stopping daemon
17:30:20 [dc255ac69325d] exiting daemon
17:30:20 [dc255ac69325d] Daemon stopped

@GordonTheTurtle GordonTheTurtle added the dco/no Automatically set by a bot when one of the commits lacks proper signature label Jul 18, 2019
@GordonTheTurtle

This comment has been minimized.

Sign in to view
@GordonTheTurtle GordonTheTurtle removed the dco/no Automatically set by a bot when one of the commits lacks proper signature label Jul 18, 2019
@kolyshkin
Copy link
Contributor

kolyshkin commented Jul 18, 2019

Added a doc commit

thaJeztah
thaJeztah commented Jul 18, 2019
View reviewed changes
Copy link
Member Author

@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.

docs LGTM

@andrewhsu
Copy link
Contributor

andrewhsu commented Jul 18, 2019

Looks like the known flaky test in the experimental PR check failed:

FAIL: docker_cli_swarm_test.go:1329: DockerSwarmSuite.TestSwarmClusterRotateUnlockKey

Likely because this PR was based on codeline before #39531 was merged.

thaJeztah and others added 2 commits July 18, 2019 17:36
@thaJeztah @kolyshkin
Dockerfile: use GO_VERSION build-arg for overriding Go version
c6281bc 
This allows overriding the version of Go without making modifications in the
source code, which can be useful to test against multiple versions.

For example:

    make GO_VERSION=1.13beta1 shell

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@kolyshkin
TESTING.md: document GO_VERSION
a557538 
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
@kolyshkin
Copy link
Contributor

kolyshkin commented Jul 19, 2019

Likely because this PR was based on codeline before #39531 was merged

You're right. Rebased to master; let's see how it goes.

@andrewhsu
Copy link
Contributor

andrewhsu commented Jul 19, 2019

Sorry, didn't mean to imply this needed to be rebased and tests re-run. I think it is good to go because the only failing test was a known flaky test.

andrewhsu
andrewhsu approved these changes Jul 19, 2019
View reviewed changes
Copy link
Contributor

@andrewhsu andrewhsu 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
Copy link
Member Author

thaJeztah commented Jul 19, 2019

all green now

@thaJeztah thaJeztah merged commit 0949fe8 into moby:master Jul 19, 2019
@thaJeztah thaJeztah deleted the golang_arg branch July 19, 2019 07:25
This was referenced Jul 20, 2019
Merged
Merged
@thaJeztah thaJeztah mentioned this pull request Aug 8, 2019
Merged
@thaJeztah thaJeztah mentioned this pull request Aug 28, 2019
Merged
@thaJeztah thaJeztah added this to the 20.03.0 milestone Apr 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tiborvass tiborvass tiborvass approved these changes

@kolyshkin kolyshkin kolyshkin approved these changes

@AkihiroSuda AkihiroSuda AkihiroSuda approved these changes

@tianon tianon Awaiting requested review from tianon tianon is a code owner

+2 more reviewers

@andrewhsu andrewhsu andrewhsu approved these changes

@seemethere seemethere seemethere approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

area/testing process/cherry-picked status/2-code-review

Projects

None yet

Milestone

20.10.0

Development

Successfully merging this pull request may close these issues.

7 participants

@thaJeztah @tiborvass @GordonTheTurtle @kolyshkin @andrewhsu @seemethere @AkihiroSuda