Skip to content
This repository was archived by the owner on Oct 13, 2023. It is now read-only.

Conversation

@vieux
Copy link
Contributor

@vieux vieux commented Sep 5, 2017

backport:

$ git cherry-pick -s -x -Xsubtree=components/cli a0113c3a448cb1ed875f2116eb8436ee8bd97bfc
$ git cherry-pick -s -x -Xsubtree=components/cli 724f03bb23bae4339edf6af5017ae695dd977f2f
$ git cherry-pick -s -x -Xsubtree=components/engine 58b96aced87b33c4175fa5d3422289f763ab599d
$ git cherry-pick -s -x -Xsubtree=components/cli ef027b6d7292b0ed249e70294f5f23c51a68e794
$ git cherry-pick -s -x -Xsubtree=components/cli 50696bbf72b4177bb324fc411be3121b6e935b9f
$ git cherry-pick -s -x -Xsubtree=components/cli 536da4a07a79de929e9e6857220a9a941f6a46ef
$ git cherry-pick -s -x -Xsubtree=components/cli 850b46e67cd96f7bd4fe49937305fdd7748049df
$ git cherry-pick -s -x -Xsubtree=components/cli a747389bf4180881f658b45fac2296306072756c
$ git cherry-pick -s -x -Xsubtree=components/engine b291f5a31728f7ff6386bb37f15e7c0885d3b2a7

cherry-pick was clean, no conflict

@vieux vieux requested review from andrewhsu and dnephin September 5, 2017 20:56
@andrewhsu
Copy link
Contributor

Analysis of build results show that DockerSuite.TestRunAttachDetachFromInvalidFlag test is now passing because of this PR:

$ ./check_failures.sh https://jenkins.dockerproject.org/job/docker-ce-pr/168/consoleText
FOR: https://jenkins.dockerproject.org/job/docker-ce-pr/168/consoleText

Passed (thought would fail):
DockerSuite.TestRunAttachDetachFromInvalidFlag
DockerSwarmSuite.TestServiceLogs
DockerSwarmSuite.TestSwarmNetworkPluginV2
DockerSwarmSuite.TestSwarmServicePsMultipleServiceIDs

Failed (should fail)
DockerSuite.TestRmiContainerImageNotFound
DockerSuite.TestRmiImageIDForceWithRunningContainersAndMultipleTags
DockerSuite.TestRunWithNanoCPUs
DockerSuite.TestUpdateWithNanoCPUs

Failed (thought would pass):
DockerSuite.TestContainersAPICreateMountsCreate
DockerSuite.TestRunEnvironment

@vieux
Copy link
Contributor Author

vieux commented Sep 5, 2017

but it's also adding 2 new failures :/

@vieux vieux closed this Sep 6, 2017
@dnephin
Copy link
Contributor

dnephin commented Sep 6, 2017

TestRunEnvironment is a flaky test, see moby/moby#34757 , so we can ignore that failure.

I will look into TestContainersAPICreateMountsCreate now.
Want to re-open this PR? We need it to fix a real failure (TestRunAttachDetachFromInvalidFlag)

@dnephin
Copy link
Contributor

dnephin commented Sep 6, 2017

TestContainersAPICreateMountsCreate should be fixed by moby/moby#34761

It was caused by moby/moby#34487

The test is calling docker volume inspect "" (yes with an empty volume name). Previously that would be an error, but because of moby/moby#34487 the client performs the API call to /v1.32/volumes instead of /v1.30/volumes/, which doesn't produce an error and since it returns a valid json object it decodes it into the empty Volume that we see printed.

I guess we can also fix this in the client to return an error when the ID is empty.

@dnephin
Copy link
Contributor

dnephin commented Sep 7, 2017

moby/moby#34770 also fixed TestContainersAPICreateMountsCreate once it gets vendored into docker/cli

simonferquel and others added 2 commits September 8, 2017 16:03
Signed-off-by: Simon Ferquel <[email protected]>
(cherry picked from commit a0113c3a448cb1ed875f2116eb8436ee8bd97bfc)
Signed-off-by: Victor Vieux <[email protected]>
Signed-off-by: Daniel Nephin <[email protected]>
(cherry picked from commit 724f03bb23bae4339edf6af5017ae695dd977f2f)
Signed-off-by: Victor Vieux <[email protected]>
@vieux vieux reopened this Sep 8, 2017
@vieux
Copy link
Contributor Author

vieux commented Sep 8, 2017

added moby/moby#34761

@vieux vieux changed the title [17.09] cli updated vendoring [do not merge] [17.09] cli updated vendoring Sep 8, 2017
@vieux
Copy link
Contributor Author

vieux commented Sep 11, 2017

alight, that's very encouraging for 17.10. unfortunatly, merge such PR after the initial code freeze seems very risky to me, so I don't think we should merge it now.

@andrewhsu @dnephin @tiborvass

@dnephin
Copy link
Contributor

dnephin commented Sep 11, 2017

Don't we need this to fix the panic in docker/cli (docker/cli#380) ?

This is mostly just vendoring and test changes, so I would be fine to merge it. The risk is pretty low.

This reverts commit b33e8d4.

Signed-off-by: Victor Vieux <[email protected]>
@vieux vieux changed the title [do not merge] [17.09] cli updated vendoring [17.09] cli updated vendoring Sep 12, 2017
@vieux
Copy link
Contributor Author

vieux commented Sep 12, 2017

@dnephin correct, I fixed the last commit. I think we are ready to merge this one.

Primarily to bring in fix for "Clear Architecture field in platform
constraint for arm architectures".

Signed-off-by: Andrew Hsu <[email protected]>
(cherry picked from commit ef027b6d7292b0ed249e70294f5f23c51a68e794)
Signed-off-by: Victor Vieux <[email protected]>
To satisfy requirements of the vndr of docker/docker

Signed-off-by: Andrew Hsu <[email protected]>
(cherry picked from commit 50696bbf72b4177bb324fc411be3121b6e935b9f)
Signed-off-by: Victor Vieux <[email protected]>
Signed-off-by: Andrew Hsu <[email protected]>
(cherry picked from commit 536da4a07a79de929e9e6857220a9a941f6a46ef)
Signed-off-by: Victor Vieux <[email protected]>
@vieux
Copy link
Contributor Author

vieux commented Sep 12, 2017

@andrewhsu I added docker/cli#516

@andrewhsu
Copy link
Contributor

@dnephin seeing just 1 failure from the ce-tests:

[DockerSuite] FAIL: docker_api_containers_test.go:1894: DockerSuite.TestContainersAPICreateMountsCreate
[DockerSuite] 
[DockerSuite] case 0 - config: {volume  /foo false  <nil> <nil> <nil>}
[DockerSuite] case 1 - config: {volume  /foo/ false  <nil> <nil> <nil>}
[DockerSuite] case 2 - config: {volume test1 /foo false  <nil> <nil> <nil>}
[DockerSuite] case 3 - config: {volume test2 /foo true  <nil> <nil> <nil>}
[DockerSuite] case 4 - config: {volume test3 /foo false  <nil> 0xc420233f80 <nil>}
[DockerSuite] case 5 - config: {bind /tmp/test-mounts-api-1625168725 /foo false  <nil> <nil> <nil>}
[DockerSuite] docker_api_containers_test.go:2018:
[DockerSuite]     c.Assert(err, checker.NotNil, check.Commentf(out))
[DockerSuite] ... value = nil
[DockerSuite] ... [
[DockerSuite]     {
[DockerSuite]         "Driver": "",
[DockerSuite]         "Labels": null,
[DockerSuite]         "Mountpoint": "",
[DockerSuite]         "Name": "",
[DockerSuite]         "Options": null,
[DockerSuite]         "Scope": ""
[DockerSuite]     }
[DockerSuite] ]

@andrewhsu
Copy link
Contributor

I think the point to vndr for github.com/docker/docker did not include enough fixes. I've created another PR to have docker/cli vendor to the latest on moby/moby: docker/cli#528

Signed-off-by: Andrew Hsu <[email protected]>
(cherry picked from commit 850b46e67cd96f7bd4fe49937305fdd7748049df)
Signed-off-by: Victor Vieux <[email protected]>
@vieux
Copy link
Contributor Author

vieux commented Sep 13, 2017

I added docker/cli#528

@dnephin
Copy link
Contributor

dnephin commented Sep 13, 2017

Looks like CI failed, but changes look good

@andrewhsu
Copy link
Contributor

I think this is a reproducible error:

$ cd components/cli
$ make -f docker.Makefile binary
...
Building statically linked build/docker-linux-amd64
# github.com/docker/cli/cli/command/service
cli/command/service/logs.go:260: undefined: "github.com/docker/cli/vendor/github.com/docker/docker/client".ParseLogDetails
make: *** [Makefile:31: binary] Error 2
make: *** [binary] Error 2

@dnephin
Copy link
Contributor

dnephin commented Sep 13, 2017

Yes, you'll need docker/cli#510

It's being removed from client/

Signed-off-by: Daniel Nephin <[email protected]>
(cherry picked from commit a747389bf4180881f658b45fac2296306072756c)
Signed-off-by: Victor Vieux <[email protected]>
@vieux
Copy link
Contributor Author

vieux commented Sep 13, 2017

@dnephin I added it, thanks

@andrewhsu
Copy link
Contributor

The ce-tests looking green, just one failure on the windows job:

FAIL: docker_cli_pull_test.go:273: DockerSuite.TestPullLinuxImageFailsOnWindows

docker_cli_pull_test.go:276:
    c.Assert(err.Error(), checker.Contains, "cannot be used on this platform")
... obtained string = "" +
...     "\n" +
...     "Command:  d:\\CI\\CI-c6e21043d8\\binary\\docker.exe pull ubuntu\n" +
...     "ExitCode: 1\n" +
...     "Error:    exit status 1\n" +
...     "Stdout:   Using default tag: latest\n" +
...     "latest: Pulling from library/ubuntu\n" +
...     "\n" +
...     "Stderr:   no matching manifest for windows/amd64 in the manifest list entries\n" +
...     "\n" +
...     "\n" +
...     "Failures:\n" +
...     "ExitCode was 1 expected 0\n" +
...     "Expected no error"
... substring string = "cannot be used on this platform"

cc @johnstep

@johnstep
Copy link
Contributor

This is fixed by moby/moby#34829.

Signed-off-by: John Howard <[email protected]>
(cherry picked from commit b291f5a)
Signed-off-by: Andrew Hsu <[email protected]>
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 when builds are green

@andrewhsu andrewhsu merged commit efac66c into docker-archive:17.09 Sep 13, 2017
@andrewhsu andrewhsu added this to the 17.09.0 milestone Sep 13, 2017
@vieux vieux deleted the updatecli branch September 13, 2017 21:51
docker-jenkins pushed a commit that referenced this pull request Jun 18, 2019
[18.09 backport] Switch swarmmode services to NanoCpu
Upstream-commit: d1a30309de3f9e13d78779976e28ce52dfebdf2f
Component: engine
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants