Add support to volume v2 🐡 #339
Conversation
| c.Volumes[k] = v | ||
| if isVolume(v) { | ||
| volumes[v] = struct{}{} | ||
| // FIXME(vdemeester) why do we do that ? |
There was a problem hiding this comment.
I should remove that, this is handled here 👼
| @@ -75,3 +75,23 @@ func (s *CliSuite) TestNamedVolume(c *C) { | |||
| c.Assert(cn.Mounts[0].Name, DeepEquals, "vol") | |||
| c.Assert(cn.Mounts[0].Destination, DeepEquals, "/path") | |||
| } | |||
|
|
|||
| func (s *CliSuite) TestV2Volume(c *C) { | |||
There was a problem hiding this comment.
I shoud complete this one to make sure Bind and/or Mounts uses project_test as the name for the volume 👼
There was a problem hiding this comment.
I think an integration test for external volumes would be pretty useful as well.
| for _, serviceName := range p.ServiceConfigs.Keys() { | ||
| serviceConfig, _ := p.ServiceConfigs.Get(serviceName) | ||
| // Consolidate the name of the volume | ||
| // FIXME(vdemeester) probably shouldn't be there, maybe move that to interface/factory |
There was a problem hiding this comment.
It would definitely be useful if this logic could be customized 😄
There was a problem hiding this comment.
yes, same comment here and for network, this logic will have to be extracted at some point to be easier to customize 😝
vdemeester
force-pushed
the
v2-volumes
branch
2 times, most recently
from
2ace75c to
dae0f46
Compare
| c.Volumes[k] = v | ||
| if isVolume(v) { | ||
| volumes[v] = struct{}{} | ||
| func volumes(c *config.ServiceConfig, ctx project.Context) []string { |
There was a problem hiding this comment.
This function do not update serviceConfig anymore 👼
|
Unit tests / CI should be fixed 👼 |
| @@ -337,6 +383,16 @@ func (p *Project) Down(ctx context.Context, opts options.Down, services ...strin | |||
| return err | |||
| } | |||
|
|
|||
| if opts.RemoveVolume { | |||
There was a problem hiding this comment.
Should this be an option for Delete as well? I think the rm command also has a -v flag.
There was a problem hiding this comment.
hum yes, rm has the -v flag 😛
There was a problem hiding this comment.
Ah, it's already handled for rm (but I'll have to check on docker-compose to see if it's removing the named volume too)
There was a problem hiding this comment.
Ok, checked, rm -v does not remove declared/named volumes, so it's working the same :)
vdemeester
force-pushed
the
v2-volumes
branch
2 times, most recently
from
938eeb6 to
f840802
Compare
vdemeester
force-pushed
the
v2-volumes
branch
2 times, most recently
from
f0d5359 to
16b5512
Compare
- Introduce new yaml.Volume{,s} struct
- Introduce new `docker/volume` package (like `network/volume`)
Signed-off-by: Vincent Demeester <[email protected]>
|
As it's green, let's merge this and push the 0.3.0 milestone. Will work on coverage and refactoring in follow-ups. |
This PR brings the first and main step to add support for volumes for v2 compose files 🐹. Thanks to @joshwget and @shouze 🐳
yaml.Volume{,s}structdocker/volumepackage (likenetwork/volume)This closes #277
/cc @joshwget
🐸
Signed-off-by: Vincent Demeester [email protected]