-
Notifications
You must be signed in to change notification settings - Fork 18.8k
Move VolumeDriver to HostConfig to make containers portable. #15798
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
Conversation
lint errors on missing comments for new exported types. |
533a2b2
to
1ba23c2
Compare
31dbc49
to
48b1cbd
Compare
Definitely a +1 from me since I created the issue :) |
Yup, +1 from me for changing this. Don't forget to update the docs and man-pages if this moves further. |
a1070ac
to
e493149
Compare
Should this require backwards compat or should this part of 1.8.2 and thus modify the current API? |
My thought is make it part of 1.8.x. |
I'm +1 to making it part of 1.8.x, regarding it a "fix" and to reduce the exposure (yes, it's awkward doing an API change in a dot-release, but we should try to avoid people starting to rely on this) |
@cpuguy83, @thaJeztah I'm neutral, but this change is completely backwards compatible. Containers started with |
@calavera Oh yes, makes sense. We have to support already created containers here. |
I still see |
@cpuguy83 version numbers are hard! I'm going to add some tests for the inspect output. |
e493149
to
e658ee6
Compare
I've added a couple of integration tests to prevent regressions. It turned out that I left the field in the config inspect for the next version of docker 😔 |
LGTM |
e658ee6
to
5655f9f
Compare
I've rebased this. Please, take a look. |
If I understand correctly, this design still only allows one VolumeDriver per container. What happens if you want to mount one volume using Flocker and one using Convoy? |
@bboreham Need to use |
@calavera external volume tests are failing. |
@bboreham using the new docker run -d \
-v $(docker volume create --driver=foo):/volume-a/ \
-v $(docker volume create --driver=bar):/volume-b/ \
my-image |
Ah, right, thanks. Wasn't clear from https://docs.docker.com/extend/plugins_volume/ |
@bboreham it's not released yet; the |
@calavera volume test are failing after rerunning the tests. |
Signed-off-by: David Calavera <david.calavera@gmail.com>
5655f9f
to
6549d65
Compare
@crosbymichael I don't know what happened with those tests, but it looks like it's passing now. |
LGTM |
1 similar comment
LGTM |
Move VolumeDriver to HostConfig to make containers portable.
Fixes #15342
/cc @cpuguy83
Signed-off-by: David Calavera david.calavera@gmail.com