[sg] fix environment overrides for bazel and docker commands (#61260)

* implemented overlooked overrides

* switched to functional merge

* Update dev/sg/internal/run/bazel_command.go

Co-authored-by: Jean-Hadrien Chabran <jean-hadrien.chabran@sourcegraph.com>

* Update dev/sg/internal/sgconf/config.go

Co-authored-by: Jean-Hadrien Chabran <jean-hadrien.chabran@sourcegraph.com>

* addressed comments

---------

Co-authored-by: Jean-Hadrien Chabran <jean-hadrien.chabran@sourcegraph.com>

dev/sg/internal/run/bazel_command.go

@@ -12,6 +12,7 @@ import (
 // A BazelCommand is a command definition for sg run/start that uses
 // bazel under the hood. It will handle restarting itself autonomously,
 // as long as iBazel is running and watch that specific target.
+// Note: if you add a field here be sure to add it to the `Merge` method
 type BazelCommand struct {
 	Config SGConfigCommandOptions
 	Target string `yaml:"target"`
@@ -88,6 +89,18 @@ func (bc BazelCommand) GetExecCmd(ctx context.Context) (*exec.Cmd, error) {
 	return exec.CommandContext(ctx, "bash", "-c", fmt.Sprintf("%s\n%s", bc.Config.PreCmd, cmd)), nil
 }
 
+// Merge overrides the behavior of this command with other command.
+// This is used for the sg.config.overwrite.yaml functionality
+func (bc BazelCommand) Merge(other BazelCommand) BazelCommand {
+	merged := bc
+
+	merged.Config = bc.Config.Merge(other.Config)
+	merged.Target = mergeStrings(merged.Target, other.Target)
+	merged.RunTarget = mergeStrings(merged.RunTarget, other.RunTarget)
+
+	return merged
+}
+
 func binaryLocation(target string) (string, error) {
 	// Get the output directory from Bazel, which varies depending on which OS
 	// we're running against.

dev/sg/internal/run/command.go

@@ -143,24 +143,41 @@ func (c Command) Merge(other Command) Command {
 	merged := c
 
 	merged.Config = c.Config.Merge(other.Config)
-
-	if other.Cmd != merged.Cmd && other.Cmd != "" {
-		merged.Cmd = other.Cmd
-	}
-	if other.Install != merged.Install && other.Install != "" {
-		merged.Install = other.Install
-	}
-	if other.InstallFunc != merged.InstallFunc && other.InstallFunc != "" {
-		merged.InstallFunc = other.InstallFunc
-	}
-
-	if !equal(merged.Watch, other.Watch) && len(other.Watch) != 0 {
-		merged.Watch = other.Watch
-	}
-
+	merged.Cmd = mergeStrings(c.Cmd, other.Cmd)
+	merged.Install = mergeStrings(c.Install, other.Install)
+	merged.InstallFunc = mergeStrings(c.InstallFunc, other.InstallFunc)
+	merged.Watch = mergeSlices(c.Watch, other.Watch)
 	return merged
 }
 
+func mergeStrings(a, b string) string {
+	if b != "" {
+		return b
+	}
+	return a
+}
+
+func mergeSlices[T any](a, b []T) []T {
+	if len(b) > 0 {
+		return b
+	}
+	return a
+}
+
+// Merge maps properly merges the two, as opposed to every other merge method which
+// simply overwrites the first with the second.
+// This is to preserve the behavior of the original code.
+func mergeMaps[K comparable, V any](a, b map[K]V) map[K]V {
+	if a == nil {
+		return b
+	}
+	for k, v := range b {
+		a[k] = v
+	}
+
+	return a
+}
+
 func equal(a, b []string) bool {
 	if len(a) != len(b) {
 		return false

dev/sg/internal/run/docker_commmand.go

@@ -15,6 +15,7 @@ import (
 // A DockerCommand is a command definition for sg run/start that uses
 // bazel under the hood. It will handle restarting itself autonomously,
 // as long as iBazel is running and watch that specific target.
+// Note: if you add a field here be sure to add it to the `Merge` method
 type DockerCommand struct {
 	Config SGConfigCommandOptions
 	Docker DockerOptions `yaml:"docker"`
@@ -39,6 +40,17 @@ type DockerOptions struct {
 	Linux DockerLinuxOptions `yaml:"linux"`
 }
 
+func (do DockerOptions) Merge(other DockerOptions) DockerOptions {
+	merged := do
+	merged.Image = mergeStrings(merged.Image, other.Image)
+	merged.Pull = merged.Pull || other.Pull
+	merged.Volumes = mergeSlices(merged.Volumes, other.Volumes)
+	merged.Flags = mergeMaps(merged.Flags, other.Flags)
+	merged.Ports = mergeSlices(merged.Ports, other.Ports)
+	merged.Linux = merged.Linux.Merge(other.Linux)
+	return merged
+}
+
 // DockerLinuxOptions is a struct that holds linux specific modifications to
 // DockerEngine parameters for the DockerCommand
 type DockerLinuxOptions struct {
@@ -46,6 +58,14 @@ type DockerLinuxOptions struct {
 	Env   map[string]string `yaml:"env"`
 }
 
+func (dlo DockerLinuxOptions) Merge(other DockerLinuxOptions) DockerLinuxOptions {
+	merged := dlo
+	merged.Flags = mergeMaps(merged.Flags, other.Flags)
+	merged.Env = mergeMaps(merged.Env, other.Env)
+
+	return merged
+}
+
 // Details for a docker volume to mount into the container
 type DockerVolume struct {
 	From string `yaml:"from"`
@@ -212,6 +232,18 @@ func (dc DockerCommand) GetExecCmd(ctx context.Context) (*exec.Cmd, error) {
 	return exec.CommandContext(ctx, "bash", "-c", cmd), nil
 }
 
+// Overrides the behavior of this command with other command.
+// This is used for the sg.config.overwrite.yaml functionality
+func (bc DockerCommand) Merge(other DockerCommand) DockerCommand {
+	merged := bc
+
+	merged.Target = mergeStrings(merged.Target, other.Target)
+	merged.Config = merged.Config.Merge(other.Config)
+	merged.Docker = merged.Docker.Merge(other.Docker)
+
+	return merged
+}
+
 type Entry[K, V any] struct {
 	Key   K
 	Value V

dev/sg/internal/run/sgconfig_command_options.go

@@ -29,51 +29,18 @@ type SGConfigCommandOptions struct {
 func (opts SGConfigCommandOptions) Merge(other SGConfigCommandOptions) SGConfigCommandOptions {
 	merged := opts
 
-	if other.Name != merged.Name && other.Name != "" {
-		merged.Name = other.Name
-	}
-	if other.Description != merged.Description && other.Description != "" {
-		merged.Description = other.Description
-	}
-	if other.Description != merged.Description && other.Description != "" {
-		merged.Description = other.Description
-	}
-	if other.PreCmd != merged.PreCmd && other.PreCmd != "" {
-		merged.PreCmd = other.PreCmd
-	}
-	if other.Args != merged.Args && other.Args != "" {
-		merged.Args = other.Args
-	}
-	if other.IgnoreStdout != merged.IgnoreStdout && !merged.IgnoreStdout {
-		merged.IgnoreStdout = other.IgnoreStdout
-	}
-	if other.IgnoreStderr != merged.IgnoreStderr && !merged.IgnoreStderr {
-		merged.IgnoreStderr = other.IgnoreStderr
-	}
+	merged.Name = mergeStrings(merged.Name, other.Name)
+	merged.Description = mergeStrings(merged.Description, other.Description)
+	merged.PreCmd = mergeStrings(merged.PreCmd, other.PreCmd)
+	merged.Args = mergeStrings(merged.Args, other.Args)
+	merged.IgnoreStdout = other.IgnoreStdout || merged.IgnoreStdout
+	merged.IgnoreStderr = other.IgnoreStderr || merged.IgnoreStderr
 	merged.ContinueWatchOnExit = other.ContinueWatchOnExit || merged.ContinueWatchOnExit
-	if other.Preamble != merged.Preamble && other.Preamble != "" {
-		merged.Preamble = other.Preamble
-	}
-	if other.Logfile != merged.Logfile && other.Logfile != "" {
-		merged.Logfile = other.Logfile
-	}
-	if other.RepositoryRoot != merged.RepositoryRoot && other.RepositoryRoot != "" {
-		merged.RepositoryRoot = other.RepositoryRoot
-	}
-
-	for k, v := range other.Env {
-		if merged.Env == nil {
-			merged.Env = make(map[string]string)
-		}
-		merged.Env[k] = v
-	}
-
-	for k, v := range other.ExternalSecrets {
-		if merged.ExternalSecrets == nil {
-			merged.ExternalSecrets = make(map[string]secrets.ExternalSecret)
-		}
-		merged.ExternalSecrets[k] = v
-	}
+	merged.Preamble = mergeStrings(merged.Preamble, other.Preamble)
+	merged.Logfile = mergeStrings(merged.Logfile, other.Logfile)
+	merged.RepositoryRoot = mergeStrings(merged.RepositoryRoot, other.RepositoryRoot)
+	merged.Env = mergeMaps(merged.Env, other.Env)
+	merged.ExternalSecrets = mergeMaps(merged.ExternalSecrets, other.ExternalSecrets)
 
 	return merged
 }

dev/sg/internal/sgconf/BUILD.bazel

@@ -13,6 +13,7 @@ go_library(
         "//dev/sg/internal/run",
         "//dev/sg/root",
         "//lib/errors",
+        "//lib/pointers",
         "@in_gopkg_yaml_v2//:yaml_v2",
     ],
 )

dev/sg/internal/sgconf/config.go

@@ -10,6 +10,7 @@ import (
 	"github.com/sourcegraph/sourcegraph/dev/sg/internal/run"
 	"github.com/sourcegraph/sourcegraph/dev/sg/root"
 	"github.com/sourcegraph/sourcegraph/lib/errors"
+	"github.com/sourcegraph/sourcegraph/lib/pointers"
 )
 
 func parseConfigFile(name string) (*Config, error) {
@@ -115,19 +116,19 @@ func (c *Commandset) Merge(other *Commandset) *Commandset {
 		merged.Name = other.Name
 	}
 
-	if !equal(merged.Commands, other.Commands) && len(other.Commands) != 0 {
+	if len(other.Commands) != 0 {
 		merged.Commands = other.Commands
 	}
 
-	if !equal(merged.Checks, other.Checks) && len(other.Checks) != 0 {
+	if len(other.Checks) != 0 {
 		merged.Checks = other.Checks
 	}
 
-	if !equal(merged.BazelCommands, other.BazelCommands) && len(other.BazelCommands) != 0 {
+	if len(other.BazelCommands) != 0 {
 		merged.BazelCommands = other.BazelCommands
 	}
 
-	if !equal(merged.DockerCommands, other.DockerCommands) && len(other.DockerCommands) != 0 {
+	if len(other.DockerCommands) != 0 {
 		merged.DockerCommands = other.DockerCommands
 	}
 
@@ -140,6 +141,7 @@ func (c *Commandset) Merge(other *Commandset) *Commandset {
 	return merged
 }
 
+// If you add an entry here, remember to add it to the merge function.
 type Config struct {
 	Env               map[string]string             `yaml:"env"`
 	Commands          map[string]*run.Command       `yaml:"commands"`
@@ -150,56 +152,59 @@ type Config struct {
 	Tests             map[string]*run.Command       `yaml:"tests"`
 }
 
-// Merges merges the top-level entries of two Config objects, with the receiver
-// being modified.
-func (c *Config) Merge(other *Config) {
+// Merge merges the top-level entries of two Config objects, using the
+// values from `other` if they are set as overrides and returns a new config
+func (c *Config) Merge(other *Config) *Config {
+	merged := *c
 	for k, v := range other.Env {
-		c.Env[k] = v
+		merged.Env[k] = v
 	}
 
-	for k, v := range other.Commands {
-		if original, ok := c.Commands[k]; ok {
-			merged := original.Merge(*v)
-			c.Commands[k] = &merged
+	for name, override := range other.Commands {
+		if original, ok := merged.Commands[name]; ok {
+			merged.Commands[name] = pointers.Ptr(original.Merge(*override))
 		} else {
-			c.Commands[k] = v
+			merged.Commands[name] = override
 		}
 	}
 
-	for k, v := range other.Commandsets {
-		if original, ok := c.Commandsets[k]; ok {
-			c.Commandsets[k] = original.Merge(v)
+	for name, override := range other.BazelCommands {
+		if original, ok := merged.BazelCommands[name]; ok {
+			merged.BazelCommands[name] = pointers.Ptr(original.Merge(*override))
 		} else {
-			c.Commandsets[k] = v
+			merged.BazelCommands[name] = override
+		}
+	}
+
+	for name, override := range other.DockerCommands {
+		if original, ok := merged.DockerCommands[name]; ok {
+			merged.DockerCommands[name] = pointers.Ptr(original.Merge(*override))
+		} else {
+			merged.DockerCommands[name] = override
+		}
+	}
+
+	for name, override := range other.Commandsets {
+		if original, ok := merged.Commandsets[name]; ok {
+			merged.Commandsets[name] = original.Merge(override)
+		} else {
+			merged.Commandsets[name] = override
 		}
 	}
 
 	if other.DefaultCommandset != "" {
-		c.DefaultCommandset = other.DefaultCommandset
+		merged.DefaultCommandset = other.DefaultCommandset
 	}
 
-	for k, v := range other.Tests {
-		if original, ok := c.Tests[k]; ok {
-			merged := original.Merge(*v)
-			c.Tests[k] = &merged
+	for name, override := range other.Tests {
+		if original, ok := merged.Tests[name]; ok {
+			merged.Tests[name] = pointers.Ptr(original.Merge(*override))
 		} else {
-			c.Tests[k] = v
-		}
-	}
-}
-
-func equal(a, b []string) bool {
-	if len(a) != len(b) {
-		return false
-	}
-
-	for i, v := range a {
-		if v != b[i] {
-			return false
+			merged.Tests[name] = override
 		}
 	}
 
-	return true
+	return &merged
 }
 
 func (c *Config) GetEnv(key string) string {

dev/sg/internal/sgconf/config_test.go

@@ -81,17 +81,45 @@ commandsets:
 
 func TestParseAndMerge(t *testing.T) {
 	a := `
+env:
+  GLOBAL_VAR: 'global var orig'
+  OVERRIDE_VAR: 'override var orig'
 commands:
   frontend:
     cmd: .bin/frontend
     install: go build .bin/frontend github.com/sourcegraph/sourcegraph/cmd/frontend
     checkBinary: .bin/frontend
     env:
-      EXTSVC_CONFIG_FILE: '../dev-private/enterprise/dev/external-services-config.json'
+      COMMAND_VAR: 'command local'
+      COMMAND_OVERRIDE_VAR: 'command local'
     watch:
       - lib
       - internal
       - cmd/frontend
+bazelCommands:
+  frontend:
+    target: //cmd/frontend
+    env:
+      BAZEL_VAR: 'bazel command local'
+      BAZEL_OVERRIDE_VAR: 'bazel command local'
+dockerCommands:
+  frontend:
+    docker:
+      image: grafana:candidate
+      ports:
+        - 3370
+      flags:
+        cpus: 1
+      volumes:
+          - from: src
+            to: dest
+      linux:
+          flags:
+            add-host: host.docker.internal:host-gateway
+            user: $UID
+    env:
+      DOCKER_VAR: 'docker command local'
+      DOCKER_OVERRIDE_VAR: 'docker command local'
 `
 	config, err := parseConfig([]byte(a))
 	if err != nil {
@@ -99,10 +127,36 @@ commands:
 	}
 
 	b := `
+env:
+  OVERRIDE_VAR: 'override var override'
 commands:
   frontend:
     env:
-      EXTSVC_CONFIG_FILE: ''
+      COMMAND_OVERRIDE_VAR: 'command override'
+bazelCommands:
+  frontend:
+    runTarget: //cmd/frontend-run
+    env:
+      BAZEL_OVERRIDE_VAR: 'bazel command override'
+dockerCommands:
+  frontend:
+    docker:
+      image: grafana:update
+      ports:
+        - 3370
+        - 3371
+      flags:
+        memory: 1g
+      volumes:
+          - from: override-src
+            to: dst
+      linux:
+          flags:
+            user: root
+          env:
+            FOO: bar
+    env:
+      DOCKER_OVERRIDE_VAR: 'docker command override'
 `
 
 	overwrite, err := parseConfig([]byte(b))
@@ -110,30 +164,75 @@ commands:
 		t.Errorf("unexpected error: %s", err)
 	}
 
-	config.Merge(overwrite)
+	merged := config.Merge(overwrite)
 
-	cmd, ok := config.Commands["frontend"]
-	if !ok {
-		t.Fatalf("command not found")
-	}
-
-	want := &run.Command{
-		Config: run.SGConfigCommandOptions{
-			Name:           "frontend",
-			Env:            map[string]string{"EXTSVC_CONFIG_FILE": ""},
-			RepositoryRoot: os.Getenv("SG_FORCE_REPO_ROOT"),
+	want := &Config{
+		Env: map[string]string{
+			"GLOBAL_VAR":   "global var orig",
+			"OVERRIDE_VAR": "override var override",
+		},
+		Commands: map[string]*run.Command{"frontend": {
+			Config: run.SGConfigCommandOptions{
+				Name: "frontend",
+				Env: map[string]string{
+					"COMMAND_VAR":          "command local",
+					"COMMAND_OVERRIDE_VAR": "command override"},
+				RepositoryRoot: os.Getenv("SG_FORCE_REPO_ROOT"),
+			},
+			Cmd:         ".bin/frontend",
+			Install:     "go build .bin/frontend github.com/sourcegraph/sourcegraph/cmd/frontend",
+			CheckBinary: ".bin/frontend",
+			Watch: []string{
+				"lib",
+				"internal",
+				"cmd/frontend",
+			},
+		},
+		},
+		BazelCommands: map[string]*run.BazelCommand{"frontend": {
+			Config: run.SGConfigCommandOptions{
+				Name: "frontend",
+				Env: map[string]string{
+					"BAZEL_VAR":          "bazel command local",
+					"BAZEL_OVERRIDE_VAR": "bazel command override",
+				},
+				RepositoryRoot: os.Getenv("SG_FORCE_REPO_ROOT"),
+			},
+			Target:    "//cmd/frontend",
+			RunTarget: "//cmd/frontend-run",
+		},
+		},
+		DockerCommands: map[string]*run.DockerCommand{"frontend": {
+			Config: run.SGConfigCommandOptions{
+				Name: "frontend",
+				Env: map[string]string{
+					"DOCKER_VAR":          "docker command local",
+					"DOCKER_OVERRIDE_VAR": "docker command override",
+				},
+				RepositoryRoot: os.Getenv("SG_FORCE_REPO_ROOT"),
+			},
+			Docker: run.DockerOptions{
+				Image: "grafana:update",
+				Volumes: []run.DockerVolume{
+					{
+						From: "override-src",
+						To:   "dst",
+					},
+				},
+				Flags: map[string]string{"cpus": "1", "memory": "1g"},
+				Ports: []string{"3370",
+					"3371",
+				},
+				Linux: run.DockerLinuxOptions{
+					Flags: map[string]string{
+						"add-host": "host.docker.internal:host-gateway",
+						"user":     "root"},
+					Env: map[string]string{"FOO": "bar"}}},
 		},
-		Cmd:         ".bin/frontend",
-		Install:     "go build .bin/frontend github.com/sourcegraph/sourcegraph/cmd/frontend",
-		CheckBinary: ".bin/frontend",
-		Watch: []string{
-			"lib",
-			"internal",
-			"cmd/frontend",
 		},
 	}
 
-	if diff := cmp.Diff(cmd, want); diff != "" {
-		t.Fatalf("wrong cmd. (-want +got):\n%s", diff)
+	if diff := cmp.Diff(want, merged); diff != "" {
+		t.Fatalf("wrong config. (-want +got):\n%s", diff)
 	}
 }

dev/sg/internal/sgconf/global.go

@@ -84,7 +84,7 @@ func parseConf(confFile, overwriteFile string, noOverwrite bool) (*Config, error
 			if err != nil {
 				return nil, errors.Wrapf(err, "Failed to parse %q as configuration overwrite file", confFile)
 			}
-			conf.Merge(overwriteConf)
+			conf = conf.Merge(overwriteConf)
 		}
 	}
 

sg.config.yaml

@@ -337,8 +337,8 @@ commands:
       # Set for convenience - use real values in sg.config.overwrite.yaml if you
       # are interacting with RPCs that enforce SAMS M2M auth. See
       # https://github.com/sourcegraph/accounts.sourcegraph.com/wiki/Operators-Cheat-Sheet#create-a-new-idp-client
-      TELEMETRY_GATEWAY_SAMS_CLIENT_ID: "foo"
-      TELEMETRY_GATEWAY_SAMS_CLIENT_SECRET: "bar"
+      TELEMETRY_GATEWAY_SAMS_CLIENT_ID: 'foo'
+      TELEMETRY_GATEWAY_SAMS_CLIENT_SECRET: 'bar'
     watch:
       - lib
       - internal