volume support enhancements #65
Conversation
- allow container configuration parameters to be specified for - hostname - domainname - user - memory - memorySwap - entrypoint - volumes - workingDir - binds - privileged - dns - dnsSearch - capAdd - capDrop - restartPolicy Signed-off-by: Jae Gangemi <jgangemi@gmail.com>
…into 55-export-volumes
- merged ContainerConfig/ContainerHostConfig into Container
Signed-off-by: Jae Gangemi <jgangemi@gmail.com>
- fixed some compiler warnings Signed-off-by: Jae Gangemi <jgangemi@gmail.com>
Signed-off-by: Jae Gangemi <jgangemi@gmail.com>
|
Awesome ! I will do a review, hopefully today, but you can expect the PR applied tomorrow at last. |
| for (String volume : bind) { | ||
| if (volume.contains(":")) { | ||
| binds.put(volume); | ||
| volume = volume.split(":")[0]; |
There was a problem hiding this comment.
Are you sure that the volume to be specified within "Volumes" in the create configuration should be the host path ? I'm not sure, but I think it should be the container path (element [1]). The documentation is not really clear about this. The best I could find was moby/moby#1580.
There was a problem hiding this comment.
yeah - that's a bug, good catch!
There was a problem hiding this comment.
Ok, I will fix this since I already made some (more or less cosmetic) changes.
|
IMO using Yes, Maven is really hard to use for complex configuration needs. In fact I really would like to stick the bind and import stuff together, e.g. like in <volumes>
<bind>/tmp</bind>
<bind>/bla:/blub</bind>
<from>data</from>
</volumes>but as you said correctly this is not possible with Maven (e.g. every list needs a dedicated parent element). So I came up with this <volumes>
<bind>
<volume>/tmp</volume>
<volume>/bla:/blub</volume>
</bind>
<from>
<image>data</image>
</from>
</volumes>Ok, a bit verbose, but hey, its XML anyway ;-) What do you think ? I'm just about to implement that one, if you don't mind. |
|
yeah - i always wondered why filesets had to be distinct includes/excludes... i'm fine w/ that structure change. |
E.g. inserted dedicated ContainerCreateConfig and ContainerStartConfig object for better separation of concerns.
|
Pushed my refactoring (including fix and changes) to branch I would merge it immediately to master, however then the documentation won't reflect the currently deployed plugins. Still need a better workflow here .... Thanks again, Jae ... |
|
my pleasure, thanks for letting me contribute! |
|
What's left on this PR? Would really like to see it in master soon 😄 |
|
You can find the PR merged into branch Still have to overthink my workflow for the documentation, though. |
|
why don't you just have a 'next release' branch and then when you're ready to do a release, merge that into master? |
|
That's a good idea. Currently I only have that single branch, but you are right, one should have probably something like an integration branch. |
|
you could probably just rename |
|
I just created a branch Now all work the next version will be done on branches starting on Hope that's not to confusing. So if you could rebase on |
|
Going to close thie PR, since everything has been already merged to integration so far. Thanks ! |
this is meant to replace #60. in addition to resolving issue #55, it also addresses #62 to allow container volumes to be specified at image build time. (i split off the assembly enhancements into #64).
all options are supported in the property files, there are some new tests, and some compiler warning cleanup.
i have also update the docs and did some minor re-organization which i hope you won't mind (i think having
docker:buildcome beforedocker:startmakes sense from a workflow perspective and i think listing config values in alphabetical order makes scanning the docs for what you need easier).