Refactor global values to be defaults

There was recent work to add global values for `env`, `flags`, and `ldflags`. The global values would be merged with per-build values to generate the value used for the builds.

There are a couple issues with this:

- It's inconsistent with the existing code, which only has `default` values declared globally (there is no merging today).
- The name of the `flag` variable, caused a conflict with knative's `KO_FLAGS` environment variable (see #1317)

This PR does the following:

- Refactors the logic to use `defaultEnv`, `defaultFlags`, and `defaultLdflags`. This resolves both issues described above.
- Updates documentation

Fixes #1317

docs/configuration.md

@@ -125,18 +125,18 @@ KO_DEFAULTPLATFORMS=linux/arm64,linux/amd64
 ### Setting build environment variables
 
 By default, `ko` builds use the ambient environment from the system (i.e. `os.Environ()`).
-These values can be overridden globally or per-build (or both).
+These values can be overridden for your build.
 
 ```yaml
-env:
+defaultEnv:
 - FOO=foo
 builds:
 - id: foo
   dir: .
   main: ./foobar/foo
   env:
-  - FOO=bar # Overrides the global value.
-- id: bar
+  - FOO=bar
+- id: bar     # Will use defaultEnv.
   dir: ./bar
   main: .
 ```
@@ -144,33 +144,32 @@ builds:
 For a given build, the environment variables are merged in the following order:
 
 - System `os.Environ` (lowest precedence)
-- Global `env`
-- Build `env` (highest precedence)
+- Build variables: `build.env` if specified, otherwise `defaultEnv` (highest precedence)
 
 ### Setting build flags and ldflags
 
 You can specify both `flags` and `ldflags` globally as well as per-build.
 
 ```yaml
-flags:
+defaultFlags:
 - -v
-ldflags:
+defaultLdflags:
 - -s
 builds:
 - id: foo
   dir: .
   main: ./foobar/foo
   flags:
-  - -trimpath # Build will use: -v -trimpath
+  - -trimpath
   ldflags:
-  - -w # Build will use: -s -w
-- id: bar
+  - -w
+- id: bar     # Will use defaultFlags and defaultLdflags.
   dir: ./bar
   main: .
 ```
 
-The values for each `build` will be appended to the global values when creating each build.
-Both global and per-build values may use [template parameters](#templating-support).
+The values for a `build` will be used if specified, otherwise their respective defaults will be used.
+Both default and per-build values may use [template parameters](#templating-support).
 
 ### Environment Variables (advanced)
 

integration_test.sh

@@ -43,7 +43,15 @@ else
   echo "Test PASSED"
 fi
 
-echo "2. Linux capabilities."
+echo "2. Test knative 'KO_FLAGS' variable is ignored."
+RESULT="$(KO_FLAGS="--platform=badvalue" ./ko build --local --platform="linux/$GOARCH" "$GOPATH/src/github.com/google/ko/test" | grep "$FILTER" | xargs -I% docker run %)"
+if [[ "$RESULT" != *"Hello there"* ]]; then
+  echo "Test FAILED. Saw $RESULT" && exit 1
+else
+  echo "Test PASSED"
+fi
+
+echo "3. Linux capabilities."
 pushd test/build-configs || exit 1
 # run as non-root user with net_bind_service cap granted
 docker_run_opts="--user 1 --cap-add=net_bind_service"

pkg/build/gobuild.go

@@ -66,13 +66,13 @@ type GetBase func(context.Context, string) (name.Reference, Result, error)
 
 // buildContext provides parameters for a builder function.
 type buildContext struct {
-	creationTime  v1.Time
-	ip            string
-	dir           string
-	mergedEnv     []string
-	mergedFlags   []string
-	mergedLdflags []string
-	platform      v1.Platform
+	creationTime v1.Time
+	ip           string
+	dir          string
+	env          []string
+	flags        []string
+	ldflags      []string
+	platform     v1.Platform
 }
 
 type builder func(context.Context, buildContext) (string, error)
@@ -95,9 +95,9 @@ type gobuild struct {
 	disableOptimizations bool
 	trimpath             bool
 	buildConfigs         map[string]Config
-	env                  []string
-	flags                []string
-	ldflags              []string
+	defaultEnv           []string
+	defaultFlags         []string
+	defaultLdflags       []string
 	platformMatcher      *platformMatcher
 	dir                  string
 	labels               map[string]string
@@ -120,9 +120,9 @@ type gobuildOpener struct {
 	disableOptimizations bool
 	trimpath             bool
 	buildConfigs         map[string]Config
-	env                  []string
-	flags                []string
-	ldflags              []string
+	defaultEnv           []string
+	defaultFlags         []string
+	defaultLdflags       []string
 	platforms            []string
 	labels               map[string]string
 	dir                  string
@@ -151,9 +151,9 @@ func (gbo *gobuildOpener) Open() (Interface, error) {
 		disableOptimizations: gbo.disableOptimizations,
 		trimpath:             gbo.trimpath,
 		buildConfigs:         gbo.buildConfigs,
-		env:                  gbo.env,
-		flags:                gbo.flags,
-		ldflags:              gbo.ldflags,
+		defaultEnv:           gbo.defaultEnv,
+		defaultFlags:         gbo.defaultFlags,
+		defaultLdflags:       gbo.defaultLdflags,
 		labels:               gbo.labels,
 		dir:                  gbo.dir,
 		platformMatcher:      matcher,
@@ -320,7 +320,7 @@ func build(ctx context.Context, buildCtx buildContext) (string, error) {
 	gobin := getGoBinary()
 	cmd := exec.CommandContext(ctx, gobin, args...)
 	cmd.Dir = buildCtx.dir
-	cmd.Env = buildCtx.mergedEnv
+	cmd.Env = buildCtx.env
 
 	var output bytes.Buffer
 	cmd.Stderr = &output
@@ -493,9 +493,9 @@ func cycloneDX() sbomber {
 }
 
 // buildEnv creates the environment variables used by the `go build` command.
-// From `os/exec.Cmd`: If Env contains duplicate environment keys, only the last
+// From `os/exec.Cmd`: If there are duplicate environment keys, only the last
 // value in the slice for each duplicate key is used.
-func buildEnv(platform v1.Platform, osEnv, globalEnv, configEnv []string) ([]string, error) {
+func buildEnv(platform v1.Platform, osEnv, buildEnv []string) ([]string, error) {
 	// Default env
 	env := []string{
 		"CGO_ENABLED=0",
@@ -520,8 +520,7 @@ func buildEnv(platform v1.Platform, osEnv, globalEnv, configEnv []string) ([]str
 	}
 
 	env = append(env, osEnv...)
-	env = append(env, globalEnv...)
-	env = append(env, configEnv...)
+	env = append(env, buildEnv...)
 	return env, nil
 }
 
@@ -768,7 +767,7 @@ func createTemplateData(ctx context.Context, buildCtx buildContext) (map[string]
 	envVars := map[string]string{
 		"LDFLAGS": "",
 	}
-	for _, entry := range buildCtx.mergedEnv {
+	for _, entry := range buildCtx.env {
 		kv := strings.SplitN(entry, "=", 2)
 		if len(kv) != 2 {
 			return nil, fmt.Errorf("invalid environment variable entry: %q", entry)
@@ -837,8 +836,8 @@ func createBuildArgs(ctx context.Context, buildCtx buildContext) ([]string, erro
 		return nil, err
 	}
 
-	if len(buildCtx.mergedFlags) > 0 {
-		flags, err := applyTemplating(buildCtx.mergedFlags, data)
+	if len(buildCtx.flags) > 0 {
+		flags, err := applyTemplating(buildCtx.flags, data)
 		if err != nil {
 			return nil, err
 		}
@@ -846,8 +845,8 @@ func createBuildArgs(ctx context.Context, buildCtx buildContext) ([]string, erro
 		args = append(args, flags...)
 	}
 
-	if len(buildCtx.mergedLdflags) > 0 {
-		ldflags, err := applyTemplating(buildCtx.mergedLdflags, data)
+	if len(buildCtx.ldflags) > 0 {
+		ldflags, err := applyTemplating(buildCtx.ldflags, data)
 		if err != nil {
 			return nil, err
 		}
@@ -928,31 +927,40 @@ func (g *gobuild) buildOne(ctx context.Context, refStr string, base v1.Image, pl
 
 	config := g.configForImportPath(ref.Path())
 
-	// Merge the system, global, and build config environment variables.
-	mergedEnv, err := buildEnv(*platform, os.Environ(), g.env, config.Env)
+	// Merge the system and build environment variables.
+	env := config.Env
+	if len(env) == 0 {
+		// Use the default, if any.
+		env = g.defaultEnv
+	}
+	env, err = buildEnv(*platform, os.Environ(), env)
 	if err != nil {
 		return nil, fmt.Errorf("could not create env for %s: %w", ref.Path(), err)
 	}
 
-	// Merge global and build config flags.
-	var mergedFlags []string
-	mergedFlags = append(mergedFlags, g.flags...)
-	mergedFlags = append(mergedFlags, config.Flags...)
+	// Get the build flags.
+	flags := config.Flags
+	if len(flags) == 0 {
+		// Use the default, if any.
+		flags = g.defaultFlags
+	}
 
-	// Merge global and build config ldflags.
-	var mergedLdflags []string
-	mergedLdflags = append(mergedLdflags, g.ldflags...)
-	mergedLdflags = append(mergedLdflags, config.Ldflags...)
+	// Get the build ldflags.
+	ldflags := config.Ldflags
+	if len(ldflags) == 0 {
+		// Use the default, if any
+		ldflags = g.defaultLdflags
+	}
 
 	// Do the build into a temporary file.
 	file, err := g.build(ctx, buildContext{
-		creationTime:  g.creationTime,
-		ip:            ref.Path(),
-		dir:           g.dir,
-		mergedEnv:     mergedEnv,
-		mergedFlags:   mergedFlags,
-		mergedLdflags: mergedLdflags,
-		platform:      *platform,
+		creationTime: g.creationTime,
+		ip:           ref.Path(),
+		dir:          g.dir,
+		env:          env,
+		flags:        flags,
+		ldflags:      ldflags,
+		platform:     *platform,
 	})
 	if err != nil {
 		return nil, fmt.Errorf("build: %w", err)

pkg/build/gobuild_test.go

@@ -220,8 +220,7 @@ func TestBuildEnv(t *testing.T) {
 		description  string
 		platform     v1.Platform
 		osEnv        []string
-		globalEnv    []string
-		configEnv    []string
+		buildEnv     []string
 		expectedEnvs map[string]string
 	}{{
 		description: "defaults",
@@ -235,30 +234,16 @@ func TestBuildEnv(t *testing.T) {
 			"CGO_ENABLED": "0",
 		},
 	}, {
-		description: "override a default value",
+		description: "build override a system value",
 		osEnv:       []string{"CGO_ENABLED=0"},
-		configEnv:   []string{"CGO_ENABLED=1"},
-		expectedEnvs: map[string]string{
-			"CGO_ENABLED": "1",
-		},
-	}, {
-		description: "global override a default value",
-		osEnv:       []string{"CGO_ENABLED=0"},
-		globalEnv:   []string{"CGO_ENABLED=1"},
-		expectedEnvs: map[string]string{
-			"CGO_ENABLED": "1",
-		},
-	}, {
-		description: "override a global value",
-		globalEnv:   []string{"CGO_ENABLED=0"},
-		configEnv:   []string{"CGO_ENABLED=1"},
+		buildEnv:    []string{"CGO_ENABLED=1"},
 		expectedEnvs: map[string]string{
 			"CGO_ENABLED": "1",
 		},
 	}, {
 		description: "override an envvar and add an envvar",
 		osEnv:       []string{"CGO_ENABLED=0"},
-		configEnv:   []string{"CGO_ENABLED=1", "GOPRIVATE=git.internal.example.com,source.developers.google.com"},
+		buildEnv:    []string{"CGO_ENABLED=1", "GOPRIVATE=git.internal.example.com,source.developers.google.com"},
 		expectedEnvs: map[string]string{
 			"CGO_ENABLED": "1",
 			"GOPRIVATE":   "git.internal.example.com,source.developers.google.com",
@@ -297,7 +282,7 @@ func TestBuildEnv(t *testing.T) {
 	}}
 	for _, test := range tests {
 		t.Run(test.description, func(t *testing.T) {
-			env, err := buildEnv(test.platform, test.osEnv, test.globalEnv, test.configEnv)
+			env, err := buildEnv(test.platform, test.osEnv, test.buildEnv)
 			if err != nil {
 				t.Fatalf("unexpected error running buildEnv(): %v", err)
 			}
@@ -400,8 +385,8 @@ func TestCreateTemplateData(t *testing.T) {
 
 	t.Run("env", func(t *testing.T) {
 		params, err := createTemplateData(context.TODO(), buildContext{
-			dir:       t.TempDir(),
-			mergedEnv: []string{"FOO=bar"},
+			dir: t.TempDir(),
+			env: []string{"FOO=bar"},
 		})
 		require.NoError(t, err)
 		vars := params["Env"].(map[string]string)
@@ -410,8 +395,8 @@ func TestCreateTemplateData(t *testing.T) {
 
 	t.Run("bad env", func(t *testing.T) {
 		_, err := createTemplateData(context.TODO(), buildContext{
-			dir:       t.TempDir(),
-			mergedEnv: []string{"bad var"},
+			dir: t.TempDir(),
+			env: []string{"bad var"},
 		})
 		require.Error(t, err)
 	})
@@ -427,7 +412,7 @@ func TestCreateTemplateData(t *testing.T) {
 	t.Run("env overrides go env", func(t *testing.T) {
 		params, err := createTemplateData(context.TODO(), buildContext{
 			dir: t.TempDir(),
-			mergedEnv: []string{
+			env: []string{
 				"GOOS=testgoos",
 				"GOARCH=testgoarch",
 			},
@@ -915,7 +900,7 @@ func TestGoBuild(t *testing.T) {
 	})
 }
 
-func TestGoBuildMergedValues(t *testing.T) {
+func TestGoBuild_Defaults(t *testing.T) {
 	baseLayers := int64(3)
 	base, err := random.Image(1024, baseLayers)
 	if err != nil {
@@ -936,9 +921,54 @@ func TestGoBuildMergedValues(t *testing.T) {
 		}),
 		withSBOMber(fauxSBOM),
 		WithPlatforms("all"),
-		WithEnv([]string{"FOO=foo", "BAR=bar"}),
-		WithFlags([]string{"-v"}),
-		WithLdflags([]string{"-s"}),
+		WithDefaultEnv([]string{"FOO=foo", "BAR=bar"}),
+		WithDefaultFlags([]string{"-v"}),
+		WithDefaultLdflags([]string{"-s"}),
+		WithConfig(map[string]Config{
+			"github.com/google/ko/test": {},
+		}),
+	)
+	require.NoError(t, err)
+
+	// Build and capture the buildContext.
+	_, err = ng.Build(context.Background(), StrictScheme+filepath.Join(importpath, "test"))
+	require.ErrorContains(t, err, "fake build error")
+	require.Equal(t, []string{"-v"}, buildCtx.flags)
+	require.Equal(t, []string{"-s"}, buildCtx.ldflags)
+
+	envVars := make(map[string]string)
+	for _, val := range buildCtx.env {
+		kv := strings.SplitN(val, "=", 2)
+		envVars[kv[0]] = kv[1]
+	}
+	require.Equal(t, "foo", envVars["FOO"])
+	require.Equal(t, "bar", envVars["BAR"])
+}
+
+func TestGoBuild_ConfigOverrideDefaults(t *testing.T) {
+	baseLayers := int64(3)
+	base, err := random.Image(1024, baseLayers)
+	if err != nil {
+		t.Fatalf("random.Image() = %v", err)
+	}
+	importpath := "github.com/google/ko"
+
+	creationTime := v1.Time{Time: time.Unix(5000, 0)}
+	var buildCtx buildContext
+	ng, err := NewGo(
+		context.Background(),
+		"",
+		WithCreationTime(creationTime),
+		WithBaseImages(func(context.Context, string) (name.Reference, Result, error) { return baseRef, base, nil }),
+		withBuilder(func(_ context.Context, b buildContext) (string, error) {
+			buildCtx = b
+			return "", errors.New("fake build error")
+		}),
+		withSBOMber(fauxSBOM),
+		WithPlatforms("all"),
+		WithDefaultEnv([]string{"FOO=foo", "BAR=bar"}),
+		WithDefaultFlags([]string{"-v"}),
+		WithDefaultLdflags([]string{"-s"}),
 		WithConfig(map[string]Config{
 			"github.com/google/ko/test": {
 				Env:     StringArray{"FOO=baz"},
@@ -952,16 +982,16 @@ func TestGoBuildMergedValues(t *testing.T) {
 	// Build and capture the buildContext.
 	_, err = ng.Build(context.Background(), StrictScheme+filepath.Join(importpath, "test"))
 	require.ErrorContains(t, err, "fake build error")
-	require.Equal(t, []string{"-v", "-trimpath"}, buildCtx.mergedFlags)
-	require.Equal(t, []string{"-s", "-w"}, buildCtx.mergedLdflags)
+	require.Equal(t, []string{"-trimpath"}, buildCtx.flags)
+	require.Equal(t, []string{"-w"}, buildCtx.ldflags)
 
 	envVars := make(map[string]string)
-	for _, val := range buildCtx.mergedEnv {
+	for _, val := range buildCtx.env {
 		kv := strings.SplitN(val, "=", 2)
 		envVars[kv[0]] = kv[1]
 	}
 	require.Equal(t, "baz", envVars["FOO"])
-	require.Equal(t, "bar", envVars["BAR"])
+	require.Equal(t, "", envVars["BAR"])
 }
 
 func TestGoBuildWithKOCACHE(t *testing.T) {

pkg/build/options.go

@@ -85,27 +85,27 @@ func WithConfig(buildConfigs map[string]Config) Option {
 	}
 }
 
-// WithEnv is a functional option for providing a global set of environment
+// WithDefaultEnv is a functional option for providing a default set of environment
 // variables across all builds.
-func WithEnv(env []string) Option {
+func WithDefaultEnv(env []string) Option {
 	return func(gbo *gobuildOpener) error {
-		gbo.env = env
+		gbo.defaultEnv = env
 		return nil
 	}
 }
 
-// WithFlags is a functional option for providing a global set of flags across all builds.
-func WithFlags(flags []string) Option {
+// WithDefaultFlags is a functional option for providing a default set of flags across all builds.
+func WithDefaultFlags(flags []string) Option {
 	return func(gbo *gobuildOpener) error {
-		gbo.flags = flags
+		gbo.defaultFlags = flags
 		return nil
 	}
 }
 
-// WithLdflags is a functional option for providing a global set of ldflags across all builds.
-func WithLdflags(ldflags []string) Option {
+// WithDefaultLdflags is a functional option for providing a default set of ldflags across all builds.
+func WithDefaultLdflags(ldflags []string) Option {
 	return func(gbo *gobuildOpener) error {
-		gbo.ldflags = ldflags
+		gbo.defaultLdflags = ldflags
 		return nil
 	}
 }

pkg/commands/options/build.go

@@ -22,12 +22,11 @@ import (
 	"reflect"
 
 	"github.com/google/go-containerregistry/pkg/name"
+	"github.com/google/ko/pkg/build"
 	"github.com/mitchellh/mapstructure"
 	"github.com/spf13/cobra"
 	"github.com/spf13/viper"
 	"golang.org/x/tools/go/packages"
-
-	"github.com/google/ko/pkg/build"
 )
 
 const (
@@ -47,14 +46,14 @@ type BuildOptions struct {
 	// DefaultPlatforms defines the default platforms when Platforms is not explicitly defined
 	DefaultPlatforms []string
 
-	// Env allows setting environment variables globally and applying them to each build.
-	Env []string
+	// DefaultEnv defines the default environment when per-build value is not explicitly defined.
+	DefaultEnv []string
 
-	// Flags allows setting flags globally and applying them to each build.
-	Flags []string
+	// DefaultFlags defines the default flags when per-build value is not explicitly defined.
+	DefaultFlags []string
 
-	// Ldflags allows setting ldflags globally and applying them to each build.
-	Ldflags []string
+	// DefaultLdflags defines the default ldflags when per-build value is not explicitly defined.
+	DefaultLdflags []string
 
 	// WorkingDirectory allows for setting the working directory for invocations of the `go` tool.
 	// Empty string means the current working directory.
@@ -146,16 +145,16 @@ func (bo *BuildOptions) LoadConfig() error {
 		bo.DefaultPlatforms = dp
 	}
 
-	if env := v.GetStringSlice("env"); len(env) > 0 {
-		bo.Env = env
+	if env := v.GetStringSlice("defaultEnv"); len(env) > 0 {
+		bo.DefaultEnv = env
 	}
 
-	if flags := v.GetStringSlice("flags"); len(flags) > 0 {
-		bo.Flags = flags
+	if flags := v.GetStringSlice("defaultFlags"); len(flags) > 0 {
+		bo.DefaultFlags = flags
 	}
 
-	if ldflags := v.GetStringSlice("ldflags"); len(ldflags) > 0 {
-		bo.Ldflags = ldflags
+	if ldflags := v.GetStringSlice("defaultLdflags"); len(ldflags) > 0 {
+		bo.DefaultLdflags = ldflags
 	}
 
 	if bo.BaseImage == "" {

pkg/commands/options/build_test.go

@@ -68,31 +68,31 @@ func TestDefaultPlatformsAll(t *testing.T) {
 	}
 }
 
-func TestEnv(t *testing.T) {
+func TestDefaultEnv(t *testing.T) {
 	bo := &BuildOptions{
 		WorkingDirectory: "testdata/config",
 	}
 	err := bo.LoadConfig()
 	require.NoError(t, err)
-	require.Equal(t, []string{"FOO=bar"}, bo.Env)
+	require.Equal(t, []string{"FOO=bar"}, bo.DefaultEnv)
 }
 
-func TestFlags(t *testing.T) {
+func TestDefaultFlags(t *testing.T) {
 	bo := &BuildOptions{
 		WorkingDirectory: "testdata/config",
 	}
 	err := bo.LoadConfig()
 	require.NoError(t, err)
-	require.Equal(t, []string{"-tags", "netgo"}, bo.Flags)
+	require.Equal(t, []string{"-tags", "netgo"}, bo.DefaultFlags)
 }
 
-func TestLDFlags(t *testing.T) {
+func TestDefaultLdFlags(t *testing.T) {
 	bo := &BuildOptions{
 		WorkingDirectory: "testdata/config",
 	}
 	err := bo.LoadConfig()
 	require.NoError(t, err)
-	require.Equal(t, []string{"-s -w"}, bo.Ldflags)
+	require.Equal(t, []string{"-s -w"}, bo.DefaultLdflags)
 }
 
 func TestBuildConfigWithWorkingDirectoryAndDirAndMain(t *testing.T) {

pkg/commands/options/testdata/config/.ko.yaml

@@ -1,8 +1,8 @@
 defaultBaseImage: alpine
 defaultPlatforms: all
-env: FOO=bar
-flags:
+defaultEnv: FOO=bar
+defaultFlags:
   - -tags
   - netgo
-ldflags:
+defaultLdflags:
   - -s -w

pkg/commands/resolver.go

@@ -86,9 +86,9 @@ func gobuildOptions(bo *options.BuildOptions) ([]build.Option, error) {
 
 	opts := []build.Option{
 		build.WithBaseImages(getBaseImage(bo)),
-		build.WithEnv(bo.Env),
-		build.WithFlags(bo.Flags),
-		build.WithLdflags(bo.Ldflags),
+		build.WithDefaultEnv(bo.DefaultEnv),
+		build.WithDefaultFlags(bo.DefaultFlags),
+		build.WithDefaultLdflags(bo.DefaultLdflags),
 		build.WithPlatforms(bo.Platforms...),
 		build.WithJobs(bo.ConcurrentBuilds),
 	}