Skip to content

Remove Job from Info API #12266

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.

Sign up for GitHub

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

Jump to bottom
Merged
cpuguy83 merged 1 commit into from
Apr 20, 2015
Merged

Remove Job from Info API #12266

cpuguy83 merged 1 commit into from
Apr 20, 2015

Conversation

@HuKeping
Copy link
Contributor

@HuKeping HuKeping commented Apr 10, 2015

a part of issue #12151

Signed-off-by: Hu Keping hukeping@huawei.com

@icecrime icecrime mentioned this pull request Apr 10, 2015
Closed
41 tasks
duglin
duglin reviewed Apr 10, 2015
View reviewed changes
daemon/info.go Outdated
Copy link
Contributor

@duglin duglin Apr 10, 2015

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@duglin duglin Apr 10, 2015

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@LK4D4 LK4D4 Apr 10, 2015

Choose a reason for hiding this comment

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

Yup, +1. Env is superbad.

Copy link
Contributor Author

@HuKeping HuKeping Apr 11, 2015

Choose a reason for hiding this comment

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

get it

LK4D4
LK4D4 reviewed Apr 10, 2015
View reviewed changes
api/server/server.go Outdated
Copy link
Contributor

@LK4D4 LK4D4 Apr 10, 2015

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@HuKeping HuKeping Apr 11, 2015

Choose a reason for hiding this comment

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

I'll update this

@HuKeping
Copy link
Contributor Author

HuKeping commented Apr 13, 2015

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

LK4D4
LK4D4 reviewed Apr 13, 2015
View reviewed changes
api/server/server.go Outdated
Copy link
Contributor

@LK4D4 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
Copy link
Contributor

LK4D4 commented Apr 13, 2015

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

@HuKeping
Copy link
Contributor Author

HuKeping commented Apr 13, 2015

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
Copy link
Contributor Author

HuKeping commented Apr 13, 2015

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
Copy link
Contributor

duglin commented Apr 13, 2015

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
LK4D4 reviewed Apr 13, 2015
View reviewed changes
integration-cli/docker_api_info_test.go Outdated
Copy link
Contributor

@LK4D4 LK4D4 Apr 13, 2015

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@HuKeping HuKeping Apr 14, 2015

Choose a reason for hiding this comment

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

ok

@HuKeping
Copy link
Contributor Author

HuKeping commented Apr 14, 2015

@duglin I'll do it.

@HuKeping
Copy link
Contributor Author

HuKeping commented Apr 14, 2015

update.

  • make change on CLI command docker info
  • upgrade test

ping @duglin @LK4D4

@HuKeping
Copy link
Contributor Author

HuKeping commented Apr 14, 2015

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
Copy link
Contributor Author

HuKeping commented Apr 17, 2015

rebase, ping @duglin @LK4D4

cpuguy83
cpuguy83 reviewed Apr 17, 2015
View reviewed changes
api/client/info.go Outdated
Copy link
Member

@cpuguy83 cpuguy83 Apr 17, 2015

Choose a reason for hiding this comment

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

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
Copy link
Member

cpuguy83 commented Apr 17, 2015

:( needs rebase.

@HuKeping
Copy link
Contributor Author

HuKeping commented Apr 20, 2015

@cpuguy83 rebased

@HuKeping
Copy link
Contributor Author

HuKeping commented Apr 20, 2015

ping @duglin @LK4D4

LK4D4
LK4D4 reviewed Apr 20, 2015
View reviewed changes
integration-cli/docker_api_info_test.go
Copy link
Contributor

@LK4D4 LK4D4 Apr 20, 2015

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@HuKeping HuKeping Apr 20, 2015

Choose a reason for hiding this comment

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

We have it under integration-cli/docker_cli_info_test.go

@LK4D4
Copy link
Contributor

LK4D4 commented Apr 20, 2015

LGTM

@HuKeping
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 <hukeping@huawei.com>
@cpuguy83
Copy link
Member

cpuguy83 commented Apr 20, 2015

LGTM

cpuguy83 added a commit that referenced this pull request Apr 20, 2015
@cpuguy83
Merge pull request #12266 from HuKeping/rmjob-info
dacc050 
Remove Job from Info API
@cpuguy83 cpuguy83 merged commit dacc050 into moby:master Apr 20, 2015
@HuKeping HuKeping deleted the rmjob-info branch April 20, 2015 11:13
@vieux vieux mentioned this pull request May 29, 2015
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

status/2-code-review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@HuKeping @LK4D4 @duglin @cpuguy83 @jessfraz @GordonTheTurtle