Revert "run interpolation after merge, but for required attributes"

idsulik · ndeloof · commit bfaf10bba3cb · 2024-07-23T15:26:30.000+02:00
Signed-off-by: Suleiman Dibirov &lt;idsulik@gmail.com&gt;
diff --git a/cli/options_test.go b/cli/options_test.go
@@ -253,7 +253,7 @@ func TestProjectWithDotEnv(t *testing.T) {
 		"compose-with-variables.yaml",
 	}, WithName("my_project"), WithEnvFiles(), WithDotEnv)
 	assert.NilError(t, err)
-	p, err := opts.LoadProject(context.TODO())
+	p, err := ProjectFromOptions(context.TODO(), opts)
 	assert.NilError(t, err)
 	service, err := p.GetService("simple")
 	assert.NilError(t, err)
diff --git a/cli/testdata/env-file/compose-with-env-files.yaml b/cli/testdata/env-file/compose-with-env-files.yaml
@@ -1,3 +1,4 @@
+version: "3"
 services:
   simple:
     image: nginx
diff --git a/loader/extends.go b/loader/extends.go
@@ -22,11 +22,8 @@ import (
 	"path/filepath"
 
 	"github.com/compose-spec/compose-go/v2/consts"
-	"github.com/compose-spec/compose-go/v2/interpolation"
 	"github.com/compose-spec/compose-go/v2/override"
 	"github.com/compose-spec/compose-go/v2/paths"
-	"github.com/compose-spec/compose-go/v2/template"
-	"github.com/compose-spec/compose-go/v2/transform"
 	"github.com/compose-spec/compose-go/v2/types"
 )
 
@@ -71,22 +68,10 @@ func applyServiceExtends(ctx context.Context, name string, services map[string]a
 	)
 	switch v := extends.(type) {
 	case map[string]any:
-		if opts.Interpolate != nil {
-			v, err = interpolation.Interpolate(v, *opts.Interpolate)
-			if err != nil {
-				return nil, err
-			}
-		}
 		ref = v["service"].(string)
 		file = v["file"]
 		opts.ProcessEvent("extends", v)
 	case string:
-		if opts.Interpolate != nil {
-			v, err = opts.Interpolate.Substitute(v, template.Mapping(opts.Interpolate.LookupValue))
-			if err != nil {
-				return nil, err
-			}
-		}
 		ref = v
 		opts.ProcessEvent("extends", map[string]any{"service": ref})
 	}
@@ -190,12 +175,6 @@ func getExtendsBaseFromFile(
 			)
 		}
 
-		// Attempt to make a canonical model so ResolveRelativePaths can operate on source:target short syntax
-		source, err = transform.Canonical(source, true)
-		if err != nil {
-			return nil, nil, err
-		}
-
 		var remotes []paths.RemoteResource
 		for _, loader := range opts.RemoteResourceLoaders() {
 			remotes = append(remotes, loader.Accept)
diff --git a/loader/include.go b/loader/include.go
@@ -30,7 +30,7 @@ import (
 )
 
 // loadIncludeConfig parse the required config from raw yaml
-func loadIncludeConfig(source any, options *Options) ([]types.IncludeConfig, error) {
+func loadIncludeConfig(source any) ([]types.IncludeConfig, error) {
 	if source == nil {
 		return nil, nil
 	}
@@ -45,23 +45,13 @@ func loadIncludeConfig(source any, options *Options) ([]types.IncludeConfig, err
 			}
 		}
 	}
-	if options.Interpolate != nil {
-		for i, config := range configs {
-			interpolated, err := interp.Interpolate(config.(map[string]any), *options.Interpolate)
-			if err != nil {
-				return nil, err
-			}
-			configs[i] = interpolated
-		}
-	}
-
 	var requires []types.IncludeConfig
 	err := Transform(source, &requires)
 	return requires, err
 }
 
 func ApplyInclude(ctx context.Context, workingDir string, environment types.Mapping, model map[string]any, options *Options, included []string) error {
-	includeConfig, err := loadIncludeConfig(model["include"], options)
+	includeConfig, err := loadIncludeConfig(model["include"])
 	if err != nil {
 		return err
 	}
diff --git a/loader/loader.go b/loader/loader.go
@@ -367,23 +367,6 @@ func loadYamlModel(ctx context.Context, config types.ConfigDetails, opts *Option
 		}
 	}
 
-	if opts.Interpolate != nil && !opts.SkipInterpolation {
-		dict, err = interp.Interpolate(dict, *opts.Interpolate)
-		if err != nil {
-			return nil, err
-		}
-	}
-
-	dict, err = transform.Canonical(dict, opts.SkipInterpolation)
-	if err != nil {
-		return nil, err
-	}
-
-	dict, err = override.EnforceUnicity(dict)
-	if err != nil {
-		return nil, err
-	}
-
 	if !opts.SkipDefaultValues {
 		dict, err = transform.SetDefaultValues(dict)
 		if err != nil {
@@ -432,6 +415,13 @@ func loadYamlFile(ctx context.Context, file types.ConfigFile, opts *Options, wor
 			return errors.New("Top-level object must be a mapping")
 		}
 
+		if opts.Interpolate != nil && !opts.SkipInterpolation {
+			cfg, err = interp.Interpolate(cfg, *opts.Interpolate)
+			if err != nil {
+				return err
+			}
+		}
+
 		fixEmptyNotNull(cfg)
 
 		if !opts.SkipExtends {
@@ -460,6 +450,11 @@ func loadYamlFile(ctx context.Context, file types.ConfigFile, opts *Options, wor
 			return err
 		}
 
+		dict, err = override.EnforceUnicity(dict)
+		if err != nil {
+			return err
+		}
+
 		if !opts.SkipValidation {
 			if err := schema.Validate(dict); err != nil {
 				return fmt.Errorf("validating %s: %w", file.Filename, err)
@@ -469,7 +464,15 @@ func loadYamlFile(ctx context.Context, file types.ConfigFile, opts *Options, wor
 				delete(dict, "version")
 			}
 		}
-		return nil
+
+		dict, err = transform.Canonical(dict, opts.SkipInterpolation)
+		if err != nil {
+			return err
+		}
+
+		// Canonical transformation can reveal duplicates, typically as ports can be a range and conflict with an override
+		dict, err = override.EnforceUnicity(dict)
+		return err
 	}
 
 	var processor PostProcessor
diff --git a/loader/loader_yaml_test.go b/loader/loader_yaml_test.go
@@ -56,46 +56,6 @@ services:
 	})
 }
 
-func TestParseYAMLFilesInterpolateAfterMerge(t *testing.T) {
-	model, err := loadYamlModel(
-		context.TODO(), types.ConfigDetails{
-			ConfigFiles: []types.ConfigFile{
-				{
-					Filename: "test.yaml",
-					Content: []byte(`
-services:
-  test:
-    image: foo
-    environment:
-      my_env: ${my_env?my_env must be set}
-`),
-				},
-				{
-					Filename: "override.yaml",
-					Content: []byte(`
-services:
-  test:
-    image: bar
-    environment:
-      my_env: ${my_env:-default}
-`),
-				},
-			},
-		}, &Options{}, &cycleTracker{}, nil,
-	)
-	assert.NilError(t, err)
-	assert.DeepEqual(
-		t, model, map[string]interface{}{
-			"services": map[string]interface{}{
-				"test": map[string]interface{}{
-					"image":       "bar",
-					"environment": []any{"my_env=${my_env:-default}"},
-				},
-			},
-		},
-	)
-}
-
 func TestParseYAMLFilesMergeOverride(t *testing.T) {
 	model, err := loadYamlModel(context.TODO(), types.ConfigDetails{
 		ConfigFiles: []types.ConfigFile{
diff --git a/schema/compose-spec.json b/schema/compose-spec.json
@@ -155,8 +155,8 @@
           },
           "additionalProperties": false
         },
-        "cap_add": {"type": "array", "items": {"type": "string"}},
-        "cap_drop": {"type": "array", "items": {"type": "string"}},
+        "cap_add": {"type": "array", "items": {"type": "string"}, "uniqueItems": true},
+        "cap_drop": {"type": "array", "items": {"type": "string"}, "uniqueItems": true},
         "cgroup": {"type": "string", "enum": ["host", "private"]},
         "cgroup_parent": {"type": "string"},
         "command": {"$ref": "#/definitions/command"},
@@ -216,9 +216,9 @@
           ]
         },
         "device_cgroup_rules": {"$ref": "#/definitions/list_of_strings"},
-        "devices": {"type": "array", "items": {"type": "string"}},
+        "devices": {"type": "array", "items": {"type": "string"}, "uniqueItems": true},
         "dns": {"$ref": "#/definitions/string_or_list"},
-        "dns_opt": {"type": "array","items": {"type": "string"}},
+        "dns_opt": {"type": "array","items": {"type": "string"}, "uniqueItems": true},
         "dns_search": {"$ref": "#/definitions/string_or_list"},
         "domainname": {"type": "string"},
         "entrypoint": {"$ref": "#/definitions/command"},
@@ -230,7 +230,8 @@
           "items": {
             "type": ["string", "number"],
             "format": "expose"
-          }
+          },
+          "uniqueItems": true
         },
         "extends": {
           "oneOf": [
@@ -247,13 +248,14 @@
             }
           ]
         },
-        "external_links": {"type": "array", "items": {"type": "string"}},
+        "external_links": {"type": "array", "items": {"type": "string"}, "uniqueItems": true},
         "extra_hosts": {"$ref": "#/definitions/list_or_dict"},
         "group_add": {
           "type": "array",
           "items": {
             "type": ["string", "number"]
-          }
+          },
+          "uniqueItems": true
         },
         "healthcheck": {"$ref": "#/definitions/healthcheck"},
         "hostname": {"type": "string"},
@@ -262,7 +264,7 @@
         "ipc": {"type": "string"},
         "isolation": {"type": "string"},
         "labels": {"$ref": "#/definitions/list_or_dict"},
-        "links": {"type": "array", "items": {"type": "string"}},
+        "links": {"type": "array", "items": {"type": "string"}, "uniqueItems": true},
         "logging": {
           "type": "object",
 
@@ -348,7 +350,8 @@
                 "patternProperties": {"^x-": {}}
               }
             ]
-          }
+          },
+          "uniqueItems": true
         },
         "privileged": {"type": ["boolean", "string"]},
         "profiles": {"$ref": "#/definitions/list_of_strings"},
@@ -363,7 +366,7 @@
         "scale": {
           "type": ["integer", "string"]
         },
-        "security_opt": {"type": "array", "items": {"type": "string"}},
+        "security_opt": {"type": "array", "items": {"type": "string"}, "uniqueItems": true},
         "shm_size": {"type": ["number", "string"]},
         "secrets": {"$ref": "#/definitions/service_config_or_secret"},
         "sysctls": {"$ref": "#/definitions/list_or_dict"},
@@ -429,11 +432,13 @@
                 "patternProperties": {"^x-": {}}
               }
             ]
-          }
+          },
+          "uniqueItems": true
         },
         "volumes_from": {
           "type": "array",
-          "items": {"type": "string"}
+          "items": {"type": "string"},
+          "uniqueItems": true
         },
         "working_dir": {"type": "string"}
       },
@@ -828,7 +833,8 @@
 
     "list_of_strings": {
       "type": "array",
-      "items": {"type": "string"}
+      "items": {"type": "string"},
+      "uniqueItems": true
     },
 
     "list_or_dict": {
@@ -842,7 +848,7 @@
           },
           "additionalProperties": false
         },
-        {"type": "array", "items": {"type": "string"}}
+        {"type": "array", "items": {"type": "string"}, "uniqueItems": true}
       ]
     },
 

Original file line number	Diff line number	Diff line change
`@@ -1,3 +1,4 @@`
	`1`	`+version: "3"`
`1`	`2`	`services:`
`2`	`3`	`simple:`
`3`	`4`	`image: nginx`
Original file line number	Diff line number	Diff line change
`@@ -30,7 +30,7 @@ import (`
`30`	`30`	`)`
`31`	`31`
`32`	`32`	`// loadIncludeConfig parse the required config from raw yaml`
`33`		`-func loadIncludeConfig(source any, options *Options) ([]types.IncludeConfig, error) {`
	`33`	`+func loadIncludeConfig(source any) ([]types.IncludeConfig, error) {`
`34`	`34`	`if source == nil {`
`35`	`35`	`return nil, nil`
`36`	`36`	`}`
`@@ -45,23 +45,13 @@ func loadIncludeConfig(source any, options *Options) ([]types.IncludeConfig, err`
`45`	`45`	`}`
`46`	`46`	`}`
`47`	`47`	`}`
`48`		`- if options.Interpolate != nil {`
`49`		`- for i, config := range configs {`
`50`		`- interpolated, err := interp.Interpolate(config.(map[string]any), *options.Interpolate)`
`51`		`- if err != nil {`
`52`		`- return nil, err`
`53`		`- }`
`54`		`- configs[i] = interpolated`
`55`		`- }`
`56`		`- }`
`57`		`-`
`58`	`48`	`var requires []types.IncludeConfig`
`59`	`49`	`err := Transform(source, &requires)`
`60`	`50`	`return requires, err`
`61`	`51`	`}`
`62`	`52`
`63`	`53`	`func ApplyInclude(ctx context.Context, workingDir string, environment types.Mapping, model map[string]any, options *Options, included []string) error {`
`64`		`- includeConfig, err := loadIncludeConfig(model["include"], options)`
	`54`	`+ includeConfig, err := loadIncludeConfig(model["include"])`
`65`	`55`	`if err != nil {`
`66`	`56`	`return err`
`67`	`57`	`}`