From 1740b529a6da60c453381f97d6f12900b516c0bb Mon Sep 17 00:00:00 2001 From: Alan Guo Xiang Tan Date: Thu, 29 Aug 2024 01:02:49 +0800 Subject: [PATCH] DEV: Improve assertions of `cli_build_test.go` (#844) This commit improves the assertions by testing against the entire command string and env so that we can be sure of the full command we are running. --- launcher_go/v2/cli_build.go | 18 ++++- launcher_go/v2/cli_build_test.go | 87 ++++++++++++++++++++----- launcher_go/v2/config/config.go | 12 +++- launcher_go/v2/docker/commands.go | 46 +++++++++---- launcher_go/v2/test/containers/test.yml | 3 +- 5 files changed, 133 insertions(+), 33 deletions(-) diff --git a/launcher_go/v2/cli_build.go b/launcher_go/v2/cli_build.go index d35f8d2..8d0c17a 100644 --- a/launcher_go/v2/cli_build.go +++ b/launcher_go/v2/cli_build.go @@ -3,12 +3,14 @@ package main import ( "context" "errors" + "flag" + "os" + "strings" + "github.com/discourse/discourse_docker/launcher_go/v2/config" "github.com/discourse/discourse_docker/launcher_go/v2/docker" "github.com/discourse/discourse_docker/launcher_go/v2/utils" "github.com/google/uuid" - "os" - "strings" ) /* @@ -66,7 +68,16 @@ func (r *DockerConfigureCmd) Run(cli *Cli, ctx *context.Context) error { return errors.New("YAML syntax error. Please check your containers/*.yml config files.") } - containerId := "discourse-build-" + uuid.NewString() + var uuidString string + + if flag.Lookup("test.v") == nil { + uuidString = uuid.NewString() + } else { + uuidString = "test" + } + + containerId := "discourse-build-" + uuidString + pups := docker.DockerPupsRunner{ Config: config, PupsArgs: "--tags=db,precompile", @@ -75,6 +86,7 @@ func (r *DockerConfigureCmd) Run(cli *Cli, ctx *context.Context) error { Ctx: ctx, ContainerId: containerId, } + return pups.Run() } diff --git a/launcher_go/v2/cli_build_test.go b/launcher_go/v2/cli_build_test.go index 698228c..1d50318 100644 --- a/launcher_go/v2/cli_build_test.go +++ b/launcher_go/v2/cli_build_test.go @@ -59,33 +59,89 @@ var _ = Describe("Build", func() { } var checkConfigureCmd = func(cmd exec.Cmd) { - Expect(cmd.String()).To(ContainSubstring("docker run")) - Expect(cmd.String()).To(ContainSubstring("--env DISCOURSE_DEVELOPER_EMAILS")) - Expect(cmd.String()).To(ContainSubstring("--env SKIP_EMBER_CLI_COMPILE=1")) - // we commit, we need the container to stick around after it is stopped. - Expect(cmd.String()).ToNot(ContainSubstring("--rm")) - - // we don't expose ports on configure command - Expect(cmd.String()).ToNot(ContainSubstring("-p 80")) - Expect(cmd.Env).To(ContainElement("DISCOURSE_DB_PASSWORD=SOME_SECRET")) + Expect(cmd.String()).To(Equal( + "/usr/local/bin/docker run " + + "--env DISCOURSE_DB_HOST " + + "--env DISCOURSE_DB_PASSWORD " + + "--env DISCOURSE_DB_PORT " + + "--env DISCOURSE_DB_SOCKET " + + "--env DISCOURSE_DEVELOPER_EMAILS " + + "--env DISCOURSE_HOSTNAME " + + "--env DISCOURSE_REDIS_HOST " + + "--env DISCOURSE_SMTP_ADDRESS " + + "--env DISCOURSE_SMTP_PASSWORD " + + "--env DISCOURSE_SMTP_USER_NAME " + + "--env LANG " + + "--env LANGUAGE " + + "--env LC_ALL " + + "--env MULTI " + + "--env RAILS_ENV " + + "--env REPLACED " + + "--env RUBY_GC_HEAP_GROWTH_MAX_SLOTS " + + "--env RUBY_GC_HEAP_INIT_SLOTS " + + "--env RUBY_GC_HEAP_OLDOBJECT_LIMIT_FACTOR " + + "--env UNICORN_SIDEKIQS " + + "--env UNICORN_WORKERS " + + "--env SKIP_EMBER_CLI_COMPILE=1 " + + "--volume /var/discourse/shared/web-only:/shared " + + "--volume /var/discourse/shared/web-only/log/var-log:/var/log " + + "--link data:data " + + "--shm-size=512m " + + "--restart=no " + + "--interactive " + + "--expose 100 " + + "--name discourse-build-test " + + "local_discourse/test /bin/bash -c /usr/local/bin/pups --stdin --tags=db,precompile", + )) + + Expect(cmd.Env).To(Equal([]string{ + "DISCOURSE_DB_HOST=data", + "DISCOURSE_DB_PASSWORD=SOME_SECRET", + "DISCOURSE_DB_PORT=", + "DISCOURSE_DB_SOCKET=", + "DISCOURSE_DEVELOPER_EMAILS=me@example.com,you@example.com", + "DISCOURSE_HOSTNAME=discourse.example.com", + "DISCOURSE_REDIS_HOST=data", + "DISCOURSE_SMTP_ADDRESS=smtp.example.com", + "DISCOURSE_SMTP_PASSWORD=pa$$word", + "DISCOURSE_SMTP_USER_NAME=user@example.com", + "LANG=en_US.UTF-8", + "LANGUAGE=en_US.UTF-8", + "LC_ALL=en_US.UTF-8", + "MULTI=test\nmultiline with some spaces\nvar\n", + "RAILS_ENV=production", + "REPLACED=test/test/test", + "RUBY_GC_HEAP_GROWTH_MAX_SLOTS=40000", + "RUBY_GC_HEAP_INIT_SLOTS=400000", + "RUBY_GC_HEAP_OLDOBJECT_LIMIT_FACTOR=1.5", + "UNICORN_SIDEKIQS=1", + "UNICORN_WORKERS=3", + })) + buf := new(strings.Builder) io.Copy(buf, cmd.Stdin) // docker run's stdin is a pups config + + // web.template.yml is merged with the test config Expect(buf.String()).To(ContainSubstring("path: /etc/service/nginx/run")) + Expect(buf.String()).To(ContainSubstring(`exec: echo "custom test command"`)) } // commit on configure var checkConfigureCommit = func(cmd exec.Cmd) { - Expect(cmd.String()).To(ContainSubstring("docker commit")) - Expect(cmd.String()).To(ContainSubstring("--change CMD [\"/sbin/boot\"]")) - Expect(cmd.String()).To(ContainSubstring("discourse-build")) - Expect(cmd.String()).To(ContainSubstring("local_discourse/test")) - Expect(cmd.Env).ToNot(ContainElement("DISCOURSE_DB_PASSWORD=SOME_SECRET")) + Expect(cmd.String()).To(MatchRegexp( + "/usr/local/bin/docker commit " + + `--change LABEL org\.opencontainers\.image\.created="[\d\-T:+]+" ` + + `--change CMD \["/sbin/boot"\] ` + + "discourse-build-test local_discourse/test", + )) + + Expect(cmd.Env).To(BeNil()) } // configure also cleans up var checkConfigureClean = func(cmd exec.Cmd) { - Expect(cmd.String()).To(ContainSubstring("docker rm -f discourse-build-")) + Expect(cmd.String()).To(Equal("/usr/local/bin/docker rm --force discourse-build-test")) } It("Should run docker build with correct arguments", func() { @@ -99,6 +155,7 @@ var _ = Describe("Build", func() { runner := ddocker.DockerConfigureCmd{Config: "test"} runner.Run(cli, &ctx) Expect(len(RanCmds)).To(Equal(3)) + checkConfigureCmd(RanCmds[0]) checkConfigureCommit(RanCmds[1]) checkConfigureClean(RanCmds[2]) diff --git a/launcher_go/v2/config/config.go b/launcher_go/v2/config/config.go index c527b4b..3e74928 100644 --- a/launcher_go/v2/config/config.go +++ b/launcher_go/v2/config/config.go @@ -1,16 +1,17 @@ package config import ( - "dario.cat/mergo" "errors" "fmt" - "github.com/discourse/discourse_docker/launcher_go/v2/utils" "os" "regexp" "runtime" "slices" "strings" + "dario.cat/mergo" + "github.com/discourse/discourse_docker/launcher_go/v2/utils" + "gopkg.in/yaml.v3" ) @@ -77,7 +78,9 @@ func LoadConfig(dir string, configName string, includeTemplates bool, templatesD Boot_Command: defaultBootCommand, Base_Image: DefaultBaseImage(), } + matched, _ := regexp.MatchString("[[:upper:]/ !@#$%^&*()+~`=]", configName) + if matched { msg := "ERROR: Config name '" + configName + "' must not contain upper case characters, spaces or special characters. Correct config name and rerun." fmt.Println(msg) @@ -86,12 +89,14 @@ func LoadConfig(dir string, configName string, includeTemplates bool, templatesD config_filename := string(strings.TrimRight(dir, "/") + "/" + config.Name + ".yml") content, err := os.ReadFile(config_filename) + if err != nil { if os.IsNotExist(err) { fmt.Println("config file does not exist: " + config_filename) } return nil, err } + baseConfig := &Config{} if err := yaml.Unmarshal(content, baseConfig); err != nil { @@ -105,10 +110,13 @@ func LoadConfig(dir string, configName string, includeTemplates bool, templatesD } } } + if err := mergo.Merge(config, baseConfig, mergo.WithOverride); err != nil { return nil, err } + config.rawYaml = append(config.rawYaml, string(content[:])) + if err != nil { return nil, err } diff --git a/launcher_go/v2/docker/commands.go b/launcher_go/v2/docker/commands.go index e8835b0..c9636e4 100644 --- a/launcher_go/v2/docker/commands.go +++ b/launcher_go/v2/docker/commands.go @@ -7,6 +7,7 @@ import ( "os" "os/exec" "runtime" + "sort" "strings" "syscall" "time" @@ -94,19 +95,28 @@ func (r *DockerRunner) Run() error { } cmd.Env = r.Config.EnvArray(true) + envKeys := make([]string, 0, len(r.Config.Env)) + + for envKey := range r.Config.Env { + envKeys = append(envKeys, envKey) + } + + sort.Strings(envKeys) if r.DryRun { // multi-line env doesn't work super great from CLI, but we can print out the rest. - for k, v := range r.Config.Env { - if !strings.Contains(v, "\n") { + for _, envKey := range envKeys { + value := r.Config.Env[envKey] + + if !strings.Contains(value, "\n") { cmd.Args = append(cmd.Args, "--env") - cmd.Args = append(cmd.Args, k+"="+shellwords.Escape(v)) + cmd.Args = append(cmd.Args, envKey+"="+shellwords.Escape(value)) } } } else { - for k, _ := range r.Config.Env { + for _, envKey := range envKeys { cmd.Args = append(cmd.Args, "--env") - cmd.Args = append(cmd.Args, k) + cmd.Args = append(cmd.Args, envKey) } } @@ -156,10 +166,10 @@ func (r *DockerRunner) Run() error { } if r.Detatch { - cmd.Args = append(cmd.Args, "-d") + cmd.Args = append(cmd.Args, "--detach") } - cmd.Args = append(cmd.Args, "-i") + cmd.Args = append(cmd.Args, "--interactive") // Docker args override settings above for _, f := range r.Config.DockerArgs() { @@ -170,8 +180,11 @@ func (r *DockerRunner) Run() error { cmd.Args = append(cmd.Args, f) } - cmd.Args = append(cmd.Args, "-h") - cmd.Args = append(cmd.Args, r.Hostname) + if r.Hostname != "" { + cmd.Args = append(cmd.Args, "--hostname") + cmd.Args = append(cmd.Args, r.Hostname) + } + cmd.Args = append(cmd.Args, "--name") cmd.Args = append(cmd.Args, r.ContainerId) @@ -216,21 +229,26 @@ func (r *DockerPupsRunner) Run() error { rm := false // remove : in case docker tag is blank, and use default latest tag r.SavedImageName = strings.TrimRight(r.SavedImageName, ":") + if r.SavedImageName == "" { rm = true } + defer func(rm bool) { if !rm { time.Sleep(utils.CommitWait) runCtx, cancel := context.WithTimeout(context.Background(), 10*time.Second) - cmd := exec.CommandContext(runCtx, utils.DockerPath, "rm", "-f", r.ContainerId) + cmd := exec.CommandContext(runCtx, utils.DockerPath, "rm", "--force", r.ContainerId) utils.CmdRunner(cmd).Run() cancel() } }(rm) - commands := []string{"/bin/bash", + + commands := []string{ + "/bin/bash", "-c", - "/usr/local/bin/pups --stdin " + r.PupsArgs} + "/usr/local/bin/pups --stdin " + r.PupsArgs, + } runner := DockerRunner{Config: r.Config, Ctx: r.Ctx, @@ -248,6 +266,7 @@ func (r *DockerPupsRunner) Run() error { if len(r.SavedImageName) > 0 { time.Sleep(utils.CommitWait) + cmd := exec.Command("docker", "commit", "--change", @@ -257,13 +276,16 @@ func (r *DockerPupsRunner) Run() error { r.ContainerId, r.SavedImageName, ) + cmd.Stdout = os.Stdout cmd.Stderr = os.Stderr fmt.Fprintln(utils.Out, cmd) + if err := utils.CmdRunner(cmd).Run(); err != nil { return err } } + return nil } diff --git a/launcher_go/v2/test/containers/test.yml b/launcher_go/v2/test/containers/test.yml index ef3b706..3737d9a 100644 --- a/launcher_go/v2/test/containers/test.yml +++ b/launcher_go/v2/test/containers/test.yml @@ -81,7 +81,7 @@ env: ## The http or https CDN address for this Discourse instance (configured to pull) ## see https://meta.discourse.org/t/14857 for details #DISCOURSE_CDN_URL: https://discourse-cdn.example.com - + ## The maxmind geolocation IP address key for IP address lookup ## see https://meta.discourse.org/t/-/137387/23 for details #DISCOURSE_MAXMIND_LICENSE_KEY: 1234567890123456 @@ -105,6 +105,7 @@ hooks: ## Remember, this is YAML syntax - you can only have one block with a name run: + - exec: echo "custom test command" - exec: echo "Beginning of custom commands" ## If you want to configure password login for root, uncomment and change: -- 2.25.1