-
Notifications
You must be signed in to change notification settings - Fork 1
FIX: append os.Environ to docker build and docker run calls #18
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
since temp dirs are being dynamically created, no need to set subdirectories
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Surface level language readability review only. I am not maintainer.
Sorry, I started reviewing commit by commit, then realised some of my earlier comments were invalidated by later commits. GitHub make it very hard for me to revise -- or even see -- my old comments, so please disregard any that you have already addressed. (As you can probably tell, I don't usually conduct reviews in GitHub.)
Some commit messages explain what but they do not explain why.
append os.Environ to docker build and docker run calls
Why? Imagine there is a bug in the patch. Someone else finds the bug -- maybe a long time from now -- and begins to question your work. Use this space to help them understand what was going through your mind.
inherit process environment on docker exec
docker must typically consult PATH
when configured with a credential helper.
PATH was previously removed by mistake.
(Along with the rest of the existing environment.)
Infra generally frown on one-line commit messages and we do not ever squash. I will leave you to find your own way.
Don’t add a Context member to a struct type
https://go.dev/wiki/CodeReviewComments#contexts
Pass Context
by value.
Convention dictates that it is always the first function argument.
yes:
func Foo(ctx context.Context, n int)
no:
func Foo(ctx *context.Context)
no:
func Foo(n int, ctx context.Context)
v2/docker/commands_test.go
Outdated
@@ -10,6 +10,7 @@ import ( | |||
"github.com/discourse/launcher/v2/docker" | |||
. "github.com/discourse/launcher/v2/test_utils" | |||
"github.com/discourse/launcher/v2/utils" | |||
"os" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Imports are organized in groups, with blank lines between them. The standard library packages are always in the first group.
goimports will do this for you.
https://go.dev/wiki/CodeReviewComments#imports
I usually prefer to see local imports grouped together and listed last, but I won't hold you to this additional constraint if you would prefer to do otherwise. (stdlib imports must, however, be grouped as noted above.)
import (
"bytes"
"context"
"os"
"strings"
. "github.com/onsi/ginkgo/v2"
. "github.com/onsi/gomega"
"github.com/discourse/launcher/v2/config"
"github.com/discourse/launcher/v2/docker"
. "github.com/discourse/launcher/v2/test_utils"
"github.com/discourse/launcher/v2/utils"
)
Almost all source files are affected.
v2/docker/commands.go
Outdated
env := r.Config.EnvArray(false) | ||
for _, e := range env { | ||
cmd.Env = append(cmd.Env, e) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
append
can take a slice source argument.
cmd.Env = append(os.Environ(), r.Config.EnvArray(false)...)
v2/docker/commands.go
Outdated
env := r.Config.EnvArray(true) | ||
for _, e := range env { | ||
cmd.Env = append(cmd.Env, e) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
append
can take a slice source argument.
cmd.Env = append(os.Environ(), r.Config.EnvArray(false)...)
@@ -29,7 +29,7 @@ type DockerBuildCmd struct { | |||
func (r *DockerBuildCmd) Run(cli *Cli, ctx *context.Context) error { | |||
config, err := config.LoadConfig(cli.ConfDir, r.Config, true, cli.TemplatesDir) | |||
if err != nil { | |||
return errors.New("YAML syntax error. Please check your containers/*.yml config files.") | |||
return errors.New("YAML syntax error. Please check your containers/*.yml config files") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you are going to use sentences, it would make sense to end with punctuation.
Was this diff hunk intentional? The commit message says 'apply linting' but I don't know of any linter that would modify a string. Maybe the linter complained about the trailing full stop. If that is the case, you are only adhering to the letter of the law -- not the spirit.
This would be better:
syntax error: blah.yaml: some specific error message from the underlying decoder
As a user, how can I rectify the problem if I don't know what is broken? Please don't gobble error messages. We have lost entire days to developers who gobble important error messages.
The Gorbists typically style their error messages as shown in the quote above. Note that there are no sentences. (There are no capital letters and there is no full stop.) They do this because it is easier for callers to annotate the error message with additional context.
if err := flib(); err != nil { // flib encountered a problem
return fmt.Errorf("universe %s has exploded: %v", uni, err)
}
The Gorbist might say this provides satisfactory information without the clutter of a full stack trace.
v2/cli_build.go
Outdated
@@ -172,7 +174,7 @@ type CleanCmd struct { | |||
|
|||
func (r *CleanCmd) Run(cli *Cli) error { | |||
dir := cli.BuildDir + "/" + r.Config |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not all operating systems use /
as a directory separator. When trying to write portable software, it is better to use filepath.Join
, but -- as this software is probably only ever expected to run on Linux -- I guess this is fine. You may nevertheless want to get into this habit.
@@ -249,7 +240,9 @@ func (r *DockerPupsRunner) Run() error { | |||
time.Sleep(utils.CommitWait) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have no idea what is going on here but inserting sleeps is usually a sign that something is buggy. There should be hard points on which you can synchronise -- like a successful return from wait(2)
(child process exit) -- or other such event. This is never an acceptable workaround.
(also below)
v2/cli_build.go
Outdated
@@ -32,6 +32,9 @@ func (r *DockerBuildCmd) Run(cli *Cli, ctx *context.Context) error { | |||
return errors.New("YAML syntax error. Please check your containers/*.yml config files") | |||
} | |||
|
|||
if cli.BuildDir == "" { | |||
cli.BuildDir, _ = os.MkdirTemp("", "launcher") //nolint:errcheck |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do not discard this error -- or errors from similar system calls.
I have seen everything fail. Even things you would not expect to ever fail.
If this system call fails, the program will probably violate the behavioural invariants you have in your mind, and it will do so silently. Now put yourself in the shoes of a user and imagine their confusion.
v2/main.go
Outdated
@@ -17,7 +17,7 @@ type Cli struct { | |||
Version kong.VersionFlag `help:"Show version."` | |||
ConfDir string `default:"./containers" hidden:"" help:"Discourse pups config directory." predictor:"dir"` | |||
TemplatesDir string `default:"." hidden:"" help:"Home project directory containing a templates/ directory which in turn contains pups yaml templates." predictor:"dir"` | |||
BuildDir string `default:"./tmp" hidden:"" help:"Temporary build folder for building images." predictor:"dir"` | |||
BuildDir string `default:"" hidden:"" help:"Temporary build folder for building images." predictor:"dir"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: replace 'folder' with 'directory'
There is no such thing as a folder on your target platform. The flag is called --build-dir
. The documentation on the lines immediately preceding use the word directory.
v2/cli_build.go
Outdated
@@ -36,9 +36,6 @@ func (r *DockerBuildCmd) Run(cli *Cli, ctx *context.Context) error { | |||
if dir == "" { | |||
dir, _ = os.MkdirTemp("", "launcher") //nolint:errcheck |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do not discard this error.
v2/cli_build.go
Outdated
@@ -59,6 +59,7 @@ func (r *DockerBuildCmd) Run(cli *Cli, ctx *context.Context) error { | |||
if err := builder.Run(); err != nil { | |||
return err | |||
} | |||
os.RemoveAll(cli.BuildDir) //nolint:errcheck |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens if the program hits a return
before it reaches this statement?
Be more loud about errors when creating temp directory. Ensure temp directory is cleaned up regardless of error.
See also https://go.dev/wiki/CodeReviewComments#contexts Do not hold context in structs. As best effort as I can get, I'm attempting to pass context into Run functions called by kong. It seems like Kong Client is always first value in the binding after a Bind() or BindTo() is called. The method signature to read as Run(context.Context, *Cli) is difficult to set up properly, as such I have left the context as the second param for kong commands. See also: alecthomas/kong#48 alecthomas/kong#144
Per Saj's feedback: It would be more efficient to restructure the program so that it need not buffer all of the output. Let the operating system do all of the work. This is especially true for large amounts of data, as may be the case here. The io.Reader and io.Writer interfaces in Go are quite powerful, and should form the basis of your internal I/O design. Conversions of string → []byte (and vice versa) require a memory allocation. Allocations are sometimes unavoidable, but they generate garbage. The garbage must be collected, later. In real programs, it is usually better to avoid unnecessary allocations and alleviate pressure on the garbage collector. If you already have []byte slice, use the []byte slice as-is. The fmt package is useful when you truly need a formatted string. In all other cases, all it does is make your program slower. (fmt can only work with run-time reflection.)
up, down, rm are natural to be in parallel with docker commands
Append extra env in docker build/run calls to the current process' environment, rather than replace it wholesale.