Remove Job from Info API by HuKeping · Pull Request #12266 · moby/moby

HuKeping · 2015-04-10T09:25:59Z

a part of issue #12151

Signed-off-by: Hu Keping [email protected]

duglin · 2015-04-10T12:41:52Z

daemon/info.go

Even if the result is just an array of strings, let's either create a new type in api/types or just use []string instead of engine.Env because I'm hoping engine.Env goes away, eventually.

Is there any reason we're not typing all of the results here? IOW, create a real struct for all of the data with the proper types info.

Yup, +1. Env is superbad.

LK4D4 · 2015-04-10T20:33:41Z

api/server/server.go

It is better to use simple form without else. Also it is recommended by google:
https://code.google.com/p/go-wiki/wiki/CodeReviewComments#Indent_Error_Flow

I'll update this

HuKeping · 2015-04-13T04:50:51Z

Updated-add a structure Info, is this change good to you? ping @LK4D4 @duglin

LK4D4 · 2015-04-13T05:08:24Z

api/server/server.go

Just pass info, should work.

LK4D4 · 2015-04-13T05:09:13Z

Ok for me. What about test? Maybe there is already some in integration-cli.

HuKeping · 2015-04-13T06:06:38Z

I am plan to add a new file integration-cli/docker_api_info_test.go to do the test. It's on the way.

HuKeping · 2015-04-13T06:48:46Z

Test ready. @LK4D4 @jfrazelle

Since the CLI command docker info has been checked by docker_cli_info_test.go, I think there is only lack of checking for REST API GET /info. I put it into a new file docker_api_info_test.go.

duglin · 2015-04-13T12:56:00Z

Please squash your commits down to one.
Overall this looks good.
We need to also modify the CLI code to use a structure too. Would you like to do that one as well? If so, we could do it in this PR at the same time if you're up for it. If not we can do it in a separate PR. Just let us know.

LK4D4 · 2015-04-13T15:53:33Z

integration-cli/docker_api_info_test.go

sockRequest changed signature in master. Now you can get StatusCode and check it. Also maybe makes sense to unmarshall body to some temp structure for checking that important fields is in there.

HuKeping · 2015-04-14T01:29:55Z

@duglin I'll do it.

HuKeping · 2015-04-14T07:16:33Z

update.

make change on CLI command docker info
upgrade test

ping @duglin @LK4D4

HuKeping · 2015-04-14T09:32:30Z

I'm trying to fix this CI error:
daemon/execdriver/native/exec.go:13:2: C source files not allowed when not using cgo: nsexec.c

HuKeping · 2015-04-17T08:19:21Z

rebase, ping @duglin @LK4D4

cpuguy83 · 2015-04-17T12:44:45Z

api/client/info.go

I don't think we need this error message here since we're returning error below.
Maybe update the error with this extra info, though?

cpuguy83 · 2015-04-17T19:32:50Z

:( needs rebase.

HuKeping · 2015-04-20T02:59:29Z

@cpuguy83 rebased

HuKeping · 2015-04-20T04:20:11Z

ping @duglin @LK4D4

LK4D4 · 2015-04-20T04:20:12Z

integration-cli/docker_api_info_test.go

Can we have simple test for cli too? Because it was changed. Or maybe there is already tests?

We have it under integration-cli/docker_cli_info_test.go

LK4D4 · 2015-04-20T05:06:01Z

LGTM

Two main things - Create a real struct Info for all of the data with the proper types - Add test for REST API get info Signed-off-by: Hu Keping <[email protected]>

cpuguy83 · 2015-04-20T11:10:53Z

LGTM

Remove Job from Info API

GordonTheTurtle added the status/0-triage label Apr 10, 2015

icecrime mentioned this pull request Apr 10, 2015

Remove engine mechanism #12151

Closed

41 tasks

duglin reviewed Apr 10, 2015
View reviewed changes

cpuguy83 added status/2-code-review and removed status/0-triage labels Apr 10, 2015

LK4D4 reviewed Apr 10, 2015
View reviewed changes

LK4D4 reviewed Apr 13, 2015
View reviewed changes

api/server/server.go Outdated

Copy link

Contributor

LK4D4 Apr 13, 2015

Choose a reason for hiding this comment

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

Just pass info, should work.

LK4D4 reviewed Apr 13, 2015
View reviewed changes

GordonTheTurtle added dco/yes and removed dco/no labels Apr 14, 2015

GordonTheTurtle added dco/yes and removed dco/no labels Apr 17, 2015

cpuguy83 reviewed Apr 17, 2015
View reviewed changes

GordonTheTurtle added dco/yes and removed dco/no labels Apr 17, 2015

GordonTheTurtle added dco/yes and removed dco/no labels Apr 20, 2015

LK4D4 reviewed Apr 20, 2015
View reviewed changes

Remove Job from Info API

f4942ed

Two main things - Create a real struct Info for all of the data with the proper types - Add test for REST API get info Signed-off-by: Hu Keping <[email protected]>

cpuguy83 added a commit that referenced this pull request Apr 20, 2015

Merge pull request #12266 from HuKeping/rmjob-info

dacc050

Remove Job from Info API

cpuguy83 merged commit dacc050 into moby:master Apr 20, 2015

HuKeping deleted the rmjob-info branch April 20, 2015 11:13

vieux mentioned this pull request May 29, 2015

do not print empty values in docker info #13607

Merged

Comments

Conversation

HuKeping commented Apr 10, 2015

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!

HuKeping commented Apr 13, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LK4D4 commented Apr 13, 2015

Uh oh!

HuKeping commented Apr 13, 2015

Uh oh!

HuKeping commented Apr 13, 2015

Uh oh!

duglin commented Apr 13, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HuKeping commented Apr 14, 2015

Uh oh!

HuKeping commented Apr 14, 2015

Uh oh!

HuKeping commented Apr 14, 2015

Uh oh!

HuKeping commented Apr 17, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cpuguy83 commented Apr 17, 2015

Uh oh!

HuKeping commented Apr 20, 2015

Uh oh!

HuKeping commented Apr 20, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LK4D4 commented Apr 20, 2015

Uh oh!

cpuguy83 commented Apr 20, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants