updated vendoring #424

simonferquel · 2017-08-07T12:29:11Z

- What I did
Updated dependencies so that we can use the new sirupsen import path
- How I did it
Had to fork notary
- How to verify it
See vendor.conf

codecov-io · 2017-08-07T13:32:10Z

Codecov Report

Merging #424 into master will decrease coverage by <.01%.
The diff coverage is 25%.

@@            Coverage Diff             @@
##           master     #424      +/-   ##
==========================================
- Coverage    48.8%   48.79%   -0.01%     
==========================================
  Files         199      199              
  Lines       16389    16392       +3     
==========================================
  Hits         7998     7998              
- Misses       7976     7978       +2     
- Partials      415      416       +1

aluzzardi · 2017-08-15T23:14:28Z

Hey @simonferquel:

why do we need to update the logrus import path?
If we need to do so, why do we need to change the other dependencies?
Why do we need to change notary, and if we do need to change it, why not change upstream directly rather than using a fork?

vdemeester · 2017-08-16T09:20:59Z

@aluzzardi

why do we need to update the logrus import path?

Because it has been changed upstream (i.e. moby/moby for example) and thus, any vendoring update will need to fix thoses.

If we need to do so, why do we need to change the other dependencies?

Because most of the time the sirupsen fix is in updated version of the deps.

Why do we need to change notary, and if we do need to change it, why not change upstream directly rather than using a fork?

Multiple reason here :

cycle vendoring (libnetwork, swarmkit with moby/moby for example — so while it's been merge upstream and update in moby/moby we need to vendor the same)
waiting for PR to get reviewed and merged (notary, etc..)
dependencies that are heavily changed if update (and would need change to our code other than the import) and likely to not getting merged (rethinkdb for example)

simonferquel · 2017-08-29T08:55:44Z

@vdemeester can we move along on this PR (don't want to rebase it once again, and only code coverage fails - there is no code addition, only vendoring) ?

vdemeester

LGTM 🐯

dnephin · 2017-08-29T15:01:48Z

vendor.conf

-github.com/docker/distribution b38e5838b7b2f2ad48e06ec4b500011976080621
-github.com/docker/docker d58ffa0364c04d03a8f25704d7f0489ee6cd9634
-github.com/docker/docker-credential-helpers v0.5.1
+# docker


I think we should leave this file sorted. It makes it easier to maintain and avoid conflicts when multiple PRs change this file. Commented sections are difficult to maintain and will lead to an unsorted mess. I guess there were a few lines that were not sorted previously. I'll see about adding a validation for this file to keep it sorted.

I checked out this PR and re-sorted the file to look at the vendor changes. I noticed that we have a bunch of new dependencies that I don't think we actually use. Somewhere in the dependency there are some bad packages that are overloaded and forcing us to pull in a bunch of unnecessary extra libraries.

I'm going to look into this to see if we can remove some of these.

dnephin

I looked over the dependency tree for the new dependencies. (notes are here https://gist.github.com/dnephin/137d578fd0095d15ab9da63f236fb7c6)

It looks like everything can be removed with two small changes:

1. Remove the import of github.com/docker/docker/opts from github.com/docker/docker/registry. This should be really easy because that package should not be doing anything with options. I believe it will be as easy as moving InstallCliFlags to a new package. (opened moby/moby#34668, will need to be included in this vendor update once merged)

~~2. Remove the import of docker/runconfig from cli/command/container/opts_test.go (opened #480 for this)~~

Both of these should now be fixed with the latest moby/moby master

dnephin · 2017-08-29T15:26:07Z

vendor.conf

+github.com/docker/notary v0.4.2-sirupsen https://github.com/simonferquel/notary.git
+github.com/docker/swarmkit 0554c9bc9a485025e89b8e5c2c1f0d75961906a2
+github.com/moby/buildkit da2b9dc7dab99e824b2b1067ad7d0523e32dd2d9 https://github.com/dmcgowan/buildkit.git
+github.com/containerd/continuity cf279e6ac893682272b4479d4c67fd3abf878b4


it looks like this is not used, please remove

dnephin · 2017-08-29T15:41:49Z

cli/command/image/build_session.go

 	"github.com/docker/docker/pkg/progress"
+	"github.com/moby/buildkit/session"
+	"github.com/moby/buildkit/session/filesync"


Let's keep this for a separate PR. This code is still in docker/docker/client in master, so we shouldn't need to do this right now.

It is not there anymore on moby/moby master :(

oops, I thought I checked before commenting, but it must have been an old branch. ok to ignore this one

dnephin · 2017-08-29T15:42:37Z

vendor.conf

-github.com/gogo/protobuf 7efa791bd276fd4db00867cbd982b552627c24cb
+github.com/docker/notary v0.4.2-sirupsen https://github.com/simonferquel/notary.git
+github.com/docker/swarmkit 0554c9bc9a485025e89b8e5c2c1f0d75961906a2
+github.com/moby/buildkit da2b9dc7dab99e824b2b1067ad7d0523e32dd2d9 https://github.com/dmcgowan/buildkit.git


this can be removed, we should be using docker/docker/client

simonferquel · 2017-08-31T17:55:17Z

@dnephin I updated to latest master and was able to remove quite a few dependencies. I went back to a sorted vendor.conf (and sorted it correctly).

simonferquel · 2017-09-01T07:32:35Z

Oh and btw, thanks a lot @dnephin for reducing the dependency surface. It makes this PR far more manageable :)

dnephin

It looks like there are some bugs in vndr because those 2 dependencies should have caused the vendor check to fail. I ran it locally and vndr isn't complaining about unused things.

I had to manually remove them. I'm going to push a fix to this PR which removes the two unused things.

Just one remaining question about why we need a fork for buildkit

dnephin · 2017-09-01T14:44:42Z

vendor.conf

 github.com/docker/go-units 9e638d38cf6977a37a8ea0078f3ee75a7cdb2dd1
+github.com/docker/libnetwork 19ac3ea7f52bb46e0eb10669756cdae0c441a5b1 


I believe this is not used, can be removed.

dnephin · 2017-09-01T14:45:37Z

vendor.conf

 github.com/golang/protobuf 7a211bcf3bce0e3f1d74f9894916e6f116ae83b4
-github.com/gotestyourself/gotestyourself v1.0.0
+github.com/google/certificate-transparency 0f6e3d1d1ba4d03fdaab7cd716f36255c2e48341


dnephin · 2017-09-01T14:47:38Z

vendor.conf

-github.com/opencontainers/go-digest a6d0ee40d4207ea02364bd3b9e8e77b9159ba1eb
-github.com/opencontainers/image-spec f03dbe35d449c54915d235f1a3cf8f585a24babe
-github.com/opencontainers/runc 9c2d8d184e5da67c95d601382adf14862e4f2228 https://github.com/docker/runc.git
+github.com/moby/buildkit da2b9dc7dab99e824b2b1067ad7d0523e32dd2d9 https://github.com/dmcgowan/buildkit.git


can't we get this fix into an official buildkit branch at least? Why is this a fork?

dnephin

sounds like master of buildkit has a different API, so let's use the fork for now

LGTM

andrewhsu · 2017-09-01T23:18:57Z

@simonferquel @dnephin looks like this has issues when tests run code from moby/moby master branch: https://jenkins.dockerproject.org/job/docker-integration-tests/114/console

dnephin · 2017-09-01T23:39:47Z

I think this hasn't been rebased with master, let me do that now.

Signed-off-by: Simon Ferquel <simon.ferquel@docker.com>

Signed-off-by: Daniel Nephin <dnephin@docker.com>

dnephin · 2017-09-01T23:41:39Z

done. Can you try again?

dnephin · 2017-09-01T23:42:55Z

Most of the failures seem to be from the panic we fixed a couple days ago.

andrewhsu · 2017-09-01T23:53:10Z

https://jenkins.dockerproject.org/job/docker-integration-tests/115/

andrewhsu · 2017-09-02T00:38:23Z

hmm...seeing new errors:

[DockerSwarmSuite] FAIL: docker_api_swarm_test.go:349: DockerSwarmSuite.TestAPISwarmRaftQuorum
[DockerSwarmSuite] 
[DockerSwarmSuite] [dfb093aea8ec6] waiting for daemon to start
[DockerSwarmSuite] [dfb093aea8ec6] daemon started
[DockerSwarmSuite] 
[DockerSwarmSuite] [ddc9a034c25d9] waiting for daemon to start
[DockerSwarmSuite] [ddc9a034c25d9] daemon started
[DockerSwarmSuite] 
[DockerSwarmSuite] [df42acc6abc29] waiting for daemon to start
[DockerSwarmSuite] [df42acc6abc29] daemon started
[DockerSwarmSuite] 
[DockerSwarmSuite] [ddc9a034c25d9] exiting daemon
[DockerSwarmSuite] [df42acc6abc29] exiting daemon
[DockerSwarmSuite] docker_api_swarm_test.go:374:
[DockerSwarmSuite]     c.Assert(err.Error(), checker.Contains, "deadline exceeded")
[DockerSwarmSuite] ... obtained string = "Error response from daemon: rpc error: code = Internal desc = transport is closing"
[DockerSwarmSuite] ... substring string = "deadline exceeded"

[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> 0xc420539940 <nil>}
[DockerSuite] case 5 - config: {bind /tmp/test-mounts-api-1758465792 /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] ]

and a lot of these:

[DockerExternalVolumeSuite] --- FAIL: TestInspect (0.51s)
[DockerExternalVolumeSuite] 	daemon.go:285: [db2c0ab830136] waiting for daemon to start
[DockerExternalVolumeSuite] 	daemon.go:317: [db2c0ab830136] daemon started
[DockerExternalVolumeSuite] 	assertions.go:226:

dnephin · 2017-09-02T04:07:35Z

TestInspect is fixed by moby/moby#34682

The other 2 are API tests, so should not be affected by the CLI, probably flakes.

dnephin · 2017-09-05T17:20:57Z

Let's merge this?

simonferquel requested review from dnephin and vdemeester as code owners August 7, 2017 12:29

GordonTheTurtle added the status/0-triage label Aug 7, 2017

simonferquel force-pushed the update-vendoring branch from ebd1c7e to 4ba98b3 Compare August 7, 2017 12:48

simonferquel force-pushed the update-vendoring branch 3 times, most recently from 4cf7a48 to 4af6b5f Compare August 8, 2017 15:00

RenaudWasTaken mentioned this pull request Aug 9, 2017

Integrated Generic resource in service create #429

Merged

simonferquel mentioned this pull request Aug 9, 2017

--isolation for setting swarm service isolation mode #426

Merged

vdemeester added status/2-code-review and removed status/0-triage labels Aug 25, 2017

simonferquel force-pushed the update-vendoring branch 5 times, most recently from 449f764 to 333e270 Compare August 28, 2017 16:29

vdemeester approved these changes Aug 29, 2017

View reviewed changes

dnephin reviewed Aug 29, 2017

View reviewed changes

This was referenced Aug 29, 2017

Remove 5k lines of unnecessary dependencies #480

Merged

Fix bad import graph from opts/opts.go moby/moby#34668

Merged

Remove command line flag install from registry package moby/moby#34669

Merged

simonferquel force-pushed the update-vendoring branch 2 times, most recently from 3473c59 to 86990e5 Compare August 31, 2017 17:49

dnephin reviewed Sep 1, 2017

View reviewed changes

dnephin mentioned this pull request Sep 1, 2017

Vendor github.com/docker/docker to a127269803b #497

Closed

dnephin approved these changes Sep 1, 2017

View reviewed changes

simonferquel and others added 2 commits September 1, 2017 19:41

updated vendoring

a0113c3

Signed-off-by: Simon Ferquel <simon.ferquel@docker.com>

Remove unused depdendencies from vendor

724f03b

Signed-off-by: Daniel Nephin <dnephin@docker.com>

dnephin force-pushed the update-vendoring branch from 3f58191 to 724f03b Compare September 1, 2017 23:41

dnephin merged commit 5c5cdd0 into docker:master Sep 5, 2017

GordonTheTurtle added this to the 17.08.0 milestone Sep 5, 2017

dnephin mentioned this pull request Sep 5, 2017

docker image rm -f should exit with 1 if the image was not removed #418

Closed

vieux mentioned this pull request Sep 5, 2017

[17.09] cli updated vendoring docker-archive/docker-ce#221

Merged

dnephin mentioned this pull request Sep 6, 2017

panic when using wrong detach keys #380

Closed

thaJeztah mentioned this pull request Sep 26, 2017

Fixed import in loader.go #558

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

updated vendoring #424

updated vendoring #424

simonferquel commented Aug 7, 2017

codecov-io commented Aug 7, 2017 •

edited

Loading

aluzzardi commented Aug 15, 2017

vdemeester commented Aug 16, 2017 •

edited

Loading

simonferquel commented Aug 29, 2017

vdemeester left a comment

dnephin Aug 29, 2017

dnephin left a comment •

edited

Loading

dnephin Aug 29, 2017

dnephin Aug 29, 2017

simonferquel Aug 31, 2017

dnephin Aug 31, 2017

dnephin Aug 29, 2017

simonferquel commented Aug 31, 2017

simonferquel commented Sep 1, 2017

dnephin left a comment

dnephin Sep 1, 2017

dnephin Sep 1, 2017

dnephin Sep 1, 2017

dnephin left a comment

andrewhsu commented Sep 1, 2017

dnephin commented Sep 1, 2017

dnephin commented Sep 1, 2017

dnephin commented Sep 1, 2017

andrewhsu commented Sep 1, 2017

andrewhsu commented Sep 2, 2017

dnephin commented Sep 2, 2017

dnephin commented Sep 5, 2017

		github.com/docker/go-units 9e638d38cf6977a37a8ea0078f3ee75a7cdb2dd1
		github.com/docker/libnetwork 19ac3ea7f52bb46e0eb10669756cdae0c441a5b1

updated vendoring #424

updated vendoring #424

Conversation

simonferquel commented Aug 7, 2017

codecov-io commented Aug 7, 2017 • edited Loading

Codecov Report

aluzzardi commented Aug 15, 2017

vdemeester commented Aug 16, 2017 • edited Loading

simonferquel commented Aug 29, 2017

vdemeester left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dnephin left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simonferquel commented Aug 31, 2017

simonferquel commented Sep 1, 2017

dnephin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dnephin left a comment

Choose a reason for hiding this comment

andrewhsu commented Sep 1, 2017

dnephin commented Sep 1, 2017

dnephin commented Sep 1, 2017

dnephin commented Sep 1, 2017

andrewhsu commented Sep 1, 2017

andrewhsu commented Sep 2, 2017

dnephin commented Sep 2, 2017

dnephin commented Sep 5, 2017

codecov-io commented Aug 7, 2017 •

edited

Loading

vdemeester commented Aug 16, 2017 •

edited

Loading

dnephin left a comment •

edited

Loading