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

Extend client API with custom HTTP requests #37071

Merged
merged 1 commit into from May 17, 2018

Conversation

mat007
Copy link
Contributor

@mat007 mat007 commented May 15, 2018

- What I did

I made the client underlying HTTP client accessible, useful for instance for docker/cli#1034

- How I did it

Added a HTTPClient() accessor to the Client interface.

- A picture of a cute animal (not mandatory but encouraged)

image

@cpuguy83
Copy link
Member

I don't really see the context of why this would be needed in the mentioned issue.

Also would it be better to add a method to get the HTTP client rather than having the API client itself perform the request? It seems weird to have the API client just do any random request.

@mat007
Copy link
Contributor Author

mat007 commented May 15, 2018

I don't really see the context of why this would be needed in the mentioned issue.

It might be a bit hard to follow with much of the context being lost due to the many rebases.
Basically this was suggested by @vdemeester to be able to rewrite docker/cli@0bf781e into docker/cli@05bbb2e

Also would it be better to add a method to get the HTTP client rather than having the API client itself perform the request? It seems weird to have the API client just do any random request.

I really don't have enough experience with the design of moby and docker/CLI to answer that.
However the initial need was indeed a way to get access to the underlying HTTP client in order to use its configuration (mainly credentials), so I'd be happy with a good old GetHTTPClient().
@vdemeester any though on this?

@vdemeester
Copy link
Member

@mat007 yeah having a exported method to get the client's http client would work too 👼

@cpuguy83
Copy link
Member

Or something to read the credentials. Is this just the tlsconfig? We could provide a copy of the tlsconfig easily enough.

@GordonTheTurtle GordonTheTurtle added the dco/no Automatically set by a bot when one of the commits lacks proper signature label May 15, 2018
@GordonTheTurtle GordonTheTurtle removed the dco/no Automatically set by a bot when one of the commits lacks proper signature label May 15, 2018
@mat007
Copy link
Contributor Author

mat007 commented May 15, 2018

@cpuguy83 I changed the PR to now expose a HTTPClient getter instead.

client/client.go Outdated
@@ -356,6 +356,11 @@ func (cli *Client) DaemonHost() string {
return cli.host
}

// HTTPClient returns the HTTP client bound to the server
func (cli *Client) HTTPClient() *http.Client {
return cli.client
Copy link
Member

Choose a reason for hiding this comment

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

Should this make a copy of the client?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I changed that. Still returning a pointer though as http.Client functions take a pointer as receptor and it's more convenient to directly get a pointer there.

@codecov
Copy link

codecov bot commented May 15, 2018

Codecov Report

❗ No coverage uploaded for pull request base (master@b7b6b69). Click here to learn what that means.
The diff coverage is 0%.

@@            Coverage Diff            @@
##             master   #37071   +/-   ##
=========================================
  Coverage          ?   35.09%           
=========================================
  Files             ?      615           
  Lines             ?    45820           
  Branches          ?        0           
=========================================
  Hits              ?    16080           
  Misses            ?    27630           
  Partials          ?     2110

Signed-off-by: Mathieu Champlon <mathieu.champlon@docker.com>
Copy link
Member

@cpuguy83 cpuguy83 left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants