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

Additional validation for container volumes and ports. #442

Merged
merged 3 commits into from
Sep 4, 2014

Conversation

dnephin
Copy link

@dnephin dnephin commented Aug 24, 2014

Note: Mapping local volumes is currently unsupported on boot2docker. We recommend you use [docker-osx](https://github.com/noplay/docker-osx) if want to map local volumes.
Note for fig on OSX: Mapping local volumes is currently unsupported on
boot2docker. We recommend you use [docker-osx](https://github.com/noplay/docker-osx)
if want to map local volumes on OSX.
Copy link
Author

Choose a reason for hiding this comment

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

I've seen multiple people confused about this so I tried to clarify that this is only an issue on OSX, not linux.

Choose a reason for hiding this comment

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

+1

if len(parts) == 2:
return reversed(parts)

return parts[2], (parts[0], parts[1] or None)

Copy link

Choose a reason for hiding this comment

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

This list-fiddling stuff is really confusing. Could we bring back the local variables, so I can see what's being returned in each case?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, I'm happy to make some changes here. I think in order to validate the string properly, we still need to make it a list, but additional local variables I can add.

@aanand
Copy link

aanand commented Aug 26, 2014

Sorry I missed this. Looks good.

Could it be split out into several logical commits? A lot of things are conflated here.

@dnephin
Copy link
Author

dnephin commented Aug 26, 2014

Sure, I'm definitely happy to split this into a few commits.

@dnephin dnephin force-pushed the fix_create_container_volumes branch from 26a6e18 to a813717 Compare August 26, 2014 02:18
Signed-off-by: Daniel Nephin <dnephin@gmail.com>
@dnephin dnephin force-pushed the fix_create_container_volumes branch from a813717 to 9abdf34 Compare August 26, 2014 02:20
@dnephin
Copy link
Author

dnephin commented Aug 26, 2014

Rebased into three commits, and added additional local variables for split_port()

@d11wtq
Copy link

d11wtq commented Aug 27, 2014

Awesome. Good to see this. Thanks for all your contributions, @dnephin! Massively appreciated.

Adds ~ support and ro mode support for volumes, along with some additional validation.

Signed-off-by: Daniel Nephin <dnephin@gmail.com>
…st cases.

Signed-off-by: Daniel Nephin <dnephin@gmail.com>
@dnephin dnephin force-pushed the fix_create_container_volumes branch from be67aa7 to 24044fa Compare August 31, 2014 06:03
@dnephin
Copy link
Author

dnephin commented Aug 31, 2014

additional validation for mode (should be in ro, rw).

@bfirsh
Copy link

bfirsh commented Sep 4, 2014

LGTM

1 similar comment
@aanand
Copy link

aanand commented Sep 4, 2014

LGTM

aanand added a commit that referenced this pull request Sep 4, 2014
Additional validation for container volumes and ports.
@aanand aanand merged commit 7ad91f3 into docker:master Sep 4, 2014
@dnephin dnephin deleted the fix_create_container_volumes branch September 5, 2014 01:59
yuval-k pushed a commit to yuval-k/compose that referenced this pull request Apr 10, 2015
Additional validation for container volumes and ports.
Signed-off-by: Yuval Kohavi <yuval.kohavi@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants