From aea9544db5218505df621fadf2483678a163099c Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Mon, 6 Nov 2017 17:03:43 -0500 Subject: [PATCH] Only warn on volume.external.name for version 3.4 Also remove use of volume.external.name Signed-off-by: Daniel Nephin --- cli/compose/convert/volume.go | 11 ++--- cli/compose/convert/volume_test.go | 22 +++------ cli/compose/loader/loader.go | 54 +++++++++++---------- cli/compose/loader/loader_test.go | 78 ++++++++++++++++++++++++------ cli/compose/types/types.go | 1 + 5 files changed, 107 insertions(+), 59 deletions(-) diff --git a/cli/compose/convert/volume.go b/cli/compose/convert/volume.go index 8400f8d523..36dc54a13b 100644 --- a/cli/compose/convert/volume.go +++ b/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{ diff --git a/cli/compose/convert/volume_test.go b/cli/compose/convert/volume_test.go index 9e921701a0..3df88c1728 100644 --- a/cli/compose/convert/volume_test.go +++ b/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{ - External: true, - Name: "special", - }, + Name: "special", + External: composetypes.External{External: true}, }, } namespace := NewNamespace("foo") expected := mount.Mount{ - Type: mount.TypeVolume, - Source: "special", - Target: "/foo", - VolumeOptions: &mount.VolumeOptions{ - NoCopy: false, - }, + Type: mount.TypeVolume, + Source: "special", + Target: "/foo", + VolumeOptions: &mount.VolumeOptions{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{ - External: true, - Name: "special", - }, + Name: "special", + External: composetypes.External{External: true}, }, } namespace := NewNamespace("foo") diff --git a/cli/compose/loader/loader.go b/cli/compose/loader/loader.go index 01081a246f..a1a6ed0a12 100644 --- a/cli/compose/loader/loader.go +++ b/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 != nil { + if err := transform(source, &volumes); 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 != "" { - return nil, errors.Errorf("volume %s: volume.external.name and volume.name conflict; only use volume.name", name) - } - } + for name, volume := range volumes { + 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 } diff --git a/cli/compose/loader/loader_test.go b/cli/compose/loader/loader_test.go index 6fcc25534c..8ad65d2f9d 100644 --- a/cli/compose/loader/loader_test.go +++ b/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", - External: true, - }, + Name: "external-volume", + External: types.External{External: true}, }, "other-external-volume": { - External: types.External{ - Name: "my-cool-volume", - External: true, - }, + Name: "my-cool-volume", + External: types.External{External: true}, }, "external-volume3": { - Name: "this-is-volume3", - External: types.External{ - Name: "external-volume3", - External: true, - }, + Name: "this-is-volume3", + External: types.External{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()) + +} diff --git a/cli/compose/types/types.go b/cli/compose/types/types.go index e44b92c14e..b5afc9b241 100644 --- a/cli/compose/types/types.go +++ b/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