-
Notifications
You must be signed in to change notification settings - Fork 61
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.
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
Add minimal support for docker-py-2.x module #663
Conversation
It looks fine to me, maybe its worth adding a new configuration to travis where docker-py 2 would be installed? |
Not sure it's worth adding it to Travis: we mock the docker module completely in unit tests. |
I'll try to test this asap, kind of under water with some other stuff though so it'll be a bit. |
@maxamillion, had any time for this? |
@twaugh I have not, but I'd be amazed if this caused any issues so please don't hold this up on my account. Apologies on the lag time as well as if this has been blocked by me for any reason. |
tests/docker_mock.py
Outdated
docker.errors.APIError, "xyz", response) | ||
else: | ||
flexmock(docker.Client).should_receive(method).and_raise(docker.errors.APIError, "xyz", | ||
flexmock(docker.APIClient).should_receive(method).and_raise(docker.errors.APIError, "xyz", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
E501 line too long (102 > 100 characters)
tests/test_tasker.py
Outdated
|
||
kwargs = {} | ||
if timeout is not None: | ||
kwargs['timeout'] = timeout | ||
|
||
DockerTasker(**kwargs) | ||
t = DockerTasker(**kwargs) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
F841 local variable 't' is assigned to but never used
Rebased, and updated to make the test suite pass when python-docker 2.x is installed. |
Looks good so far, I'll check if it works in latest rawhide on osbs box |
Crashes here - https://gist.github.com/vrutkovs/bdb44bb52b305c70d26516898d24a347 Tested on F26 osbs-box with Dockerfile: https://gist.github.com/vrutkovs/e6ddfba090f8214d1b31dedf4b3d44fa. UPD. Seems to come from docker-squash though :/ |
Okay, now got a valid crash:
Since Tim is on PTO next week I'll try to come up with a fix and push it to the branch UPD. This is docker-py weirdness - docker/compose#4344 (comment) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Worked flawlessly for me, with docker-squash from master
Signed-off-by: Tim Waugh <twaugh@redhat.com>
Draft release notes updated. |
FWIW, this is live in production in Fedora Infra now and it works great. |
Great! Thanks for letting us know. |
@maxamillion, something to test when using a rawhide- or f26-based buildroot.