Change dependency from `github.com/fsouza/go-dockerclient` to `github.com/docker/docker/client`"

What does this MR do?

Switch from github.com/fsouza/go-dockerclient to github.com/docker/docker/client

Why was this MR needed?

We're out of date with go-dockerclient and would rather use the official client, which is now docker/docker/client, not docker/engine-api/client

Are there points in the code the reviewer needs to double check?

Does this MR meet the acceptance criteria?

• CHANGELOG entry added
• Documentation created/updated
• Tests
  • Added for this feature/bug
  • All builds are passing
• Branch has no merge conflicts with master (if you do - rebase it please)

What are the relevant issue numbers?

Closes #1606 (closed)

Added 1 commit:

• 3f1630ac - fixup! Move from github.com/fsouza/go-dockerclient to github.com/docker/docker/client

I'm developing against go1.7 like an idiot :D will downgrade if we're still targeting go1.6

@ayufan can we update to go1.7, do you think? Godeps/Godeps.json already references it (might be my fault :/)

The build failure is on go1.6 generally, per https://github.com/docker/docker/pull/26406

Merged, so we can stick with go1.6 for now

Added 2 commits:

• 85e160fa - Update Godeps
• 139ce220 - Move from github.com/fsouza/go-dockerclient to github.com/docker/docker/client

mentioned in issue #1222 (closed)

Added 2 commits:

• 737b85e1 - fixup! Update Godeps
• c67093a9 - fixup! Move from github.com/fsouza/go-dockerclient to github.com/docker/docker/client

Resolved all discussions

Added 1 commit:

• a83fd6e3 - fixup! Move from github.com/fsouza/go-dockerclient to github.com/docker/docker/client

Added 1 commit:

• 205da413 - Support old-style docker config files

Resolved all discussions

mentioned in merge request !282 (closed)

Changed title: WIP{- Resolve "Change dependency from github.com/fsouza/go-dockerclient to github.com/docker/engine-api-}" → WIP{+: Change dependency from github.com/fsouza/go-dockerclient to github.com/docker/docker/client+}"

Added 3 commits:

• 38eed946 - Update Godeps
• 2d281b9e - Move from github.com/fsouza/go-dockerclient to github.com/docker/docker/client
• 115620bd - Support old-style docker config files

Added 2 commits:

• dbe0bcff - fixup! Move from github.com/fsouza/go-dockerclient to github.com/docker/docker/client
• 46dc2f32 - fixup! Update Godeps

Added 1 commit:

• 63937db5 - fixup! fixup! Move from github.com/fsouza/go-dockerclient to github.com/docker/docker/client

mentioned in merge request !139 (closed)

mentioned in merge request !192 (closed)

mentioned in merge request !246 (closed)

Added 1 commit:

• 4535b685 - Simply the ImagePull call

Added 1 commit:

• 3e0b4aa9 - Attach to containers before starting them

Added 5 commits:

• 2c2e0d0d - Update Godeps
• d3bca4b4 - Move from github.com/fsouza/go-dockerclient to github.com/docker/docker/client
• 0ae90243 - Support old-style docker config files
• ad64e9a3 - Simply the ImagePull call
• 5647f380 - Attach to containers before starting them

Milestone changed to %v1.6

Reassigned to @nick.thomas

Assignee removed

Reassigned to @ayufan

@ayfan @tmaczukin I believe I've resolved the ImagePull authentication thing to my satisfaction. The asynchronous consumers (ImageImport and ContainerLogs) are reasonable now too, I think.

@nick.thomas

It would be nice if you could look at my comments for previous MR :)

• context.TODO()
• naming in a few places

This implementation looks very valid. I like it. Lets try to use our context where possible and we should be able to merge it and give it test run on our internal runners.

Reassigned to @nick.thomas

@ayufan I responded to them on the previous MR, awaiting feedback.

For context.TODO():

I don't want to address implementing contexts in this MR; we could introduce a helper method that just returns context.TODO()? What would that gain us?

For names (presumably the fakeContainer diffs):

runServiceHealthCheckContainer makes use of the container name to build its own name. This only uses s.services, which is populated from objects returned from createService, which makes use of container.Name (or Names[0] in this MR).

We could modify the name the service healthcheck container gets so it includes the container ID, rather than the name? It's a touch less user-friendly but would allow us to remove fakeContainer

One possible worry is that we are depending on a very recent version of docker. I don't now how appropriate it is to release this while depending on an arbitrary commit in github.com/docker/docker rather than a named release.

Please never ever vendor random commits. Vendoring should be done only by tag (i.e. stable release). Vendoring unreleased snapshots cause serious problems downstream...

I agree with @onlyjob, we should probably wait for tagged release. And merge that only by then.

OK, I'll keep gardening this one until then. I guess it won't go into the runner until v1.8 at the earliest.

Milestone removed

Mentioned in issue #1729

Added 75 commits:

• 5647f380...2d7d2fc0 - 70 commits from branch gitlab-org:master
• 60b7bec5 - Update Godeps
• 5afab000 - Move from github.com/fsouza/go-dockerclient to github.com/docker/docker/client
• a1f7a351 - Support old-style docker config files
• ed4b28f6 - Simply the ImagePull call
• bf4b3661 - Attach to containers before starting them

Rebased against current master, one conflict in the new volumesFrom functionality. No tests for that, but the conflict was trivial enough that I'm not particularly worried.

As noted in #1729, this MR changes libraries to ones that include Windows npipe support, alongside unix sockets. It's possible that this could allow us to claim Docker on Windows support, but it needs testing - and tests!

Mentioned in issue #1764

Mentioned in issue #1583

Mentioned in merge request !375 (closed)

Mentioned in merge request !376 (merged)

@nick.thomas Docker 1.13 is to be released. We should aim for finishing that now!

Absolutely! I've been watching docker releases like a hawk.

This was complete before we ran into the "no stable release of docker has these files in it" problem. As soon as 1.13 is released, I plan to rebase. That should be fun!

mentioned in merge request !432 (closed)

Can we make this our priority for the next release? Of course if Docker 1.13 will be released at the beginning of January as it is currently planned. The age and compatibility of used github.com/fsouza/go-dockerclient version is starting to bite us :| (e.g. https://gitlab.com/gitlab-org/gitlab-ci-multi-runner/merge_requests/432/diffs#1a3fbe14739730f842b79608147eb41db63aafe0_166_168)

mentioned in issue #1642 (closed)

Docker 1.13 is released! https://blog.docker.com/2017/01/whats-new-in-docker-1-13/

Yep, I expect to get on this next week.

added 399 commits

• bf4b3661...25fb7833 - 396 commits from branch gitlab-org:master
• 907eea02 - Update Godeps
• fa58b869 - Move from github.com/fsouza/go-dockerclient to github.com/docker/docker/client
• 2f04f7e1 - Simplify the ImagePull call

Compare with previous version

added 3 commits

• 3cd4e2f0 - Update Godeps
• 0538ffd7 - Move from github.com/fsouza/go-dockerclient to github.com/docker/docker/client
• 699f3250 - Simplify the ImagePull call

Compare with previous version

added 2 commits

• 64f908d3 - Move from github.com/fsouza/go-dockerclient to github.com/docker/docker/client
• 60ffdc12 - Simplify the ImagePull call

Compare with previous version

added 2 commits

• 4550a026 - Move from github.com/fsouza/go-dockerclient to github.com/docker/docker
• 71232066 - Simplify the ImagePull call

Compare with previous version

added 1 commit

• 0aa5a588 - Simplify the ImagePull call

Compare with previous version

added 3 commits

• 3a67b313 - Update Godeps
• 5c2aac70 - Move from github.com/fsouza/go-dockerclient to github.com/docker/docker
• 969873b5 - Simplify the ImagePull call

Compare with previous version

added 3 commits

• 2a013976 - Update Godeps
• 696a6b31 - Move from github.com/fsouza/go-dockerclient to github.com/docker/docker
• 1b1f1ed8 - Simplify the ImagePull call

Compare with previous version

added 3 commits

• 32b9fc08 - Update Godeps
• bd6ab990 - Move from github.com/fsouza/go-dockerclient to github.com/docker/docker
• 7c26daf3 - Simplify the ImagePull call

Compare with previous version

OK, this is now docker v1.13 and I've resolved all the merge conflicts. I've wrestled godep into submission and the tests pass... sometimes.

https://gitlab.com/nick.thomas/gitlab-ci-multi-runner/builds/9160382 is an intermittent failure that I need to look into.

added 3 commits

• 38dff79d - Move from github.com/fsouza/go-dockerclient to github.com/docker/docker
• e90e531a - Simplify the ImagePull call
• 8abea7f2 - Fix an intermittent test failure

Compare with previous version

assigned to @tmaczukin

@tmaczukin Wdyt? Is there anything I can do to make this easier to review?

marked the task All builds are passing as completed

marked the task Added for this feature/bug as completed

marked the task Branch has no merge conflicts with master (if you do - rebase it please) as completed

unmarked as a Work In Progress

changed milestone to %v1.10

changed milestone to %v1.11

Wow, 426 changed files! This will be interesting

More actually :-D gitlab is cutting off at 426 for reasons i don't understand.

Almost all of it is in vendor/ though.

mentioned in issue #2066

mentioned in issue #1913 (closed)

mentioned in merge request !468 (merged)

OK, finally I've reviewed this

@nick.thomas It looks really, really good! I left some minor comments but overall replacement looks good and clean. I didn't look on changes inside of /vendor other than listing changed files, and it also looks OK. Let's work on resolving above comments and I think we can try to merge this.

@ayufan Tests are green, and there is not many changes in tests (mainly type names replacement from the one used by previous library, to the one provided by new library). I've also made some manual tests, and as for now all was working. I suggest that we should finish work on this, prepare RC version and install it on one of our runners ASAP to discover any regressions.

assigned to @nick.thomas

added executordocker label

@tmaczukin great, thanks for the review! I'll get onto the suggestions next week.

The biggest area of concern for me is the change to how image pulls are authenticated. I tried to use official docker code for this, rather than copying blocks out of fsouza/go-dockerclient, and it's possible there are some unintended behaviour changes as a result.

@nick.thomas While you'll be updating this, I can test how pulling works. Especially that I'm waiting for this MR to fix one of the issues related to push/pull authentication - #2066

mentioned in merge request !469 (closed)

mentioned in issue #1503 (closed)

@nick FYI while waiting for this MR to be finished I'm cherry picking your fix from 8abea7f20700199afae740a56db63fb1fdb893df into master ;)

changed milestone to %v9.0

mentioned in issue #2176

@nick.thomas Can you work on this at the beginning of next week? I would like to test this, and if this is working - port it back to 1.9.x and 1.10.x which will be supported for some time after 9.0 will be released.

If you don't have time for this just let me know. I will grab this and do the final updates myself

@tmaczukin I was hoping to be able to finish it by now, but then I ended up on deploy boards and elasticsearch so I've not had time. I've no objections if you want to finish it off; I'm not sure when I'll be able to get back to it.

OK. So I'll take this and finish at the beginning of next week :)

assigned to @tmaczukin

resolved all discussions

merged

@nick.thomas Merged into master. Thanks for the huge work on making this real!

Oh. Good. I start to be afraid what we did broke now! Seriously :) <3

@ayufan We will see :). That's why I've released 9.0.0-rc.1 yesterday - we should install this ASAP on some of our runner managers to test this properly :)

@tmaczukin Can not install that yet on runners manager, but on something that we use and is on lower severity? To give it one week test round? Maybe GitLab Runner runner?

Let it be like that. I will update gitlab-runner-builder.gitlap.com :)

@ayufan gitlab-runner-builder.gitlap.com is updated to 9.0.0-rc.1. Jobs are processed, but I see first (minor) regression - look at the output of e.g. https://gitlab.com/gitlab-org/gitlab-ci-multi-runner/builds/11479731. Almost every line fetched from container's output is prefixed with some more or less random character.

That's peculiar :/

The character seems to be three bytes:

irb(main):008:0> "�".each_byte.map {|b| b.to_s(16) }
=> ["ef", "bf", "bd"]

The "attach to container" code is all new, so that's the obvious suspect.

The above should be now fixed with !503 (merged)

mentioned in merge request !501 (merged)

@ayufan I was thinking on porting this back to 1.11.X. Two weeks ago when I've tested this, the patch have been cherry-picked without any conflicts.

Any objections? Since we plan to support 1.11.X until August 2017 I think it would be good to have new Docker library also in this branch.

@tmaczukin I don't like that idea. This is full-fledged feature in itself, the 1.11.x is closed for such big things.

mentioned in issue #2277 (closed)