[client] Remove duplicate NewClient functions by dnephin · Pull Request #34899 · moby/moby

dnephin · 2017-09-19T21:19:10Z

Improve docstrings in client package
Remove some unnecessary code in client
Add more ops to client
Remove duplicate ways of getting an APIClient for integration testing
Remove some deprecated code

vdemeester

LGTM 🐮

thaJeztah · 2017-09-20T13:01:24Z

CI is failing 😢

dnephin · 2018-02-16T20:07:18Z

client/client.go

I removed this because NewClientWithOpts always sets a default c.client, so there is no way for this branch to ever be hit. This is related to the fix from #36270. Before that fix we always set a new defaultHTTPClient, but now we never set a client because c.client is already set. cc @dperny

codecov · 2018-02-16T22:29:34Z

Codecov Report

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

@@            Coverage Diff            @@
##             master   #34899   +/-   ##
=========================================
  Coverage          ?   34.31%           
=========================================
  Files             ?      609           
  Lines             ?    45308           
  Branches          ?        0           
=========================================
  Hits              ?    15549           
  Misses            ?    27759           
  Partials          ?     2000

dnephin · 2018-02-16T22:32:06Z

hack/make/.integration-daemon-start

I removed this because it was causing DockerSuite.TestBuildWithSession to fail.

I don't think we should be setting an explicit API version here. The client package uses this version to disable some features, so setting it to an old version makes it impossible to test those features.

I believe the pinned version of the cli we are using can properly negotiate the API version.

vdemeester

Still LGTM 🦁

thaJeztah

Left one question (related to the gotestyourself package), also, can you squash ca1b31e (Remove duplicate calls for getting an APIClient) and 78c097c (revert changes to exec resize)?

thaJeztah · 2018-02-19T10:34:41Z

client/client_test.go

I noticed env.Patch() and env.PatchAll() have a difference in behaviour; where env.Patch() actually patches the current environment variables, env.PatchAll() is overriding the current environment (i.e., it removes all existing environment variables); https://github.com/gotestyourself/gotestyourself/blob/511344eed30e4384f010579a593dfb442033a692/env/env.go#L40-L45

I wonder if that function needs to have a different name, or if the os.ClearEnv() should be removed

Yes, that is exactly how they are documented: https://godoc.org/github.com/gotestyourself/gotestyourself/env

Patch changes the value of an environment variable

PatchAll sets the environment to env

I wonder if that function needs to have a different name,

All -> "the whole of", complete, entire

I think the name should imply that it replaces everything. What did you expect PatchAll to do?

If it were just a "multi set" version of Patch it would be called something like PatchMany, or MatchMulti, but I think there is little value in such a call, so would not be in favor of adding it.

The “Patch” part lead me to believe I’d be modifying just what I sent, but keep other values as-is; even more because the “Patch” does so for a single var.

idk, “SetEnv()” / “Override()” would perhaps perhaps be closed to that it does

I think SetEnv would be too close to https://golang.org/pkg/os/#Setenv, and would be confusing because it's not just SetEnv, it also returns a cleanup function. Override doesn't really have any specific meaning in this context.

Patch here, in the context of testing, refers to Monkey Patching, not the more general patch as in "partial change".

Perhaps that's where my confusion was; anyway, it's not really related to this PR 😅

thaJeztah · 2018-02-19T10:41:52Z

integration-cli/docker_api_exec_resize_test.go

How did this test pass previously on Windows?

There are some big problems with this test. I think it's actually failing on windows, but all ignores are being ignored because it's ignoring all errors except EOF:

moby/integration-cli/docker_api_exec_resize_test.go

Lines 68 to 79 in 733ed2d

_, rc, err := request.Post(fmt.Sprintf("/exec/%s/resize?h=24&w=80", execID), request.ContentType("text/plain"))

// It's probably a panic of the daemon if io.ErrUnexpectedEOF is returned.

if err == io.ErrUnexpectedEOF {

return fmt.Errorf("The daemon might have crashed.")

}

if err == nil {

rc.Close()

}

// We only interested in the io.ErrUnexpectedEOF error, so we return nil otherwise.

return nil

I tried to fix it, but it was failing, so I decided to leave it for another time.

Opened #36354 for this issue

thaJeztah · 2018-02-19T10:47:31Z

integration-cli/docker_api_exec_resize_test.go

Ah, it's re-added here 😅

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

Use client instead of helpers for TLS in integration test Signed-off-by: Daniel Nephin <dnephin@docker.com>

Remove request.SockRequest Remove request.SockRequestHijack Remove request.SockRequestRaw() Remove deprecated ParseHost Deprecate and unexport more helpers Signed-off-by: Daniel Nephin <dnephin@docker.com>

Use the default version because it is used by the client package Signed-off-by: Daniel Nephin <dnephin@docker.com>

dnephin · 2018-02-20T23:35:46Z

Squashed those 2 commits and fixed a test that was failing

vdemeester

LGTM 🐸

thaJeztah

LGTM, thanks!

dnephin requested a review from vdemeester September 19, 2017 21:19

GordonTheTurtle added the status/0-triage label Sep 19, 2017

dnephin added status/2-code-review and removed status/0-triage labels Sep 19, 2017

vdemeester approved these changes Sep 20, 2017

View reviewed changes

thaJeztah added the status/failing-ci Indicates that the PR in its current state fails the test suite label Sep 20, 2017

dnephin force-pushed the fix-duplicate-new-client branch from 69e0266 to ccf5103 Compare September 20, 2017 18:32

dnephin removed the status/failing-ci Indicates that the PR in its current state fails the test suite label Sep 20, 2017

dnephin force-pushed the fix-duplicate-new-client branch 3 times, most recently from b2990da to 818393f Compare September 20, 2017 22:15

dnephin added rebuild/experimental labels Sep 21, 2017

GordonTheTurtle removed rebuild/experimental labels Sep 21, 2017

dnephin added rebuild/janky labels Sep 21, 2017

GordonTheTurtle removed rebuild/janky labels Sep 21, 2017

dnephin force-pushed the fix-duplicate-new-client branch 3 times, most recently from 2ba31f4 to a65413e Compare September 22, 2017 18:15

GordonTheTurtle added the dco/no Automatically set by a bot when one of the commits lacks proper signature label Sep 22, 2017

dnephin force-pushed the fix-duplicate-new-client branch from d1fe81f to 7c09dc5 Compare September 22, 2017 21:04

GordonTheTurtle removed the dco/no Automatically set by a bot when one of the commits lacks proper signature label Sep 22, 2017

yongtang added the rebuild/experimental label Sep 25, 2017

GordonTheTurtle removed the rebuild/experimental label Sep 25, 2017

moby deleted a comment from GordonTheTurtle Sep 25, 2017

dnephin mentioned this pull request Sep 26, 2017

Add a Close method on DockerCli docker/cli#554

Closed

dnephin force-pushed the fix-duplicate-new-client branch from 3d4ae0e to ca1b31e Compare February 16, 2018 20:15

dnephin commented Feb 16, 2018

View reviewed changes

dnephin changed the title ~~[client] Remove duplicate ways of getting a client in integration/request~~ [client] Remove duplicate NewClient functions Feb 16, 2018

dnephin requested a review from tianon as a code owner February 16, 2018 22:29

dnephin commented Feb 16, 2018

View reviewed changes

vdemeester approved these changes Feb 19, 2018

View reviewed changes

thaJeztah requested changes Feb 19, 2018

View reviewed changes

dnephin added rebuild/windowsRS1 labels Feb 19, 2018

GordonTheTurtle removed rebuild/experimental labels Feb 19, 2018

dnephin added 2 commits February 20, 2018 15:15

Use gotestyourself env patching

2b445a5

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

Improve docstrings and small cleanup in client

a68ae4a

Use client instead of helpers for TLS in integration test Signed-off-by: Daniel Nephin <dnephin@docker.com>

dnephin force-pushed the fix-duplicate-new-client branch from 115c2ea to 7ad8dff Compare February 20, 2018 20:48

dnephin added rebuild/janky labels Feb 20, 2018

GordonTheTurtle removed rebuild/janky labels Feb 20, 2018

dnephin added 2 commits February 20, 2018 17:27

Remove duplicate calls for getting an APIClient

0a91ba2

Remove request.SockRequest Remove request.SockRequestHijack Remove request.SockRequestRaw() Remove deprecated ParseHost Deprecate and unexport more helpers Signed-off-by: Daniel Nephin <dnephin@docker.com>

Remove explicit DOCKER_API_VERSION from integration env setup

e73d742

Use the default version because it is used by the client package Signed-off-by: Daniel Nephin <dnephin@docker.com>

dnephin force-pushed the fix-duplicate-new-client branch from 7ad8dff to e73d742 Compare February 20, 2018 22:27

vdemeester approved these changes Feb 21, 2018

View reviewed changes

thaJeztah approved these changes Feb 21, 2018

View reviewed changes

thaJeztah merged commit 466cc98 into moby:master Feb 21, 2018

thaJeztah mentioned this pull request Feb 21, 2018

Bump moby to 0ede01237c9ab871f1b8db0364427407f3e46541 docker/cli#894

Merged

dnephin deleted the fix-duplicate-new-client branch February 21, 2018 17:30

thaJeztah mentioned this pull request Jul 29, 2021

client: remove deprecated SetCustomHTTPHeaders(), CustomHTTPHeaders() #42694

Merged

thaJeztah mentioned this pull request Jul 23, 2025

client: remove deprecated types and functions #50485

Merged

	_, rc, err := request.Post(fmt.Sprintf("/exec/%s/resize?h=24&w=80", execID), request.ContentType("text/plain"))
	// It's probably a panic of the daemon if io.ErrUnexpectedEOF is returned.
	if err == io.ErrUnexpectedEOF {
	return fmt.Errorf("The daemon might have crashed.")
	}

	if err == nil {
	rc.Close()
	}

	// We only interested in the io.ErrUnexpectedEOF error, so we return nil otherwise.
	return nil

Conversation

dnephin commented Sep 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vdemeester left a comment

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Sep 20, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Feb 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vdemeester left a comment

Choose a reason for hiding this comment

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dnephin Feb 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dnephin commented Feb 20, 2018

Uh oh!

vdemeester left a comment

Choose a reason for hiding this comment

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

dnephin commented Sep 19, 2017 •

edited

Loading

codecov bot commented Feb 16, 2018 •

edited

Loading

dnephin Feb 19, 2018 •

edited

Loading