From 2dc2792d7ccb29d5ead47b61630eaa2559a2bf01 Mon Sep 17 00:00:00 2001 From: Halvard Skogsrud Date: Mon, 23 Aug 2021 17:50:23 +1000 Subject: [PATCH] Use working directory and build config `dir` Use the working directory from `BuildOptions` to load `.ko.yaml`. Also, use the `dir` build config field to load package information, instead of assuming that `go.mod` is in the current working directory. This removes the `init()` function from `./pkg/commands/config.go`. And avoids the global viper instance, which caused some Heisenbugs (and associated hair loss). Fixes: #422, #424 --- pkg/commands/config.go | 73 +++++++++++-------- pkg/commands/config_test.go | 31 ++++++++ pkg/commands/resolver.go | 1 + pkg/commands/testdata/config/.ko.yaml | 1 + pkg/commands/testdata/paths/.ko.yaml | 4 + .../testdata/paths/app/cmd/foo/main.go | 5 ++ pkg/commands/testdata/paths/app/go.mod | 3 + 7 files changed, 86 insertions(+), 32 deletions(-) create mode 100644 pkg/commands/testdata/config/.ko.yaml create mode 100644 pkg/commands/testdata/paths/.ko.yaml create mode 100644 pkg/commands/testdata/paths/app/cmd/foo/main.go create mode 100644 pkg/commands/testdata/paths/app/go.mod diff --git a/pkg/commands/config.go b/pkg/commands/config.go index 79c9634980..ccf893cb38 100644 --- a/pkg/commands/config.go +++ b/pkg/commands/config.go @@ -41,6 +41,11 @@ import ( "golang.org/x/tools/go/packages" ) +const ( + // configDefaultBaseImage is the default base image if not specified in .ko.yaml. + configDefaultBaseImage = "gcr.io/distroless/static:nonroot" +) + var ( defaultBaseImage string baseImageOverrides map[string]string @@ -168,8 +173,8 @@ func createCancellableContext() context.Context { return ctx } -func createBuildConfigs(baseDir string, configs []build.Config) map[string]build.Config { - buildConfigs = make(map[string]build.Config) +func createBuildConfigs(workingDirectory string, configs []build.Config) map[string]build.Config { + buildConfigsByImportPath := make(map[string]build.Config) for i, config := range configs { // Make sure to behave like GoReleaser by defaulting to the current // directory in case the build or main field is not set, check @@ -181,74 +186,78 @@ func createBuildConfigs(baseDir string, configs []build.Config) map[string]build config.Main = "." } - // To behave like GoReleaser, check whether the configured path points to a - // source file, and if so, just use the directory it is in - var path string - if fi, err := os.Stat(filepath.Join(baseDir, config.Dir, config.Main)); err == nil && fi.Mode().IsRegular() { - path = filepath.Dir(filepath.Join(config.Dir, config.Main)) + // baseDir is the directory where `go list` will be run to look for package information + baseDir := filepath.Join(workingDirectory, config.Dir) - } else { - path = filepath.Join(config.Dir, config.Main) + // To behave like GoReleaser, check whether the configured `main` config value points to a + // source file, and if so, just use the directory it is in + path := config.Main + if fi, err := os.Stat(filepath.Join(baseDir, config.Main)); err == nil && fi.Mode().IsRegular() { + path = filepath.Dir(config.Main) } // By default, paths configured in the builds section are considered // local import paths, therefore add a "./" equivalent as a prefix to // the constructured import path - importPath := fmt.Sprint(".", string(filepath.Separator), path) + localImportPath := fmt.Sprint(".", string(filepath.Separator), path) - pkgs, err := packages.Load(&packages.Config{Mode: packages.NeedName, Dir: baseDir}, importPath) + pkgs, err := packages.Load(&packages.Config{Mode: packages.NeedName, Dir: baseDir}, localImportPath) if err != nil { - log.Fatalf("'builds': entry #%d does not contain a usuable path (%s): %v", i, importPath, err) + log.Fatalf("'builds': entry #%d does not contain a valid local import path (%s) for directory (%s): %v", i, localImportPath, baseDir, err) } if len(pkgs) != 1 { log.Fatalf("'builds': entry #%d results in %d local packages, only 1 is expected", i, len(pkgs)) } - - importPath = pkgs[0].PkgPath - buildConfigs[importPath] = config + importPath := pkgs[0].PkgPath + buildConfigsByImportPath[importPath] = config } - return buildConfigs + return buildConfigsByImportPath } -func init() { +// loadConfig reads build configuration from defaults, environment variables, and the `.ko.yaml` config file. +func loadConfig(workingDirectory string) { + v := viper.New() + if workingDirectory == "" { + workingDirectory = "." + } // If omitted, use this base image. - viper.SetDefault("defaultBaseImage", "gcr.io/distroless/static:nonroot") - viper.SetConfigName(".ko") // .yaml is implicit - viper.SetEnvPrefix("KO") - viper.AutomaticEnv() + v.SetDefault("defaultBaseImage", configDefaultBaseImage) + v.SetConfigName(".ko") // .yaml is implicit + v.SetEnvPrefix("KO") + v.AutomaticEnv() if override := os.Getenv("KO_CONFIG_PATH"); override != "" { - viper.AddConfigPath(override) + v.AddConfigPath(override) } - viper.AddConfigPath("./") + v.AddConfigPath(workingDirectory) - if err := viper.ReadInConfig(); err != nil { + if err := v.ReadInConfig(); err != nil { if _, ok := err.(viper.ConfigFileNotFoundError); !ok { log.Fatalf("error reading config file: %v", err) } } - ref := viper.GetString("defaultBaseImage") + ref := v.GetString("defaultBaseImage") if _, err := name.ParseReference(ref); err != nil { log.Fatalf("'defaultBaseImage': error parsing %q as image reference: %v", ref, err) } defaultBaseImage = ref baseImageOverrides = make(map[string]string) - overrides := viper.GetStringMapString("baseImageOverrides") - for k, v := range overrides { - if _, err := name.ParseReference(v); err != nil { - log.Fatalf("'baseImageOverrides': error parsing %q as image reference: %v", v, err) + overrides := v.GetStringMapString("baseImageOverrides") + for key, value := range overrides { + if _, err := name.ParseReference(value); err != nil { + log.Fatalf("'baseImageOverrides': error parsing %q as image reference: %v", value, err) } - baseImageOverrides[k] = v + baseImageOverrides[key] = value } var builds []build.Config - if err := viper.UnmarshalKey("builds", &builds); err != nil { + if err := v.UnmarshalKey("builds", &builds); err != nil { log.Fatalf("configuration section 'builds' cannot be parsed") } - buildConfigs = createBuildConfigs(".", builds) + buildConfigs = createBuildConfigs(workingDirectory, builds) } diff --git a/pkg/commands/config_test.go b/pkg/commands/config_test.go index e6a7762b4a..245010f39e 100644 --- a/pkg/commands/config_test.go +++ b/pkg/commands/config_test.go @@ -47,6 +47,37 @@ func TestOverrideDefaultBaseImageUsingBuildOption(t *testing.T) { } } +// TestDefaultBaseImage is a canary-type test for ensuring that config has been read when creating a builder. +func TestDefaultBaseImage(t *testing.T) { + _, err := NewBuilder(context.Background(), &options.BuildOptions{ + WorkingDirectory: "testdata/config", + }) + if err != nil { + t.Fatal(err) + } + wantDefaultBaseImage := "gcr.io/distroless/base:nonroot" // matches value in ./testdata/.ko.yaml + if defaultBaseImage != wantDefaultBaseImage { + t.Fatalf("wanted defaultBaseImage %s, got %s", wantDefaultBaseImage, defaultBaseImage) + } +} + +func TestBuildConfigWithWorkingDirectoryAndDirAndMain(t *testing.T) { + _, err := NewBuilder(context.Background(), &options.BuildOptions{ + WorkingDirectory: "testdata/paths", + }) + if err != nil { + t.Fatalf("NewBuilder(): %+v", err) + } + + if len(buildConfigs) != 1 { + t.Fatalf("expected 1 build config, got %d", len(buildConfigs)) + } + expectedImportPath := "example.com/testapp/cmd/foo" // module from app/go.mod + `main` from .ko.yaml + if _, exists := buildConfigs[expectedImportPath]; !exists { + t.Fatalf("expected build config for import path [%s], got %+v", expectedImportPath, buildConfigs) + } +} + func TestCreateBuildConfigs(t *testing.T) { compare := func(expected string, actual string) { if expected != actual { diff --git a/pkg/commands/resolver.go b/pkg/commands/resolver.go index b2e95635f3..e4cb386f79 100644 --- a/pkg/commands/resolver.go +++ b/pkg/commands/resolver.go @@ -119,6 +119,7 @@ func NewBuilder(ctx context.Context, bo *options.BuildOptions) (build.Interface, } func makeBuilder(ctx context.Context, bo *options.BuildOptions) (*build.Caching, error) { + loadConfig(bo.WorkingDirectory) opt, err := gobuildOptions(bo) if err != nil { return nil, fmt.Errorf("error setting up builder options: %v", err) diff --git a/pkg/commands/testdata/config/.ko.yaml b/pkg/commands/testdata/config/.ko.yaml new file mode 100644 index 0000000000..cac68c85a4 --- /dev/null +++ b/pkg/commands/testdata/config/.ko.yaml @@ -0,0 +1 @@ +defaultBaseImage: gcr.io/distroless/base:nonroot diff --git a/pkg/commands/testdata/paths/.ko.yaml b/pkg/commands/testdata/paths/.ko.yaml new file mode 100644 index 0000000000..2e54a1b857 --- /dev/null +++ b/pkg/commands/testdata/paths/.ko.yaml @@ -0,0 +1,4 @@ +builds: +- id: app-with-main-package-in-different-directory-to-go-mod-and-ko-yaml + dir: ./app + main: ./cmd/foo diff --git a/pkg/commands/testdata/paths/app/cmd/foo/main.go b/pkg/commands/testdata/paths/app/cmd/foo/main.go new file mode 100644 index 0000000000..40f7d74553 --- /dev/null +++ b/pkg/commands/testdata/paths/app/cmd/foo/main.go @@ -0,0 +1,5 @@ +package main + +func main() { + println("cmd/foo") +} diff --git a/pkg/commands/testdata/paths/app/go.mod b/pkg/commands/testdata/paths/app/go.mod new file mode 100644 index 0000000000..225677766b --- /dev/null +++ b/pkg/commands/testdata/paths/app/go.mod @@ -0,0 +1,3 @@ +module example.com/testapp + +go 1.15