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.

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

Fix various volume bugs #1439

Merged
merged 2 commits into from
Feb 8, 2017
Merged

Conversation

bfirsh
Copy link
Contributor

@bfirsh bfirsh commented Feb 7, 2017

Fixes both issues mentioned in #1380.

Technically we shouldn't be passing volumes without a host path as binds, but the daemon doesn't seem to mind.

Seems like this is pretty much ignored by Docker, so it wasn't
causing any visible issues, except when a volume name was used
instead of a path.

Also, added integration tests.

Ref docker#1380

Signed-off-by: Ben Firshman <ben@firshman.co.uk>
Technically we shouldn't be passing them as binds, but the daemon
doesn't seem to mind.

Fixes docker#1380

Signed-off-by: Ben Firshman <ben@firshman.co.uk>
Copy link
Contributor

@shin- shin- left a comment

Choose a reason for hiding this comment

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

I really think we should support a mapping type here because of how limited and difficult to parse (when accounting for Windows clients and/or containers) the x:y:z syntax is.

I skipped the part where that was already possible, apparently. Sorry

@@ -392,7 +393,8 @@ def create_container(self, image, command=None, hostname=None, user=None,
version 1.10. Use ``host_config`` instead.
dns_opt (:py:class:`list`): Additional options to be added to the
container's ``resolv.conf`` file
volumes (str or list):
volumes (str or list): List of paths inside the container to use
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think str is a valid type here, is it?

Copy link
Contributor

@shin- shin- Feb 8, 2017

Choose a reason for hiding this comment

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

ah, nevermind, we just add it to a one-element list if it's a string. Oops!



def _host_volume_from_bind(bind):
bits = bind.split(':')
Copy link
Contributor

Choose a reason for hiding this comment

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

This is broken in the case of Windows host paths

Copy link
Contributor

@shin- shin- left a comment

Choose a reason for hiding this comment

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

Aside from the issue with Windows paths, which I don't think we have done right in docker-py anywhere, and since there's an alternative structure that doesn't rely on the x:y:z format, this LGTM!

@shin- shin- added this to the 2.1.0 milestone Feb 8, 2017
@shin- shin- merged commit 6114024 into docker:master Feb 8, 2017
@bfirsh
Copy link
Contributor Author

bfirsh commented Feb 8, 2017

In retrospect I think we should have dropped support for the string format in 2.0. ;)

Perhaps we can phase it out over a few releases...

@bfirsh bfirsh deleted the fix-various-volume-bugs branch February 8, 2017 08:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants