Migrate some kill integration cli test to api tests by vdemeester · Pull Request #36140 · moby/moby

vdemeester · 2018-01-29T22:43:51Z

Signed-off-by: Vincent Demeester vincent@sbr.pm

vdemeester · 2018-01-29T22:44:28Z

client/client.go

These changes on client are not strictly required (and might be better off discussed in a separate PR)

I think we've wanted to make this change for a while. What do you think about something like this:

create a func FromEnv(*Client) and move all the code from NewEnvClient (except the ops addition) to FromEnv()

create NewClientWithOpts() (or something like that) which creates a client from ops, but has no other inline setup.

remove ops from NewEnvClient and deprecate it. Replace the implementation with NewClientWithOpts(FromEnv).

That way the new interface would create a bare client{} and we can apply any ops onto it. If there are required fields (host and client?) maybe it would also validate that.

sounds very good to me 👼

vdemeester · 2018-01-29T22:46:28Z

integration-cli/docker_cli_kill_test.go

This one shouldn't be hard to port either

dnephin · 2018-01-30T00:19:58Z

client/client.go

I think we've wanted to make this change for a while. What do you think about something like this:

create a func FromEnv(*Client) and move all the code from NewEnvClient (except the ops addition) to FromEnv()

create NewClientWithOpts() (or something like that) which creates a client from ops, but has no other inline setup.

remove ops from NewEnvClient and deprecate it. Replace the implementation with NewClientWithOpts(FromEnv).

That way the new interface would create a bare client{} and we can apply any ops onto it. If there are required fields (host and client?) maybe it would also validate that.

dnephin · 2018-01-30T00:26:22Z

integration/container/kill_test.go

Instead of a comment this can be the last arg to skip.If() so that we see it in the test output. (same below).

good point 🙃

dnephin

looks like lint failures, and one comment, otherwise LGTM

dnephin · 2018-01-31T23:33:55Z

integration/container/kill_test.go

I think this already exists as containerIsInState in this package.

dnephin

LGTM

dnephin · 2018-01-31T23:59:05Z

integration/network/inspect_test.go

looks like this test failed

Cannot connect to the Docker daemon at unix:///tmp/docker-integration/df6a1d02502f7.sock

yongtang · 2018-02-05T14:57:15Z

Overall LGTM. Though powerpc build keeps panicking. Not sure if it is related (or it exposes some other unrelated issue).

cpuguy83

Seems mostly ok, the only thing is I don't think we can use defer setupTest(t)() with t.Parallel()... and I think we should opt for t.Parallel() over the cleanup helper.

cpuguy83 · 2018-02-05T14:40:52Z

integration/container/kill_test.go

I don't think this works with t.Parallel()
For what it's worth, the auto-cleanup is probably not needed as it's simple to cleanup manually.

Good catch. Although we don't actually use -parallel right now so these t.Parallel() don't do anything I believe.

I think we should make the extra effort to have these run parallel correctly, as we move stuff over it will be a huge help to test run times.

While trying to migrate some integration-cli tests to integration api tests, I noticed some of the tests rely on fixed "name" of containers, like foo, server, etc.. I think that may not work well with parallel. May need to convert them to random generated names to avoid collision.

Ya, I'm not against trying to get things running in parallel, but it is a lot more work than just adding t.Parallel().

It's a lot easier to get the cleanup wrong if we don't have an easy way to label everything with the test name.

May need to convert them to random generated names to avoid collision.

Where possible we should just capture the container ID that's returned

Yeah, I think we'd need to have the client automatically namespace objects or something then the defer could cleanup just the namespaces resources.

cpuguy83 · 2018-02-05T14:57:44Z

integration/container/kill_test.go

Just a performance concern here.. could this start up a "wait" goroutine earlier on for the case of it being stopped? (side thought, would be nice if the API let us wait for a container to be running).

Funny, just found #36147

This allows to create a client with default values and override those using functors. As an example, `NewEnvClient()` becomes `NewClientWithOpts(FromEnv)` ; and if you want a different api version for this client : `NewClientWithOpts(FromEnv, WithVersion("1.35"))` Signed-off-by: Vincent Demeester <vincent@sbr.pm>

Signed-off-by: Vincent Demeester <vincent@sbr.pm>

vdemeester · 2018-02-08T14:28:15Z

@cpuguy83 updated by removing t.Parallel() for now.

cpuguy83

LGTM

yongtang · 2018-02-08T15:06:12Z

@vdemeester I think there was an usage of NewClientForHost in secret_test.go which causes the build failure. Can you take a look?

vdemeester · 2018-02-08T15:17:06Z

@yongtang yep, I will 😉

Signed-off-by: Vincent Demeester <vincent@sbr.pm>

yongtang

LGTM

vdemeester requested a review from dnephin January 29, 2018 22:43

GordonTheTurtle added the status/0-triage label Jan 29, 2018

vdemeester commented Jan 29, 2018

View reviewed changes

vdemeester changed the title ~~Migration some kill integration cli test to api tests~~ Migrate some kill integration cli test to api tests Jan 29, 2018

vdemeester added status/2-code-review and removed status/0-triage labels Jan 29, 2018

vdemeester commented Jan 29, 2018

View reviewed changes

integration-cli/docker_cli_kill_test.go Outdated

Copy link

Member Author

vdemeester Jan 29, 2018

Choose a reason for hiding this comment

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

This one shouldn't be hard to port either

vdemeester force-pushed the integration-container-kill branch from ddde382 to 3ddcea8 Compare January 29, 2018 22:56

dnephin reviewed Jan 30, 2018

View reviewed changes

vdemeester force-pushed the integration-container-kill branch 4 times, most recently from 6ceb6d9 to 42d9145 Compare January 31, 2018 21:10

dnephin reviewed Jan 31, 2018

View reviewed changes

integration/container/kill_test.go Outdated

Copy link

Member

dnephin Jan 31, 2018

Choose a reason for hiding this comment

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

I think this already exists as containerIsInState in this package.

vdemeester force-pushed the integration-container-kill branch 3 times, most recently from 2d5f616 to 04abc4a Compare January 31, 2018 23:48

dnephin approved these changes Jan 31, 2018

View reviewed changes

dnephin reviewed Jan 31, 2018

View reviewed changes

vdemeester force-pushed the integration-container-kill branch from 04abc4a to 8979f0e Compare February 1, 2018 00:02

vdemeester added the rebuild/experimental label Feb 1, 2018

GordonTheTurtle removed the rebuild/experimental label Feb 1, 2018

vdemeester added the rebuild/powerpc label Feb 1, 2018

GordonTheTurtle removed the rebuild/powerpc label Feb 1, 2018

vdemeester force-pushed the integration-container-kill branch from 8979f0e to fe79ee0 Compare February 1, 2018 17:23

vdemeester added rebuild/experimental labels Feb 1, 2018

GordonTheTurtle removed rebuild/experimental labels Feb 1, 2018

vdemeester added the rebuild/powerpc label Feb 5, 2018

GordonTheTurtle removed the rebuild/powerpc label Feb 5, 2018

vdemeester requested review from cpuguy83, thaJeztah and yongtang February 5, 2018 08:59

cpuguy83 requested changes Feb 5, 2018

View reviewed changes

dnephin mentioned this pull request Feb 5, 2018

[client] Remove duplicate NewClient functions #34899

Merged

vdemeester force-pushed the integration-container-kill branch from fe79ee0 to 57fd33e Compare February 8, 2018 14:22

vdemeester added 2 commits February 8, 2018 15:26

Move some kill integration cli to api tests

2227c8a

Signed-off-by: Vincent Demeester <vincent@sbr.pm>

vdemeester force-pushed the integration-container-kill branch from 57fd33e to b1af229 Compare February 8, 2018 14:27

cpuguy83 approved these changes Feb 8, 2018

View reviewed changes

cpuguy83 added status/4-merge and removed status/2-code-review labels Feb 8, 2018

Migrate some calls to new client function

6977f46

Signed-off-by: Vincent Demeester <vincent@sbr.pm>

vdemeester force-pushed the integration-container-kill branch from b1af229 to 6977f46 Compare February 8, 2018 15:22

yongtang approved these changes Feb 8, 2018

View reviewed changes

yongtang merged commit 21c9e57 into moby:master Feb 8, 2018

vdemeester deleted the integration-container-kill branch February 8, 2018 17:56

thaJeztah mentioned this pull request Feb 9, 2018

Fix TLS from environment variables in client #36270

Merged

thaJeztah added area/api API area/cli Client impact/deprecation labels Aug 23, 2022

This was referenced Aug 23, 2022

client: disable deprecated NewClient() and NewEnvClient() #44019

Closed

[22.06 backport] client: disable deprecated NewClient() and NewEnvClient() #44023

Closed

thaJeztah mentioned this pull request Jul 23, 2025

client: remove deprecated types and functions #50485

Merged

Conversation

vdemeester commented Jan 29, 2018

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 Jan 30, 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!

dnephin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dnephin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yongtang commented Feb 5, 2018

Uh oh!

cpuguy83 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dnephin Feb 5, 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!

vdemeester commented Feb 8, 2018

Uh oh!

cpuguy83 left a comment

Choose a reason for hiding this comment

Uh oh!

yongtang commented Feb 8, 2018

Uh oh!

vdemeester commented Feb 8, 2018

Uh oh!

yongtang left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

dnephin Jan 30, 2018 •

edited

Loading

dnephin Feb 5, 2018 •

edited

Loading