Repository: compose-spec/compose-go
Pull request #480 (2023-11-03 09:55:30)

make External a boolean
For legacy reason (i.e Docker Swarm) `external` at some point has been defined as a struct, then deprecated to get back to a plain boolean using parent `name` to retrieve an existing resource.
As we manage transformation in plain yaml we can now manage this legacy without having the go structs to reflect this weird heritage.
This PR changes `types.External` definition to be a plain boolean, and handle name relocation during the "transform" phase.
Also removes the weird assumption an `external` resource will have always `name` set, even this is just the same as the one defined by the mapping keys


Patch:
diff --git a/loader/normalize.go b/loader/normalize.go
index 2d5adaa..b2c49d1 100644
--- a/loader/normalize.go
+++ b/loader/normalize.go
@@ -37,10 +37,6 @@ func Normalize(project *types.Project) error {
 		project.Networks["default"] = types.NetworkConfig{}
 	}
 
-	if err := relocateExternalName(project); err != nil {
-		return err
-	}
-
 	for i, s := range project.Services {
 		if len(s.Networks) == 0 && s.NetworkMode == "" {
 			// Service without explicit network attachment are implicitly exposed on default network
@@ -224,49 +220,6 @@ func setNameFromKey(project *types.Project) {
 	}
 }
 
-func relocateExternalName(project *types.Project) error {
-	for i, n := range project.Networks {
-		if n.External.Name != "" {
-			if n.Name != "" {
-				return errors.Wrap(errdefs.ErrInvalid, "can't use both 'networks.external.name' (deprecated) and 'networks.name'")
-			}
-			n.Name = n.External.Name
-		}
-		project.Networks[i] = n
-	}
-
-	for i, v := range project.Volumes {
-		if v.External.Name != "" {
-			if v.Name != "" {
-				return errors.Wrap(errdefs.ErrInvalid, "can't use both 'volumes.external.name' (deprecated) and 'volumes.name'")
-			}
-			v.Name = v.External.Name
-		}
-		project.Volumes[i] = v
-	}
-
-	for i, s := range project.Secrets {
-		if s.External.Name != "" {
-			if s.Name != "" {
-				return errors.Wrap(errdefs.ErrInvalid, "can't use both 'secrets.external.name' (deprecated) and 'secrets.name'")
-			}
-			s.Name = s.External.Name
-		}
-		project.Secrets[i] = s
-	}
-
-	for i, c := range project.Configs {
-		if c.External.Name != "" {
-			if c.Name != "" {
-				return errors.Wrap(errdefs.ErrInvalid, "can't use both 'configs.external.name' (deprecated) and 'configs.name'")
-			}
-			c.Name = c.External.Name
-		}
-		project.Configs[i] = c
-	}
-	return nil
-}
-
 func relocateLogOpt(s *types.ServiceConfig) error {
 	if len(s.LogOpt) != 0 {
 		logrus.Warn("`log_opts` is deprecated. Use the `logging` element")
diff --git a/loader/validate.go b/loader/validate.go
index 582a3ea..dc37007 100644
--- a/loader/validate.go
+++ b/loader/validate.go
@@ -117,7 +117,7 @@ func checkConsistency(project *types.Project) error {
 	}
 
 	for name, secret := range project.Secrets {
-		if secret.External.External {
+		if secret.External {
 			continue
 		}
 		if secret.File == "" && secret.Environment == "" {
diff --git a/transform/canonical.go b/transform/canonical.go
index c80dd39..c873039 100644
--- a/transform/canonical.go
+++ b/transform/canonical.go
@@ -41,10 +41,6 @@ func init() {
 	transformers["networks.*"] = transformMaybeExternal
 	transformers["secrets.*"] = transformMaybeExternal
 	transformers["configs.*"] = transformMaybeExternal
-	transformers["volumes.*.external"] = transformExternal
-	transformers["networks.*.external"] = transformExternal
-	transformers["secrets.*.external"] = transformExternal
-	transformers["configs.*.external"] = transformExternal
 	transformers["include.*"] = transformInclude
 }
 
diff --git a/transform/external.go b/transform/external.go
index 7b57e7e..861a6ea 100644
--- a/transform/external.go
+++ b/transform/external.go
@@ -20,29 +20,9 @@ import (
 	"fmt"
 
 	"github.com/compose-spec/compose-go/v2/tree"
-	"github.com/pkg/errors"
 	"github.com/sirupsen/logrus"
 )
 
-func transformExternal(data any, p tree.Path) (any, error) {
-	switch v := data.(type) {
-	case map[string]any:
-		if _, ok := v["name"]; ok {
-			logrus.Warnf("%s: external.name is deprecated. Please set name and external: true", p.Parent())
-		}
-		if _, ok := v["external"]; !ok {
-			v["external"] = true
-		}
-		return v, nil
-	case bool:
-		return map[string]any{
-			"external": v,
-		}, nil
-	default:
-		return data, errors.Errorf("invalid type %T for external", v)
-	}
-}
-
 func transformMaybeExternal(data any, p tree.Path) (any, error) {
 	if data == nil {
 		return nil, nil
@@ -59,14 +39,15 @@ func transformMaybeExternal(data any, p tree.Path) (any, error) {
 		}
 		name := resource["name"]
 		if ename, ok := external["name"]; ok {
+			logrus.Warnf("%s: external.name is deprecated. Please set name and external: true", p)
 			if name != nil && ename != name {
 				return nil, fmt.Errorf("%s: name and external.name conflict; only use name", p)
 			}
-			delete(external, "name")
-			resource["name"] = ename
-		} else if name == nil {
-			resource["name"] = p.Last()
+			if name == nil {
+				resource["name"] = ename
+			}
 		}
+		resource["external"] = true
 	}
 
 	return resource, nil
diff --git a/types/types.go b/types/types.go
index e3f09d8..a66d0b4 100644
--- a/types/types.go
+++ b/types/types.go
@@ -656,28 +656,7 @@ type VolumeConfig struct {
 
 // External identifies a Volume or Network as a reference to a resource that is
 // not managed, and should already exist.
-// External.name is deprecated and replaced by Volume.name
-type External struct {
-	Name       string     `yaml:"name,omitempty" json:"name,omitempty"`
-	External   bool       `yaml:"external,omitempty" json:"external,omitempty"`
-	Extensions Extensions `yaml:"#extensions,inline" json:"-"`
-}
-
-// MarshalYAML makes External implement yaml.Marshaller
-func (e External) MarshalYAML() (interface{}, error) {
-	if e.Name == "" {
-		return e.External, nil
-	}
-	return External{Name: e.Name}, nil
-}
-
-// MarshalJSON makes External implement json.Marshaller
-func (e External) MarshalJSON() ([]byte, error) {
-	if e.Name == "" {
-		return []byte(fmt.Sprintf("%v", e.External)), nil
-	}
-	return []byte(fmt.Sprintf(`{"name": %q}`, e.Name)), nil
-}
+type External bool
 
 // CredentialSpecConfig for credential spec on Windows
 type CredentialSpecConfig struct {
diff --git a/validation/external.go b/validation/external.go
index 9a5047d..c9c1a3c 100644
--- a/validation/external.go
+++ b/validation/external.go
@@ -28,8 +28,7 @@ func checkExternal(v map[string]any, p tree.Path) error {
 	if !ok {
 		return nil
 	}
-	x := b.(map[string]any)
-	if b, ok := x["external"]; ok && b == false {
+	if !b.(bool) {
 		return nil
 	}
 


Test patch:
diff --git a/loader/full-struct_test.go b/loader/full-struct_test.go
index e699e71..131a01a 100644
--- a/loader/full-struct_test.go
+++ b/loader/full-struct_test.go
@@ -482,13 +482,12 @@ func networks() map[string]types.NetworkConfig {
 		},
 
 		"external-network": {
-			Name:     "external-network",
-			External: types.External{External: true},
+			External: true,
 		},
 
 		"other-external-network": {
 			Name:     "my-cool-network",
-			External: types.External{External: true},
+			External: true,
 			Extensions: map[string]interface{}{
 				"x-bar": "baz",
 				"x-foo": "bar",
@@ -519,16 +518,15 @@ func volumes() map[string]types.VolumeConfig {
 			},
 		},
 		"external-volume": {
-			Name:     "external-volume",
-			External: types.External{External: true},
+			External: true,
 		},
 		"other-external-volume": {
 			Name:     "my-cool-volume",
-			External: types.External{External: true},
+			External: true,
 		},
 		"external-volume3": {
 			Name:     "this-is-volume3",
-			External: types.External{External: true},
+			External: true,
 			Extensions: map[string]interface{}{
 				"x-bar": "baz",
 				"x-foo": "bar",
@@ -547,11 +545,10 @@ func configs(workingDir string, homeDir string) map[string]types.ConfigObjConfig
 		},
 		"config2": {
 			Name:     "my_config",
-			External: types.External{External: true},
+			External: true,
 		},
 		"config3": {
-			Name:     "config3",
-			External: types.External{External: true},
+			External: true,
 		},
 		"config4": {
 			Name: "foo",
@@ -574,11 +571,10 @@ func secrets(workingDir string) map[string]types.SecretConfig {
 		},
 		"secret2": {
 			Name:     "my_secret",
-			External: types.External{External: true},
+			External: true,
 		},
 		"secret3": {
-			Name:     "secret3",
-			External: types.External{External: true},
+			External: true,
 		},
 		"secret4": {
 			Name:        "bar",
@@ -948,7 +944,6 @@ services:
     x-foo: bar
 networks:
   external-network:
-    name: external-network
     external: true
   other-external-network:
     name: my-cool-network
@@ -983,7 +978,6 @@ volumes:
       baz: "1"
       foo: bar
   external-volume:
-    name: external-volume
     external: true
   external-volume3:
     name: this-is-volume3
@@ -1010,7 +1004,6 @@ secrets:
     name: my_secret
     external: true
   secret3:
-    name: secret3
     external: true
   secret4:
     name: bar
@@ -1028,7 +1021,6 @@ configs:
     name: my_config
     external: true
   config3:
-    name: config3
     external: true
   config4:
     name: foo
@@ -1060,7 +1052,6 @@ func fullExampleJSON(workingDir, homeDir string) string {
   "configs": {
     "config1": {
       "file": "%s",
-      "external": false,
       "labels": {
         "foo": "bar"
       }
@@ -1070,19 +1061,16 @@ func fullExampleJSON(workingDir, homeDir string) string {
       "external": true
     },
     "config3": {
-      "name": "config3",
       "external": true
     },
     "config4": {
       "name": "foo",
-      "file": "%s",
-      "external": false
+      "file": "%s"
     }
   },
   "name": "full_example_project_name",
   "networks": {
     "external-network": {
-      "name": "external-network",
       "ipam": {},
       "external": true
     },
@@ -1116,20 +1104,17 @@ func fullExampleJSON(workingDir, homeDir string) string {
           }
         ]
       },
-      "external": false,
       "labels": {
         "foo": "bar"
       }
     },
     "some-network": {
-      "ipam": {},
-      "external": false
+      "ipam": {}
     }
   },
   "secrets": {
     "secret1": {
       "file": "%s",
-      "external": false,
       "labels": {
         "foo": "bar"
       }
@@ -1139,17 +1124,14 @@ func fullExampleJSON(workingDir, homeDir string) string {
       "external": true
     },
     "secret3": {
-      "name": "secret3",
       "external": true
     },
     "secret4": {
       "name": "bar",
-      "environment": "BAR",
-      "external": false
+      "environment": "BAR"
     },
     "secret5": {
-      "file": "/abs/secret_data",
-      "external": false
+      "file": "/abs/secret_data"
     }
   },
   "services": {
@@ -1653,11 +1635,9 @@ func fullExampleJSON(workingDir, homeDir string) string {
       "driver_opts": {
         "baz": "1",
         "foo": "bar"
-      },
-      "external": false
+      }
     },
     "external-volume": {
-      "name": "external-volume",
       "external": true
     },
     "external-volume3": {
@@ -1674,14 +1654,11 @@ func fullExampleJSON(workingDir, homeDir string) string {
         "baz": "1",
         "foo": "bar"
       },
-      "external": false,
       "labels": {
         "foo": "bar"
       }
     },
-    "some-volume": {
-      "external": false
-    }
+    "some-volume": {}
   },
   "x-bar": "baz",
   "x-foo": "bar",
diff --git a/loader/loader_test.go b/loader/loader_test.go
index 1a382ed..0e0fea7 100644
--- a/loader/loader_test.go
+++ b/loader/loader_test.go
@@ -859,19 +859,18 @@ networks:
 			},
 		},
 		Configs: map[string]types.ConfigObjConfig{
-			"appconfig": {External: types.External{External: true}, Name: "appconfig"},
+			"appconfig": {External: true},
 		},
 		Secrets: map[string]types.SecretConfig{
-			"super": {External: types.External{External: true}, Name: "super"},
+			"super": {External: true},
 		},
 		Volumes: map[string]types.VolumeConfig{
-			"data": {External: types.External{External: true}, Name: "data"},
+			"data": {External: true},
 		},
 		Networks: map[string]types.NetworkConfig{
 			"back": {},
 			"front": {
-				External:   types.External{External: true},
-				Name:       "front",
+				External:   true,
 				Internal:   true,
 				Attachable: true,
 			},
@@ -1455,7 +1454,7 @@ volumes:
 	expected := types.Volumes{
 		"foo": {
 			Name:     "oops",
-			External: types.External{External: true},
+			External: true,
 		},
 	}
 	assert.Check(t, is.DeepEqual(expected, project.Volumes))
@@ -1514,7 +1513,7 @@ secrets:
 	expected := types.Secrets{
 		"foo": {
 			Name:     "oops",
-			External: types.External{External: true},
+			External: true,
 		},
 	}
 	assert.Check(t, is.DeepEqual(expected, project.Secrets))
@@ -1537,7 +1536,7 @@ networks:
 	expected := types.Networks{
 		"foo": {
 			Name:     "oops",
-			External: types.External{External: true},
+			External: true,
 		},
 	}
 	assert.Check(t, is.DeepEqual(expected, project.Networks))
@@ -1805,14 +1804,14 @@ secrets:
 		Configs: map[string]types.ConfigObjConfig{
 			"config": {
 				Name:           "config",
-				External:       types.External{External: true},
+				External:       true,
 				TemplateDriver: "config-driver",
 			},
 		},
 		Secrets: map[string]types.SecretConfig{
 			"secret": {
 				Name:           "secret",
-				External:       types.External{External: true},
+				External:       true,
 				TemplateDriver: "secret-driver",
 			},
 		},
@@ -1874,7 +1873,7 @@ secrets:
 		Configs: map[string]types.ConfigObjConfig{
 			"config": {
 				Name:     "config",
-				External: types.External{External: true},
+				External: true,
 			},
 		},
 		Secrets: map[string]types.SecretConfig{
diff --git a/loader/normalize_test.go b/loader/normalize_test.go
index 9ea4a37..4fcd7ca 100644
--- a/loader/normalize_test.go
+++ b/loader/normalize_test.go
@@ -35,7 +35,7 @@ func TestNormalizeNetworkNames(t *testing.T) {
 		Networks: types.Networks{
 			"myExternalnet": {
 				Name:     "myExternalnet", // this is automaticaly setup by loader for externa networks before reaching normalization
-				External: types.External{External: true},
+				External: true,
 			},
 			"mynet": {},
 			"myNamedNet": {
@@ -92,7 +92,7 @@ func TestNormalizeVolumes(t *testing.T) {
 		Volumes: types.Volumes{
 			"myExternalVol": {
 				Name:     "myExternalVol", // this is automaticaly setup by loader for externa networks before reaching normalization
-				External: types.External{External: true},
+				External: true,
 			},
 			"myvol": {},
 			"myNamedVol": {
@@ -107,7 +107,7 @@ func TestNormalizeVolumes(t *testing.T) {
 		Volumes: types.Volumes{
 			"myExternalVol": {
 				Name:     "myExternalVol",
-				External: types.External{External: true},
+				External: true,
 			},
 			"myvol": {Name: "myProject_myvol"},
 			"myNamedVol": {
diff --git a/loader/validate_test.go b/loader/validate_test.go
index 0c3e891..81e6f77 100644
--- a/loader/validate_test.go
+++ b/loader/validate_test.go
@@ -185,7 +185,7 @@ func TestValidateSecret(t *testing.T) {
 		project := &types.Project{
 			Secrets: types.Secrets{
 				"foo": types.SecretConfig{
-					External: types.External{External: true},
+					External: true,
 				},
 			},
 		}
@@ -206,9 +206,7 @@ func TestValidateSecret(t *testing.T) {
 		project := &types.Project{
 			Secrets: types.Secrets{
 				"foo": types.SecretConfig{
-					External: types.External{
-						External: true,
-					},
+					External: true,
 				},
 			},
 			Services: types.Services([]types.ServiceConfig{