Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

docker engine-api library is deprecated, we should use their official SDK #2071

Closed
sparrc opened this issue Nov 23, 2016 · 9 comments
Closed
Assignees
Milestone

Comments

@sparrc
Copy link
Contributor

sparrc commented Nov 23, 2016

Docker's engine-api library is deprecated: https://github.com/docker/engine-api

we should switch to https://github.com/docker/docker/tree/master/client as soon as possible

@ndegory
Copy link

ndegory commented Dec 9, 2016

@sparrc The new API is in a vendored repo, this breaks the build because of types incompatibilities.
I've managed to fix it by adding the Docker API and a few other repos in a vendor folder, but that means changing the vendoring model of Telegraf (I used Rancher Trash for that). I'll make a PR, but I'm aware that's maybe not the way you'd like to implement it.

@sparrc
Copy link
Contributor Author

sparrc commented Dec 9, 2016

please don't bother PRing a new vendoring system, it won't be accepted. There's no reason the current vendoring system shouldn't work with the new docker API, you just need to add those dependencies to the Godeps file.

@ndegory
Copy link

ndegory commented Dec 9, 2016

OK, no worries, I'll try to adapt the tests to workaround the types issue then.

@mvaude
Copy link

mvaude commented Dec 20, 2016

Any news around this plugin update?
I got issues with the current version, I can help on the ongoing effort if needed.

@sparrc
Copy link
Contributor Author

sparrc commented Dec 20, 2016

@mvaude can you be more specific? there are no known issues with the docker plugin besides #2027, which I don't think is related to the library being used.

If you are having an issue please open a separate issue unless you're sure that it is related to the client library.

@mvaude
Copy link

mvaude commented Dec 20, 2016

@sparrc Ok thanks then I will open a new issue I am not sure it is related.

@bergerx
Copy link

bergerx commented Jan 30, 2017

Still having the old docker engine-api lib seems like blocking other docker plugin PRs to be merged in. Is there any update on this one. Is there anybody working on this issue? @apognu @ndegory

@apognu
Copy link

apognu commented Feb 1, 2017

I was planning to look into it a bit tonight to assess the quantity of work required.

I will get back to you.

@rsingh2411
Copy link
Contributor

rsingh2411 commented Feb 9, 2017

I had raised an PR for the #2379, and made following changes Recieved following issue,
"-X main.version=dev-49-gc8cc01b -X main.commit=c8cc01b -X main.branch=master" ./... github.com/influxdata/telegraf/plugins/inputs/docker plugins/inputs/docker/docker.go:103: cannot use c (type *client.Client) as type DockerClient in assignment: *client.Client does not implement DockerClient (wrong type for ContainerList method) have ContainerList("github.com/docker/docker/vendor/golang.org/x/net/context".Context, types.ContainerListOptions) ([]types.Container, error) want ContainerList("context".Context, types.ContainerListOptions) ([]types.Container, error) make: *** [build] Error 2 sh-4.2# vi plugins/inputs/docker/docker.go'

PR is closed, but, issue is because of dependency referring to wrong repository/library
This will be faced by any one who implements the change for switching from Docker's engine-api library

Summarizing the issue, import in the file (plugins/inputs/docker/docker.go) is as follows
import "golang.org/x/net/context"
docker container listing requires "golang.org/x/net/context" which takes context object as an argument, there are two repositories which have context in current code base,

  1. golang.org/x/net/context
  2. github.com/docker/docker/vendor/golang.org/x/net/context

During compilation based on length go is selecting the 2 repository. But method needs the 1st one golang.org/x/net/context. hence the type error in code snippet is recieved

Aliasing cannot be used here as libraries should not vendor other libraries.
Compilation goes through when "github.com/docker/docker/vendor/golang.org/x/net/context" is deleted. normally there are tools like glide which strip nested vendor directories. Telegraf uses gdm, if there is a way it can filter the vendor directory of docker or delete it. Then the PR #2379 will run successfully on circleci. I have made all the changes related to https://github.com/docker/docker/tree/master/client in the PR.

If the vendor thing is taken care of then the PR can be used. Please have a look

@sparrc sparrc changed the title docker engine-api library is deprecated docker engine-api library is deprecated, we should use their official SDK Feb 16, 2017
sparrc added a commit that referenced this issue Feb 19, 2017
sparrc added a commit that referenced this issue Feb 19, 2017
sparrc added a commit that referenced this issue Feb 19, 2017
sparrc added a commit that referenced this issue Feb 19, 2017
sparrc added a commit that referenced this issue Feb 22, 2017
@sparrc sparrc closed this as completed in 81408f9 Feb 22, 2017
maxunt pushed a commit that referenced this issue Jun 26, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants