Permit environment sections to be maps instead of lists (#305)

Lists are still permitted for backward compatibility. I've updated the
docs to use the new map style, as it's a better fit for YAML structures.

[Fixes ch4916]

CHANGELOG.md

@@ -13,6 +13,8 @@ The format is based on [Keep a Changelog][], and this project adheres to
 -  A new `--mode` option for `build`, `exec`, and `run` allows specifying
    whether commands should be run inside or outside Docker.
 -  `yb build` can now build multiple targets in one invocation.
+-  Environment variables in `.yourbase.yml` files may now be specified as a map
+   (e.g. `FOO: BAR`) instead of a list (e.g. `- FOO=BAR`).
 -  Build environments will now pick up credentials from `$HOME/.netrc` after any
    credentials from `$XDG_CONFIG_HOME/yb/netrc`. This can be overridden with the
    `NETRC` environment variable. To revert to the previous behavior, set

docs/configuration.md

@@ -37,8 +37,18 @@ build_targets:
 
 - `dependencies`: See the [Dependencies section](#dependencies).
 
-- `environment`: A list of `KEY=VALUE` items that are used as environment
-  variables.
+- `environment`: A map of environment variables.
+
+  ```yaml
+  build_targets:
+    - name: default
+      environment:
+        ENVIRONMENT: development
+        DATABASE_URL: db://localhost:1234/foo
+  ```
+
+  For backwards compatibility, environment variables may also be written as a
+  list of `KEY=VALUE` pairs.
 
   ```yaml
   build_targets:
@@ -91,7 +101,7 @@ build_targets:
       container:
         image: yourbase/yb_ubuntu:18.04
       environment:
-        - DEBIAN_FRONTEND=noninteractive
+        DEBIAN_FRONTEND: noninteractive
       commands:
         - apt-get update
         - apt-get install -y --no-install-recommends cowsay
@@ -194,18 +204,18 @@ build_targets:
         db:
           image: postgres:12
           environment:
-            - POSTGRES_USER=myapp
-            - POSTGRES_PASSWORD=xyzzy
-            - POSTGRES_DB=myapp
+            POSTGRES_USER: myapp
+            POSTGRES_PASSWORD: xyzzy
+            POSTGRES_DB: myapp
           port_check:
             port: 5432
             timeout: 90
     environment:
       # Use whatever environment variables make sense for your test suite.
-      - POSTGRES_HOST={{ .Containers.IP "db" }}
-      - POSTGRES_USER=myapp
-      - POSTGRES_PASSWORD=xyzzy
-      - POSTGRES_DB=myapp
+      POSTGRES_HOST: '{{ .Containers.IP "db" }}'
+      POSTGRES_USER: myapp
+      POSTGRES_PASSWORD: xyzzy
+      POSTGRES_DB: myapp
     commands:
       - ./run_tests.sh
 ```
@@ -227,8 +237,9 @@ syntax.
 - `workdir`: The working directory to run inside the container. Defaults to the
   `WORKDIR` specified in the image's Dockerfile.
 
-- `environment`: A list of `KEY=VALUE` items that are used as environment
-  variables inside the container.
+- `environment`: A map of environment variables inside the container. For
+  backwards compatibility, these may also be written as a list of `KEY=VALUE`
+  pairs.
 
 - `port_check`: If present, yb will wait until a TCP health check passes before
   continuing with the build. The check is specified by two parameters: `port`
@@ -265,8 +276,9 @@ section has the same properties as a target (as described in the
 [Build Targets section](#build-targets)), with a few small differences:
 
 - Executable targets do not support `build_after`.
-- The `environment` attribute is a map of `KEY=VALUE` lists. The `default`
-  environment is used if the `yb exec --environment` flag is not specified.
+- The `environment` attribute is a map of environment names to environment
+  variable maps. The `default` environment is used if the `yb exec --environment`
+  flag is not specified.
 
 ```yaml
 exec:
@@ -278,7 +290,7 @@ exec:
       - python:3.9.2
   environment:
     default:
-      - DJANGO_SETTINGS_MODULE=mysite.settings
+      DJANGO_SETTINGS_MODULE: mysite.settings
   commands:
     - python manage.py runserver
 ```

parse.go

@@ -66,7 +66,7 @@ type buildTarget struct {
 	Commands     []string             `yaml:"commands"`
 	HostOnly     bool                 `yaml:"host_only"`
 	Root         string               `yaml:"root"`
-	Environment  []string             `yaml:"environment"`
+	Environment  envObject            `yaml:"environment"`
 	Tags         map[string]string    `yaml:"tags"`
 	BuildAfter   []string             `yaml:"build_after"`
 	Dependencies buildDependencies    `yaml:"dependencies"`
@@ -153,8 +153,8 @@ func parseTarget(packageDir string, globalDeps map[string]BuildpackSpec, tgt *bu
 	if err := parseBuildpacks(parsed.Buildpacks, tgt.Dependencies.Build); err != nil {
 		return nil, fmt.Errorf("target %s: dependencies: build: %w", tgt.Name, err)
 	}
-	if err := parseEnv(parsed.Env, tgt.Environment); err != nil {
-		return nil, fmt.Errorf("target %s: environment: %w", tgt.Name, err)
+	if tgt.Environment != nil {
+		parsed.Env = tgt.Environment
 	}
 	return parsed, nil
 }
@@ -195,7 +195,7 @@ type execPhase struct {
 	Dependencies execDependencies     `yaml:"dependencies"`
 	Container    *containerDefinition `yaml:"container"`
 	Commands     []string             `yaml:"commands"`
-	Environment  map[string][]string  `yaml:"environment"`
+	Environment  map[string]envObject `yaml:"environment"`
 	LogFiles     []string             `yaml:"logfiles"`
 	Sandbox      bool                 `yaml:"sandbox"`
 	HostOnly     bool                 `yaml:"host_only"`
@@ -235,8 +235,8 @@ func parseExecPhase(pkg *Package, manifest *buildManifest) (map[string]*Target,
 		Buildpacks:   buildpacks,
 		Resources:    resources,
 	}
-	if err := parseEnv(defaultTarget.Env, manifest.Exec.Environment[defaultTarget.Name]); err != nil {
-		return nil, fmt.Errorf("exec environment: %s: %w", defaultTarget.Name, err)
+	if manifest.Exec.Environment != nil {
+		defaultTarget.Env = manifest.Exec.Environment[defaultTarget.Name]
 	}
 
 	// Clone target for different environments.
@@ -253,8 +253,8 @@ func parseExecPhase(pkg *Package, manifest *buildManifest) (map[string]*Target,
 		for k, v := range defaultTarget.Env {
 			newTarget.Env[k] = v
 		}
-		if err := parseEnv(newTarget.Env, env); err != nil {
-			return nil, fmt.Errorf("exec environment: %s: %w", newTarget.Name, err)
+		for k, v := range env {
+			newTarget.Env[k] = v
 		}
 		targetMap[name] = newTarget
 	}
@@ -268,7 +268,7 @@ type containerDefinition struct {
 	Image         string        `yaml:"image"`
 	Mounts        []string      `yaml:"mounts"`
 	Ports         []string      `yaml:"ports"`
-	Environment   []string      `yaml:"environment"`
+	Environment   envObject     `yaml:"environment"`
 	Command       string        `yaml:"command"`
 	WorkDir       string        `yaml:"workdir"`
 	PortWaitCheck portWaitCheck `yaml:"port_check"`
@@ -287,6 +287,10 @@ func (def *containerDefinition) toResource(packageDir string) (*ResourceDefiniti
 	if def.Image != "" {
 		image = def.Image
 	}
+	var env []string
+	for k, v := range def.Environment {
+		env = append(env, k+"="+string(v))
+	}
 	var mounts []docker.HostMount
 	for _, s := range def.Mounts {
 		mount, err := parseHostMount(packageDir, s)
@@ -300,7 +304,7 @@ func (def *containerDefinition) toResource(packageDir string) (*ResourceDefiniti
 			Image:       image,
 			Mounts:      mounts,
 			Ports:       append([]string(nil), def.Ports...),
-			Environment: append([]string(nil), def.Environment...),
+			Environment: append([]string(nil), env...),
 			Argv:        strings.Fields(def.Command),
 			WorkDir:     def.WorkDir,
 
@@ -347,19 +351,36 @@ type portWaitCheck struct {
 	Timeout int `yaml:"timeout"`
 }
 
-// parseEnv fills a map of variables from a list of variables in the
-// form "key=value". If the list contains duplicate variables, only the last
-// value in the slice for each duplicate key is used.
-func parseEnv(dst map[string]EnvTemplate, vars []string) error {
-	if len(vars) == 0 {
+// envObject is either a YAML map or YAML list that is deserialized into a map
+// of environment variables.
+//
+// If a YAML list is encountered, then it must be a list of strings in the form
+// "key=value". If the list contains duplicate variables, only the last value in
+// the slice for each duplicate key is used.
+type envObject map[string]EnvTemplate
+
+// UnmarshalYAML implements yaml.Unmarshaler.
+// https://pkg.go.dev/gopkg.in/yaml.v2#Unmarshaler
+func (eo *envObject) UnmarshalYAML(unmarshal func(interface{}) error) error {
+	if *eo == nil {
+		*eo = make(envObject)
+	}
+	mapErr := unmarshal(map[string]EnvTemplate(*eo))
+	if mapErr == nil {
 		return nil
 	}
+	var vars []string
+	if err := unmarshal(&vars); err != nil {
+		// Prefer presenting the unmarshal-map error, since we want to encourage
+		// that form more.
+		return mapErr
+	}
 	for _, kv := range vars {
 		k, v, err := parseVar(kv)
 		if err != nil {
 			return err
 		}
-		dst[k] = EnvTemplate(v)
+		(*eo)[k] = EnvTemplate(v)
 	}
 	return nil
 }

parse_test.go

@@ -25,6 +25,7 @@ import (
 	"github.com/google/go-cmp/cmp"
 	"github.com/google/go-cmp/cmp/cmpopts"
 	"github.com/yourbase/narwhal"
+	"gopkg.in/yaml.v2"
 )
 
 func TestLoadPackage(t *testing.T) {
@@ -129,6 +130,49 @@ func TestLoadPackage(t *testing.T) {
 				},
 			},
 		},
+		{
+			name: "Environment",
+			want: &Package{
+				Targets: map[string]*Target{
+					"list": {
+						Name: "list",
+						Container: &narwhal.ContainerDefinition{
+							Image: DefaultContainerImage,
+							Environment: []string{
+								"FOO=XYZZY",
+								"BAZ=QUUX",
+							},
+						},
+						UseContainer: true,
+						Env: map[string]EnvTemplate{
+							"FOO": "XYZZY",
+							"BAZ": "QUUX",
+						},
+						Commands: []string{
+							"/bin/true",
+						},
+					},
+					"kv": {
+						Name: "kv",
+						Container: &narwhal.ContainerDefinition{
+							Image: DefaultContainerImage,
+							Environment: []string{
+								"FOO=BAR",
+								"BAZ=QUUX",
+							},
+						},
+						UseContainer: true,
+						Env: map[string]EnvTemplate{
+							"FOO": "BAR",
+							"BAZ": "QUUX",
+						},
+						Commands: []string{
+							"/bin/true",
+						},
+					},
+				},
+			},
+		},
 		{
 			name: "Mounts",
 			want: &Package{
@@ -294,9 +338,21 @@ func TestLoadPackage(t *testing.T) {
 				}
 			}
 			diff := cmp.Diff(test.want, got,
+				// Equate empty collections unless it's a set of targets.
 				cmp.FilterPath(func(p cmp.Path) bool {
 					return p.Last().Type() != reflect.TypeOf(map[*Target]struct{}(nil))
 				}, cmpopts.EquateEmpty()),
+				// Ignore order in ContainerDefinition.Environment.
+				cmp.FilterPath(
+					func(p cmp.Path) bool {
+						return p.Index(-2).Type() == reflect.TypeOf(narwhal.ContainerDefinition{}) &&
+							p.Last().(cmp.StructField).Name() == "Environment"
+					},
+					cmpopts.SortSlices(func(s1, s2 string) bool {
+						return s1 < s2
+					}),
+				),
+				// Ignore package fields, since it's environment-dependent.
 				cmpopts.IgnoreFields(Package{}, "Name", "Path"),
 				cmpopts.IgnoreFields(Target{}, "Package"),
 				// Compare Deps by name.
@@ -319,45 +375,53 @@ func TestLoadPackage(t *testing.T) {
 	}
 }
 
-func TestParseEnv(t *testing.T) {
+func TestEnvObjectUnmarshal(t *testing.T) {
 	tests := []struct {
-		vars      []string
-		want      map[string]EnvTemplate
+		yaml      string
+		want      envObject
 		wantError bool
 	}{
 		{
-			vars: nil,
+			yaml: `[]`,
 			want: nil,
 		},
 		{
-			vars: []string{"FOO=BAR"},
-			want: map[string]EnvTemplate{"FOO": "BAR"},
+			yaml: `["FOO=BAR"]`,
+			want: envObject{"FOO": "BAR"},
 		},
 		{
-			vars: []string{"FOO=BAR", "BAZ=QUUX"},
-			want: map[string]EnvTemplate{"FOO": "BAR", "BAZ": "QUUX"},
+			yaml: `["FOO=BAR", "BAZ=QUUX"]`,
+			want: envObject{"FOO": "BAR", "BAZ": "QUUX"},
 		},
 		{
-			vars: []string{"FOO=BAR", "FOO=BAZ"},
-			want: map[string]EnvTemplate{"FOO": "BAZ"},
+			yaml: `["FOO=BAR", "FOO=BAZ"]`,
+			want: envObject{"FOO": "BAZ"},
 		},
 		{
-			vars:      []string{"FOO"},
+			yaml:      `["FOO"]`,
 			wantError: true,
 		},
+		{
+			yaml: "FOO: BAR\n",
+			want: envObject{"FOO": "BAR"},
+		},
+		{
+			yaml: "FOO: 1\n",
+			want: envObject{"FOO": "1"},
+		},
 	}
 	for _, test := range tests {
-		got := make(map[string]EnvTemplate)
-		err := parseEnv(got, test.vars)
+		var got envObject
+		err := yaml.UnmarshalStrict([]byte(test.yaml), &got)
 		if err != nil {
-			t.Logf("parseEnv(m, %q) = %v", test.vars, err)
+			t.Logf("yaml.UnmarshalStrict(%q, &m) = %v", test.yaml, err)
 			if !test.wantError {
 				t.Fail()
 			}
 			continue
 		}
 		if test.wantError {
-			t.Errorf("after parseEnv(m, %q), m = %v; want error", test.vars, got)
+			t.Errorf("after yaml.UnmarshalStrict(%q, &m), m = %v; want error", test.yaml, got)
 		}
 		if diff := cmp.Diff(test.want, got, cmpopts.EquateEmpty()); diff != "" {
 			t.Errorf("map (-want +got):\n%s", diff)

testdata/LoadPackage/Environment.yml

@@ -0,0 +1,24 @@
+build_targets:
+  - name: list
+    environment:
+      - FOO=BAR
+      - BAZ=QUUX
+      - FOO=XYZZY
+    container:
+      environment:
+        - FOO=BAR
+        - BAZ=QUUX
+        - FOO=XYZZY
+    commands:
+      - /bin/true
+
+  - name: kv
+    environment:
+      FOO: BAR
+      BAZ: QUUX
+    container:
+      environment:
+        FOO: BAR
+        BAZ: QUUX
+    commands:
+      - /bin/true