Skip to content

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

Open
wants to merge 22 commits into
base: main
Choose a base branch
from

Conversation

featheredtoast
Copy link
Member

Append extra env in docker build/run calls to the current process' environment, rather than replace it wholesale.

Copy link

@saj saj left a 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)

@@ -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"
Copy link

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.

env := r.Config.EnvArray(false)
for _, e := range env {
cmd.Env = append(cmd.Env, e)
}
Copy link

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)...)

env := r.Config.EnvArray(true)
for _, e := range env {
cmd.Env = append(cmd.Env, e)
}
Copy link

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")
Copy link

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.

https://go.dev/wiki/CodeReviewComments#error-strings

v2/cli_build.go Outdated
@@ -172,7 +174,7 @@ type CleanCmd struct {

func (r *CleanCmd) Run(cli *Cli) error {
dir := cli.BuildDir + "/" + r.Config
Copy link

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)
Copy link

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
Copy link

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"`
Copy link

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
Copy link

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
Copy link

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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants