Merge pull request #670 from dnephin/cleanup-compose-volume-name

Only warn on volume.external.name for version 3.4

cli/compose/convert/volume.go

@@ -68,16 +68,15 @@ func convertVolumeToMount(
 		result.VolumeOptions.NoCopy = volume.Volume.NoCopy
 	}
 
-	// External named volumes
-	if stackVolume.External.External {
-		result.Source = stackVolume.External.Name
-		return result, nil
-	}
-
 	if stackVolume.Name != "" {
 		result.Source = stackVolume.Name
 	}
 
+	// External named volumes
+	if stackVolume.External.External {
+		return result, nil
+	}
+
 	result.VolumeOptions.Labels = AddStackLabel(namespace, stackVolume.Labels)
 	if stackVolume.Driver != "" || stackVolume.DriverOpts != nil {
 		result.VolumeOptions.DriverConfig = &mount.Driver{

cli/compose/convert/volume_test.go

@@ -148,20 +148,16 @@ func TestConvertVolumeToMountNamedVolumeWithNameCustomizd(t *testing.T) {
 func TestConvertVolumeToMountNamedVolumeExternal(t *testing.T) {
 	stackVolumes := volumes{
 		"outside": composetypes.VolumeConfig{
-			External: composetypes.External{
+			Name:     "special",
-				External: true,
+			External: composetypes.External{External: true},
-				Name:     "special",
-			},
 		},
 	}
 	namespace := NewNamespace("foo")
 	expected := mount.Mount{
-		Type:   mount.TypeVolume,
+		Type:          mount.TypeVolume,
-		Source: "special",
+		Source:        "special",
-		Target: "/foo",
+		Target:        "/foo",
-		VolumeOptions: &mount.VolumeOptions{
+		VolumeOptions: &mount.VolumeOptions{NoCopy: false},
-			NoCopy: false,
-		},
 	}
 	config := composetypes.ServiceVolumeConfig{
 		Type:   "volume",
@@ -176,10 +172,8 @@ func TestConvertVolumeToMountNamedVolumeExternal(t *testing.T) {
 func TestConvertVolumeToMountNamedVolumeExternalNoCopy(t *testing.T) {
 	stackVolumes := volumes{
 		"outside": composetypes.VolumeConfig{
-			External: composetypes.External{
+			Name:     "special",
-				External: true,
+			External: composetypes.External{External: true},
-				Name:     "special",
-			},
 		},
 	}
 	namespace := NewNamespace("foo")

cli/compose/loader/loader.go

@@ -12,6 +12,7 @@ import (
 	"github.com/docker/cli/cli/compose/template"
 	"github.com/docker/cli/cli/compose/types"
 	"github.com/docker/cli/opts"
+	"github.com/docker/docker/api/types/versions"
 	"github.com/docker/go-connections/nat"
 	units "github.com/docker/go-units"
 	shellwords "github.com/mattn/go-shellwords"
@@ -49,6 +50,7 @@ func Load(configDetails types.ConfigDetails) (*types.Config, error) {
 	}
 
 	configDict := getConfigDict(configDetails)
+	configDetails.Version = schema.Version(configDict)
 
 	if err := validateForbidden(configDict); err != nil {
 		return nil, err
@@ -60,7 +62,7 @@ func Load(configDetails types.ConfigDetails) (*types.Config, error) {
 		return nil, err
 	}
 
-	if err := schema.Validate(configDict, schema.Version(configDict)); err != nil {
+	if err := schema.Validate(configDict, configDetails.Version); err != nil {
 		return nil, err
 	}
 	return loadSections(configDict, configDetails)
@@ -103,7 +105,7 @@ func loadSections(config map[string]interface{}, configDetails types.ConfigDetai
 		{
 			key: "volumes",
 			fnc: func(config map[string]interface{}) error {
-				cfg.Volumes, err = LoadVolumes(config)
+				cfg.Volumes, err = LoadVolumes(config, configDetails.Version)
 				return err
 			},
 		},
@@ -446,34 +448,36 @@ func externalVolumeError(volume, key string) error {
 
 // LoadVolumes produces a VolumeConfig map from a compose file Dict
 // the source Dict is not validated if directly used. Use Load() to enable validation
-func LoadVolumes(source map[string]interface{}) (map[string]types.VolumeConfig, error) {
+func LoadVolumes(source map[string]interface{}, version string) (map[string]types.VolumeConfig, error) {
 	volumes := make(map[string]types.VolumeConfig)
-	err := transform(source, &volumes)
+	if err := transform(source, &volumes); err != nil {
-	if err != nil {
 		return volumes, err
 	}
-	for name, volume := range volumes {
-		if volume.External.External {
-			if volume.Driver != "" {
-				return nil, externalVolumeError(name, "driver")
-			}
-			if len(volume.DriverOpts) > 0 {
-				return nil, externalVolumeError(name, "driver_opts")
-			}
-			if len(volume.Labels) > 0 {
-				return nil, externalVolumeError(name, "labels")
-			}
-			if volume.External.Name == "" {
-				volume.External.Name = name
-				volumes[name] = volume
-			} else {
-				logrus.Warnf("volume %s: volume.external.name is deprecated in favor of volume.name", name)
 
-				if volume.Name != "" {
+	for name, volume := range volumes {
-					return nil, errors.Errorf("volume %s: volume.external.name and volume.name conflict; only use volume.name", name)
+		if !volume.External.External {
-				}
+			continue
-			}
 		}
+		switch {
+		case volume.Driver != "":
+			return nil, externalVolumeError(name, "driver")
+		case len(volume.DriverOpts) > 0:
+			return nil, externalVolumeError(name, "driver_opts")
+		case len(volume.Labels) > 0:
+			return nil, externalVolumeError(name, "labels")
+		case volume.External.Name != "":
+			if volume.Name != "" {
+				return nil, errors.Errorf("volume %s: volume.external.name and volume.name conflict; only use volume.name", name)
+			}
+			if versions.GreaterThanOrEqualTo(version, "3.4") {
+				logrus.Warnf("volume %s: volume.external.name is deprecated in favor of volume.name", name)
+			}
+			volume.Name = volume.External.Name
+			volume.External.Name = ""
+		case volume.Name == "":
+			volume.Name = name
+		}
+		volumes[name] = volume
 	}
 	return volumes, nil
 }

cli/compose/loader/loader_test.go

@@ -1,6 +1,7 @@
 package loader
 
 import (
+	"bytes"
 	"fmt"
 	"io/ioutil"
 	"os"
@@ -9,6 +10,7 @@ import (
 	"time"
 
 	"github.com/docker/cli/cli/compose/types"
+	"github.com/sirupsen/logrus"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 )
@@ -630,7 +632,7 @@ networks:
 			"super": {External: types.External{External: true, Name: "super"}},
 		},
 		Volumes: map[string]types.VolumeConfig{
-			"data": {External: types.External{External: true, Name: "data"}},
+			"data": {External: types.External{External: true}, Name: "data"},
 		},
 		Networks: map[string]types.NetworkConfig{
 			"front": {
@@ -1190,23 +1192,16 @@ func TestFullExample(t *testing.T) {
 			},
 		},
 		"external-volume": {
-			External: types.External{
+			Name:     "external-volume",
-				Name:     "external-volume",
+			External: types.External{External: true},
-				External: true,
-			},
 		},
 		"other-external-volume": {
-			External: types.External{
+			Name:     "my-cool-volume",
-				Name:     "my-cool-volume",
+			External: types.External{External: true},
-				External: true,
-			},
 		},
 		"external-volume3": {
-			Name: "this-is-volume3",
+			Name:     "this-is-volume3",
-			External: types.External{
+			External: types.External{External: true},
-				Name:     "external-volume3",
-				External: true,
-			},
 		},
 	}
 
@@ -1406,3 +1401,58 @@ services:
 	require.Len(t, config.Services, 1)
 	assert.Equal(t, expected, config.Services[0].ExtraHosts)
 }
+
+func TestLoadVolumesWarnOnDeprecatedExternalNameVersion34(t *testing.T) {
+	buf, cleanup := patchLogrus()
+	defer cleanup()
+
+	source := map[string]interface{}{
+		"foo": map[string]interface{}{
+			"external": map[string]interface{}{
+				"name": "oops",
+			},
+		},
+	}
+	volumes, err := LoadVolumes(source, "3.4")
+	require.NoError(t, err)
+	expected := map[string]types.VolumeConfig{
+		"foo": {
+			Name:     "oops",
+			External: types.External{External: true},
+		},
+	}
+	assert.Equal(t, expected, volumes)
+	assert.Contains(t, buf.String(), "volume.external.name is deprecated")
+
+}
+
+func patchLogrus() (*bytes.Buffer, func()) {
+	buf := new(bytes.Buffer)
+	out := logrus.StandardLogger().Out
+	logrus.SetOutput(buf)
+	return buf, func() { logrus.SetOutput(out) }
+}
+
+func TestLoadVolumesWarnOnDeprecatedExternalNameVersion33(t *testing.T) {
+	buf, cleanup := patchLogrus()
+	defer cleanup()
+
+	source := map[string]interface{}{
+		"foo": map[string]interface{}{
+			"external": map[string]interface{}{
+				"name": "oops",
+			},
+		},
+	}
+	volumes, err := LoadVolumes(source, "3.3")
+	require.NoError(t, err)
+	expected := map[string]types.VolumeConfig{
+		"foo": {
+			Name:     "oops",
+			External: types.External{External: true},
+		},
+	}
+	assert.Equal(t, expected, volumes)
+	assert.Equal(t, "", buf.String())
+
+}

cli/compose/types/types.go

@@ -55,6 +55,7 @@ type ConfigFile struct {
 
 // ConfigDetails are the details about a group of ConfigFiles
 type ConfigDetails struct {
+	Version     string
 	WorkingDir  string
 	ConfigFiles []ConfigFile
 	Environment map[string]string