Skip to content

Remove job from container_inspect #12406

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 23, 2015
Merged

Remove job from container_inspect #12406

cpuguy83 merged 1 commit into from
Apr 23, 2015

Conversation

@runcom
Copy link
Member

@runcom runcom commented Apr 15, 2015

/cc @LK4D4 @cpuguy83 @duglin @icecrime

I'm opening this to get suggestions to finalize this work. Basically I'd like suggestions about ContainerJSON struct in daemon
It can't be put into api/types because it depends on runconfig which in turn depends on api
Clients that could use this struct are for instance api/client/logs and I wouldn't import daemon code here so I just created a type struct to extract the info I needed from the json response, is this that bad? Basically I like the idea to non couple client/server/daemon and just extract infos needed with a custom struct just to have a type to decode into (yes you have to maintain two things then but clients always adjust in response to server/api changes isn't it?).

Tests will probably fail because this isn't yet finished. but logs tests run ok with this modifications! I'll fix everything later.

Signed-off-by: Antonio Murdaca me@runcom.ninja

runcom
runcom reviewed Apr 15, 2015
View reviewed changes
api/client/logs.go Outdated
Copy link
Member Author

@runcom runcom Apr 15, 2015

Choose a reason for hiding this comment

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

I'll move this struct outside the function if it's ok

Copy link
Contributor

@LK4D4 LK4D4 Apr 16, 2015

Choose a reason for hiding this comment

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

probably okay here.

@icecrime icecrime mentioned this pull request Apr 15, 2015
Closed
41 tasks
@LK4D4
Copy link
Contributor

LK4D4 commented Apr 16, 2015

Look okay, but tests failing and need rebase.

@runcom
Copy link
Member Author

runcom commented Apr 16, 2015

Sure I didn't finished because I was not sure about the approach with local struct

@runcom runcom force-pushed the remove-job-container-inspect branch from b7996e2 to cbb5e60 Compare April 16, 2015 17:27
@runcom runcom force-pushed the remove-job-container-inspect branch from cbb5e60 to 00db6e6 Compare April 16, 2015 17:54
@calavera
Copy link
Contributor

calavera commented Apr 17, 2015

The code looks good. We can probably live with those structs in the code, but we can also move them later if they start to bother us.

Waiting for the new test in integration-cli before giving my 👍

@runcom
Copy link
Member Author

runcom commented Apr 17, 2015

@calavera yup I don't really like them but wanted not to couple daemon code with api

tiborvass
tiborvass reviewed Apr 17, 2015
View reviewed changes
api/client/attach.go Outdated
Copy link
Contributor

@tiborvass tiborvass Apr 17, 2015

Choose a reason for hiding this comment

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

It's the best solution for now, but maybe the refactoring problem comes from the fact that runconfig is a mess too. So maybe we could add a little TODO with a question like: Should we reconsider having a type in api/types with a refactored runconfig?

@LK4D4 wdyt?

Copy link
Contributor

@tiborvass tiborvass Apr 17, 2015

Choose a reason for hiding this comment

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

Same for the other structs below in other files

Copy link
Member Author

@runcom runcom Apr 17, 2015

Choose a reason for hiding this comment

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

I couldn't put the struct in types because runconfig depends on opts that depends on api again to validate host, I have a pull request "fixing" this but the support to the old raw struct still remains and I can't put it in types because it depends on daemon (and daemon has dependency on api/types) (hope you understand what I wrote lol)

Copy link
Contributor

@LK4D4 LK4D4 Apr 17, 2015

Choose a reason for hiding this comment

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

Yeah, comment won't hurt.

@runcom runcom force-pushed the remove-job-container-inspect branch from 00db6e6 to 372b374 Compare April 17, 2015 17:52
@runcom runcom force-pushed the remove-job-container-inspect branch from 372b374 to da59ff9 Compare April 18, 2015 13:07
@runcom runcom force-pushed the remove-job-container-inspect branch from da59ff9 to 836d3ef Compare April 18, 2015 13:47
@runcom
Copy link
Member Author

runcom commented Apr 18, 2015

@LK4D4 @tiborvass @calavera TODO comments added, I noticed test case I was going to move to integration-cli is already covered in https://github.com/docker/docker/blob/master/integration-cli/docker_api_inspect_test.go#L10 (what's tested is about < 1.12 json response format) so everything should be good now

@LK4D4
Copy link
Contributor

LK4D4 commented Apr 18, 2015

LGTM

@runcom runcom force-pushed the remove-job-container-inspect branch from 836d3ef to 9790909 Compare April 19, 2015 10:27
duglin
duglin reviewed Apr 19, 2015
View reviewed changes
daemon/inspect.go Outdated
Copy link
Contributor

@duglin duglin Apr 19, 2015

Choose a reason for hiding this comment

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

So would it make sense to move these info api/types?

Copy link
Member Author

@runcom runcom Apr 19, 2015

Choose a reason for hiding this comment

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

you can't as of now

Copy link
Contributor

@duglin duglin Apr 19, 2015

Choose a reason for hiding this comment

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

as we discussion via irc - we can if we just have the stuff that appears on the wire in the api/type struct.

@tiborvass
Copy link
Contributor

tiborvass commented Apr 20, 2015

@runcom how about adding a comment in runconfig to make sure the structs in api/client get updated if ever runconfig were to get updated? Not a perfect solution, but at least it will force us to refactor runconfig too at some point.

PS: I can't help to think: runcomfig :P

@runcom runcom force-pushed the remove-job-container-inspect branch from 9790909 to 2459f98 Compare April 20, 2015 18:14
@runcom
Copy link
Member Author

runcom commented Apr 20, 2015

rebase madness after @LK4D4 merge lol
@tiborvass updated with comment above runconfig Config struct
/cc @duglin @calavera

@runcom runcom force-pushed the remove-job-container-inspect branch from 2459f98 to d87ce7f Compare April 20, 2015 18:30
tiborvass
tiborvass reviewed Apr 20, 2015
View reviewed changes
api/types/types.go Outdated
Copy link
Contributor

@tiborvass tiborvass Apr 20, 2015

Choose a reason for hiding this comment

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

@LK4D4 @crosbymichael is this what we want?

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.

Looks okay for me.

Copy link
Contributor

@duglin duglin Apr 20, 2015

Choose a reason for hiding this comment

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

As long as it matches what is sent on the wire. As I mentioned in irc though, I'd want the full json represented, not just the bits the CLI uses.

Copy link
Member Author

@runcom runcom Apr 20, 2015

Choose a reason for hiding this comment

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

ok with that @duglin, working on it even if I don't like it because it's far better thinking about refactoring runconfig/opts so it can be included in api/types instead of rewriting each field in that really big struct, I don't really think it makes sense to do similar struct again..
@tiborvass Also this way there will be many new struct of those in api/types.. like ContainerConfigEntrypoint, ContainerNetworkSettings , ContainerConfigCommand and many more (me don't like)
We already have these structs elsewhere..

Copy link
Contributor

@tiborvass tiborvass Apr 20, 2015

Choose a reason for hiding this comment

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

I'm not sure anymore :( Ping @crosbymichael

Copy link
Member Author

@runcom runcom Apr 20, 2015

Choose a reason for hiding this comment

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

me neither, I'm waiting before finishing then, @tiborvass can't we just merge with those structs for now and move on to main runconfig refactor to fit in api/types? wdyt? also @duglin @LK4D4

Copy link
Contributor

@tiborvass tiborvass Apr 20, 2015

Choose a reason for hiding this comment

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

I want us to decide which way to go or else we'll forget about it and start building cruft again :(

@runcom runcom force-pushed the remove-job-container-inspect branch from d87ce7f to 3b51092 Compare April 22, 2015 22:15
@runcom runcom force-pushed the remove-job-container-inspect branch from 3b51092 to cd326da Compare April 22, 2015 22:18
@runcom runcom force-pushed the remove-job-container-inspect branch from cd326da to a45bbf7 Compare April 22, 2015 22:21
@runcom runcom force-pushed the remove-job-container-inspect branch from a45bbf7 to 4a9ea95 Compare April 22, 2015 22:45
@runcom runcom force-pushed the remove-job-container-inspect branch from 4a9ea95 to 95672e9 Compare April 22, 2015 22:53
runcom
runcom reviewed Apr 22, 2015
View reviewed changes
daemon/daemon.go
Copy link
Member Author

@runcom runcom Apr 22, 2015

Choose a reason for hiding this comment

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

🤘

Remove job from container_inspect
4b9fe9c 
Signed-off-by: Antonio Murdaca <me@runcom.ninja>
@runcom runcom force-pushed the remove-job-container-inspect branch from 95672e9 to 4b9fe9c Compare April 22, 2015 22:58
@LK4D4
Copy link
Contributor

LK4D4 commented Apr 23, 2015

Looks nice
LGTM

@cpuguy83
Copy link
Member

cpuguy83 commented Apr 23, 2015

LGTM

cpuguy83 added a commit that referenced this pull request Apr 23, 2015
@cpuguy83
Merge pull request #12406 from runcom/remove-job-container-inspect
3872272 
Remove job from container_inspect
@cpuguy83 cpuguy83 merged commit 3872272 into moby:master Apr 23, 2015
@runcom runcom deleted the remove-job-container-inspect branch April 23, 2015 06:55
@vieux vieux mentioned this pull request May 5, 2015
Closed
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.

8 participants

@runcom @LK4D4 @calavera @tiborvass @cpuguy83 @duglin @jessfraz @GordonTheTurtle