Skip to content

Remove Job from docker images #12184

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
LK4D4 merged 1 commit into from
Apr 9, 2015
Merged

Remove Job from docker images #12184

LK4D4 merged 1 commit into from
Apr 9, 2015

Conversation

@duglin
Copy link
Contributor

@duglin duglin commented Apr 8, 2015

Also removes engine.Table

Signed-off-by: Doug Davis dug@us.ibm.com

@duglin
Copy link
Contributor Author

duglin commented Apr 8, 2015

Requires #12163 to be merged first

@icecrime icecrime mentioned this pull request Apr 8, 2015
Closed
41 tasks
@duglin duglin force-pushed the RemoveJobImages branch from 07ff292 to 987dcf2 Compare April 8, 2015 20:54
LK4D4
LK4D4 reviewed Apr 8, 2015
View reviewed changes
integration/utils_test.go Outdated
Copy link
Contributor

@LK4D4 LK4D4 Apr 8, 2015

Choose a reason for hiding this comment

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

Can we just copy behavior of getDaemon here? Because I'm not happy with exposing getDaemon from server just for integration tests. Also this should reduce size of patch.

Copy link
Contributor Author

@duglin duglin Apr 8, 2015

Choose a reason for hiding this comment

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

Sure - done!

@duglin duglin force-pushed the RemoveJobImages branch from 987dcf2 to a63938d Compare April 8, 2015 21:04
jessfraz
jessfraz reviewed Apr 8, 2015
View reviewed changes
api/server/server.go
Copy link
Contributor

@jessfraz jessfraz Apr 8, 2015

Choose a reason for hiding this comment

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

are these to keep api < v1.7 working? I would almost say since docker 0.7 uses v1.7 and I couldnt get any cli to work that was less than that, we could remove anything for < v1.7 for api but idk

Copy link
Contributor Author

@duglin duglin Apr 8, 2015

Choose a reason for hiding this comment

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

@jfrazelle this is similar to what we played with yesterday and probably can be removed (and I wouldn't mind) but I didn't make a fuss about this one because generating this legacy json was easy, while the one we were looking at yesterday was a pain.
If people want it removed, I can do so.... just need to hear some opinions.

Copy link
Contributor

@jessfraz jessfraz Apr 8, 2015

Choose a reason for hiding this comment

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

I don't mind if we keep it, just saying it doesnt work anyways ;)

Copy link
Contributor Author

@duglin duglin Apr 8, 2015

Choose a reason for hiding this comment

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

actually this does appear to work for me. Yesterday we were playing with 'ps' not 'images'.

@duglin duglin force-pushed the RemoveJobImages branch 2 times, most recently from 824aace to 227aa5b Compare April 8, 2015 21:08
LK4D4
LK4D4 reviewed Apr 8, 2015
View reviewed changes
graph/list.go Outdated
Copy link
Contributor

@LK4D4 LK4D4 Apr 8, 2015

Choose a reason for hiding this comment

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

Can we rename it to Images.

Copy link
Contributor

@LK4D4 LK4D4 Apr 8, 2015

Choose a reason for hiding this comment

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

Also maybe []*types.Image

Copy link
Contributor Author

@duglin duglin Apr 8, 2015

Choose a reason for hiding this comment

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

ok got both

@duglin duglin force-pushed the RemoveJobImages branch from 227aa5b to 7cd9d1e Compare April 8, 2015 21:25
vieux
vieux reviewed Apr 8, 2015
View reviewed changes
integration/utils_test.go
Copy link
Contributor

@vieux vieux Apr 8, 2015

Choose a reason for hiding this comment

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

just a quick comment, I didn't read the PR thoroughly, but we introduced HackGetGlobalVar when we introduced jobs. I'm surprised to see this as the goal of this PR is to remove jobs.

Copy link
Contributor

@crosbymichael crosbymichael Apr 8, 2015

Choose a reason for hiding this comment

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

I think this is just temporary for now

Copy link
Contributor

@vieux vieux Apr 8, 2015

Choose a reason for hiding this comment

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

ok great

@duglin duglin force-pushed the RemoveJobImages branch from 7cd9d1e to 61120d1 Compare April 8, 2015 21:48
@duglin
Copy link
Contributor Author

duglin commented Apr 8, 2015

Any other comments?

@crosbymichael
Copy link
Contributor

crosbymichael commented Apr 8, 2015

LGTM

Remove Job from docker images
d045b97 
Also removes engine.Table

Signed-off-by: Doug Davis <dug@us.ibm.com>
@duglin duglin force-pushed the RemoveJobImages branch from 61120d1 to d045b97 Compare April 9, 2015 02:33
@duglin
Copy link
Contributor Author

duglin commented Apr 9, 2015

I just realized that my check for boolean query params was incorrect. I was looking just for "1" so I added a toBool() func to mimic what the old code did (", no, 0, none, false all mapped to 'false', everything else maps to 'true'). Personally I think its a bit backwards, I would prefer to look for true/1/... and default everything else to 'false' but I didn't want to change the current behavior.

@LK4D4
Copy link
Contributor

LK4D4 commented Apr 9, 2015

@duglin Yup, we noticed this too. @runcom was first in #12197
He added toBool too.
Yes, current behavior is awful :)

@duglin
Copy link
Contributor Author

duglin commented Apr 9, 2015

any other comments?

@runcom runcom mentioned this pull request Apr 9, 2015
Merged
@LK4D4
Copy link
Contributor

LK4D4 commented Apr 9, 2015

@duglin Just to be sure - did you fix PS from your previous PR?

@duglin
Copy link
Contributor Author

duglin commented Apr 9, 2015

do you mean the "1"/true stuff? If so,I think so - I tried to get all of the stuff in api/server/server.go.

@LK4D4
Copy link
Contributor

LK4D4 commented Apr 9, 2015

LGTM

LK4D4 added a commit that referenced this pull request Apr 9, 2015
@LK4D4
Merge pull request #12184 from duglin/RemoveJobImages
6ba7bf4 
Remove Job from `docker images`
@LK4D4 LK4D4 merged commit 6ba7bf4 into moby:master Apr 9, 2015
@duglin duglin deleted the RemoveJobImages branch April 9, 2015 22:51
@HuKeping
Copy link
Contributor

HuKeping commented Apr 10, 2015

hi @duglin why you remove the TestGetImagesJSON, we don't need to test docker image anymore?

@duglin
Copy link
Contributor Author

duglin commented Apr 10, 2015

@HuKeping if I remember correctly, it was because there wasn't a way to get to a daemon from within those testcases, plus the integration-cli tests should have covered it.

@HuKeping
Copy link
Contributor

HuKeping commented Apr 10, 2015

Thanks @duglin , I am removing Job from docker history and come into a similar issue that break the CI, so come here to ask for some advice :)

@runcom runcom mentioned this pull request May 6, 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

@duglin @crosbymichael @LK4D4 @HuKeping @vieux @jessfraz @icecrime @GordonTheTurtle