Skip to content
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

Drop engine api #834

Merged
merged 5 commits into from
Nov 28, 2017
Merged

Drop engine api #834

merged 5 commits into from
Nov 28, 2017

Conversation

soltysh
Copy link
Contributor

@soltysh soltysh commented Nov 21, 2017

Fixes #833.

@openshift/sig-master fyi, this should in the end help to limit the pkg/builder vendored dirs as well

@openshift-ci-robot openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Nov 21, 2017
soltysh
soltysh commented Nov 21, 2017
View reviewed changes
pkg/docker/docker.go Outdated
}
// FIXME
exitCode := 0
// exitCode, err := d.client.ContainerWait(context.Background(), container.ID)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the only bit that is left for me to finish it. It looks like they've changed it significantly and worst thing is that the current conditions (WaitConditionNotRunning, WaitConditionNextExit, WaitConditionRemoved) don't fit us. I was hoping the WiatConditionNextExit will, but if the container exits in between this will block until next exit, which is not quite right. I'll dig a bit more into it, and will see what's possible and what not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bparees ^

@bparees
Copy link
Contributor

bparees commented Nov 21, 2017

lgtm pending resolution of the FIXME bits.

@bparees bparees mentioned this pull request Nov 22, 2017
Closed
@soltysh soltysh mentioned this pull request Nov 23, 2017
Merged
19 tasks
@soltysh
Copy link
Contributor Author

soltysh commented Nov 23, 2017

@bparees I've fixed the appropriate bits, but I've noticed we're not parsing the docker image spec properly, it was silently falling on reference.ParseNamed which requires canonical name, iow. registry/namespace/image, I changed that to reference.WithName similarly to what we did in origin during rebase. ptal

@soltysh soltysh changed the title WIP: Drop engine api Drop engine api Nov 23, 2017
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 23, 2017
@soltysh
Copy link
Contributor Author

soltysh commented Nov 23, 2017

/test
[test]

@soltysh
Copy link
Contributor Author

soltysh commented Nov 27, 2017

@bparees all green, ptal

@jim-minter
Copy link
Contributor

with the caveat that I don't understand the ParseNormalizedNamed change, all this looks good to me. On this branch, hack/verify-godeps.sh indicates:

warning: differing godep github.com/opencontainers/runc/libcontainer/user: origin "4d6e6720a7c885c37b4cb083c0d372dda3425120" vs s2i "d223e2adae83f62d58448a799a5da05730228089"
warning: differing godep github.com/docker/go-units: origin "9e638d38cf6977a37a8ea0078f3ee75a7cdb2dd1" vs s2i "e30f1e79f3cd72542f2026ceec18d3bd67ab859c"
warning: differing godep github.com/Microsoft/go-winio: origin "78439966b38d69bf38227fbf57ac8a6fee70f69a" vs s2i "f533f7a102197536779ea3a8cb881d639e21ec5a"

I can repurpose #836 to deal with that if appropriate?

@soltysh
Copy link
Contributor Author

soltysh commented Nov 28, 2017

with the caveat that I don't understand the ParseNormalizedNamed change, all this looks good to me.

Docker changed their API, significantly. This works 😉

On this branch, hack/verify-godeps.sh indicates:

Uhm... I thought I've picked the exact same level of deps, I'll fix that right away.

@openshift-bot
Copy link
Contributor

Evaluated for source to image test up to de8ef59

@openshift-bot
Copy link
Contributor

Source To Image Test Results: SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_s2i/664/) (Base Commit: 60a42d7) (PR Branch Commit: de8ef59)

@soltysh
Copy link
Contributor Author

soltysh commented Nov 28, 2017

All fixed and green, with 2 lgtm, I'm merging.

[merge]

@openshift-bot
Copy link
Contributor

openshift-bot commented Nov 28, 2017 •
edited
Loading

Source To Image Merge Results: Waiting: You are in the build queue at position: 1

@openshift-bot
Copy link
Contributor

Evaluated for source to image merge up to de8ef59

@openshift-bot openshift-bot merged commit e3e0eed into openshift:master Nov 28, 2017
@soltysh soltysh deleted the drop_engine-api branch November 28, 2017 13:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@bparees bparees

Labels
size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@soltysh @bparees @jim-minter @openshift-bot @openshift-ci-robot