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

Promote volume drivers from experimental to master. #14659

Merged

Conversation

@calavera
Copy link
Contributor

@calavera calavera commented Jul 15, 2015

Remove volume stubs and use the experimental path as the only path.

Part of #14214.

/cc @cpuguy83, @icecrime

Signed-off-by: David Calavera david.calavera@gmail.com

@cpuguy83
Copy link
Collaborator

@cpuguy83 cpuguy83 commented Jul 15, 2015

LGTM pending janky.

--- EDIT ---
Actually the only thing we may want to discuss is... do we need to change any of the plugin endpoints (like maybe passing in container ID to mount/unmount) before doing this.

@calavera calavera added this to the 1.8.0 milestone Jul 15, 2015
@calavera
Copy link
Contributor Author

@calavera calavera commented Jul 15, 2015

@cpuguy83 I think we can do it after. It will be easier for everyone if we land this first, so we don't have to think about experimental/non-experimental.

@calavera calavera force-pushed the calavera:promote_volumes_experimental_to_master branch 4 times, most recently from fec54ae to 98e2d0d Jul 15, 2015
@icecrime
Copy link
Contributor

@icecrime icecrime commented Jul 15, 2015

Two questions:

  • I'm assuming we chose not to make #14242 a prerequisite, but are we still considering it for the near future?
  • Why are we mentioned passing the container ID down to the plugin? I don't see what additional use it would make of it as long as we guarantee a unique (user-provided) name for the volume.
@calavera
Copy link
Contributor Author

@calavera calavera commented Jul 15, 2015

I'm assuming we chose not to make #14242 a prerequisite, but are we still considering it for the near future?

@icecrime at the end, moving this out of experimental is a pre-requisite of #14242. Otherwise @cpuguy83 needs to put the api/cli in experimental and then move them out.

Why are we mentioned passing the container ID down to the plugin? I don't see what additional use it would make of it as long as we guarantee a unique (user-provided) name for the volume.

It's a different topic, we've talked about sending some extra information to the plugins, like the container id. I don't think it's related to this PR.

@icecrime
Copy link
Contributor

@icecrime icecrime commented Jul 15, 2015

Sounds good, thanks.

@icecrime
Copy link
Contributor

@icecrime icecrime commented Jul 15, 2015

Code LGTM.

Ping @lukemarsden FYI.
Spoiler alert: we're gonna need to promote the docs too.

@calavera
Copy link
Contributor Author

@calavera calavera commented Jul 15, 2015

Spoiler alert: we're gonna need to promote the docs too.

moving the docs too.

@calavera
Copy link
Contributor Author

@calavera calavera commented Jul 15, 2015

ouch, I might wait to move the docs until #13951 is merged, which should happen soon-ish.

@calavera calavera force-pushed the calavera:promote_volumes_experimental_to_master branch 2 times, most recently from a77c978 to 451f69a Jul 20, 2015
@calavera
Copy link
Contributor Author

@calavera calavera commented Jul 20, 2015

@thaJeztah I moved the volume and plugin documents from experimental to docs in this PR. Let me know what you think.

@icecrime
Copy link
Contributor

@icecrime icecrime commented Jul 20, 2015

One issue with docs here is that I believe they aren't linked to anywhere? (cc @moxiegirl)

@calavera calavera force-pushed the calavera:promote_volumes_experimental_to_master branch from 451f69a to 5cf076d Jul 20, 2015
@calavera
Copy link
Contributor Author

@calavera calavera commented Jul 20, 2015

yeah, I'm wondering if we should have a Extend docker section:

image

@thaJeztah
Copy link
Member

@thaJeztah thaJeztah commented Jul 20, 2015

@calavera IIUC, only the volume plugins will be promoted from experimental, but the network plugins still are (experimental)? In that case, shouldn't the docs in the experimental directory stay intact for the network plugins?

And, yes, we need to find a good location inside the navigation :-)

@thaJeztah
Copy link
Member

@thaJeztah thaJeztah commented Jul 20, 2015

I personally like "extend docker" as a title

@calavera
Copy link
Contributor Author

@calavera calavera commented Jul 20, 2015

@thaJeztah the networking docs are still in the experimental folder.

@thaJeztah
Copy link
Member

@thaJeztah thaJeztah commented Jul 20, 2015

@calavera yes, but the plugins.md file contained the description for both the network and volume plugins, not sure if that's still needed in the "experimental" directory?

@calavera
Copy link
Contributor Author

@calavera calavera commented Jul 20, 2015

@thaJeztah not anymore. I changed the plugins document to reflect what we have that's not experimental.

@moxiegirl
Copy link
Contributor

@moxiegirl moxiegirl commented Jul 20, 2015

@calavera Since Extend Docker seems like a good title to me too. The position is important --- I think it should come after Manage Image Repositories at the very least. Since it contains API information it really belongs under Command and API References --- WDYT?

@calavera
Copy link
Contributor Author

@calavera calavera commented Jul 20, 2015

sounds great, I'll make the change.

@calavera calavera force-pushed the calavera:promote_volumes_experimental_to_master branch from 5cf076d to 03849e6 Jul 20, 2015
@calavera
Copy link
Contributor Author

@calavera calavera commented Jul 20, 2015

I moved the docs to docs/plugins and they should show up under the Extend Docker menu when docker/docs-base#114 is merged.

@moxiegirl
Copy link
Contributor

@moxiegirl moxiegirl commented Jul 20, 2015

um. don't kill me but I renamed plugins to extend to fit the menu. After all, plugin isn't the only way a product could be extended.

@moxiegirl
Copy link
Contributor

@moxiegirl moxiegirl commented Jul 21, 2015

image

@SvenDowideit
Copy link
Contributor

@SvenDowideit SvenDowideit commented Jul 21, 2015

I'm pretty much LGTM on the docs, and while I'd love to have ID and userid passed to the plugin, I can do quite a bit without :)

Remove volume stubs and use the experimental path as the only path.

Signed-off-by: David Calavera <david.calavera@gmail.com>
@calavera calavera force-pushed the calavera:promote_volumes_experimental_to_master branch from 03849e6 to c4d45b6 Jul 21, 2015
@calavera
Copy link
Contributor Author

@calavera calavera commented Jul 21, 2015

@moxiegirl I've applied your patch and moved the docs to extend, thank you!
@SvenDowideit I'm confident we can iterate on that in future releases.

@moxiegirl
Copy link
Contributor

@moxiegirl moxiegirl commented Jul 21, 2015

Thanks @calavera LGTM

calavera added a commit that referenced this pull request Jul 21, 2015
…to_master

Promote volume drivers from experimental to master.
@calavera calavera merged commit 3ee15ac into moby:master Jul 21, 2015
4 checks passed
4 checks passed
@GordonTheTurtle
docker/dco-signed All commits signed
Details
@GordonTheTurtle
experimental Jenkins build Docker-PRs-experimental 3260 has succeeded
Details
@GordonTheTurtle
janky Jenkins build Docker-PRs 11816 has succeeded
Details
@GordonTheTurtle
windows Jenkins build Windows-PRs 8435 has succeeded
Details
@thaJeztah
Copy link
Member

@thaJeztah thaJeztah commented Jul 21, 2015

docs LGTM, thanks @calavera

Just to confirm; @cpuguy83 your LGTM is for design and code review?

if it is: press da button!

@icecrime icecrime mentioned this pull request Jul 23, 2015
0 of 5 tasks complete
@calavera calavera deleted the calavera:promote_volumes_experimental_to_master branch Jul 23, 2015
@lukemarsden
Copy link
Contributor

@lukemarsden lukemarsden commented Jul 28, 2015

@icecrime thanks for the heads up!

I have tested flocker and the flocker docker plugin with docker master from today (4ed3e3a) and, after changing the plugins path, it appears to be working in basic manual testing.

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

8 participants