From 3400ed739f75706c180cf7b923df5f67553c6a3d Mon Sep 17 00:00:00 2001 From: James McNamara Date: Mon, 25 Mar 2024 09:50:49 -0700 Subject: [PATCH] [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 * Update dev/sg/internal/sgconf/config.go Co-authored-by: Jean-Hadrien Chabran * addressed comments --------- Co-authored-by: Jean-Hadrien Chabran --- dev/sg/internal/run/bazel_command.go | 13 ++ dev/sg/internal/run/command.go | 47 ++++-- dev/sg/internal/run/docker_commmand.go | 32 ++++ .../internal/run/sgconfig_command_options.go | 55 ++----- dev/sg/internal/sgconf/BUILD.bazel | 1 + dev/sg/internal/sgconf/config.go | 77 +++++----- dev/sg/internal/sgconf/config_test.go | 143 +++++++++++++++--- dev/sg/internal/sgconf/global.go | 2 +- sg.config.yaml | 4 +- 9 files changed, 254 insertions(+), 120 deletions(-) diff --git a/dev/sg/internal/run/bazel_command.go b/dev/sg/internal/run/bazel_command.go index 99cde9b0541..1f067d22970 100644 --- a/dev/sg/internal/run/bazel_command.go +++ b/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. diff --git a/dev/sg/internal/run/command.go b/dev/sg/internal/run/command.go index dafd83ad31f..4e5d1d48ab4 100644 --- a/dev/sg/internal/run/command.go +++ b/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 diff --git a/dev/sg/internal/run/docker_commmand.go b/dev/sg/internal/run/docker_commmand.go index f9016e9e117..020a3289368 100644 --- a/dev/sg/internal/run/docker_commmand.go +++ b/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 diff --git a/dev/sg/internal/run/sgconfig_command_options.go b/dev/sg/internal/run/sgconfig_command_options.go index 91415ac85f4..c8a24cc410e 100644 --- a/dev/sg/internal/run/sgconfig_command_options.go +++ b/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 } diff --git a/dev/sg/internal/sgconf/BUILD.bazel b/dev/sg/internal/sgconf/BUILD.bazel index 47736c0dfc0..66c891197d3 100644 --- a/dev/sg/internal/sgconf/BUILD.bazel +++ b/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", ], ) diff --git a/dev/sg/internal/sgconf/config.go b/dev/sg/internal/sgconf/config.go index 31a49609201..fc835491753 100644 --- a/dev/sg/internal/sgconf/config.go +++ b/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 { diff --git a/dev/sg/internal/sgconf/config_test.go b/dev/sg/internal/sgconf/config_test.go index b2377a29a88..e1d7d24e0e1 100644 --- a/dev/sg/internal/sgconf/config_test.go +++ b/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) } } diff --git a/dev/sg/internal/sgconf/global.go b/dev/sg/internal/sgconf/global.go index 314f5682553..7f58f5e7735 100644 --- a/dev/sg/internal/sgconf/global.go +++ b/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) } } diff --git a/sg.config.yaml b/sg.config.yaml index 075df1983c3..178b11a983d 100644 --- a/sg.config.yaml +++ b/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