From eae7d5689591ba6a2856834fdf45d076974bffda Mon Sep 17 00:00:00 2001 From: Luke Kingland <58986931+lkingland@users.noreply.github.com> Date: Tue, 22 Nov 2022 03:50:42 +0900 Subject: [PATCH] feat: command function context (#1416) * feat: global config function context The function with context is considered when determining flag defaults such that: - No special logic is required to determine "if changed" - help text correctly reflects the actual values which will be used - Global config can be a direct parent of command-specific config via embed Also included: - CLI tests clear environment of FUNC_* variables - Command's bindEnv helper also sets up environment variable auto-binding and prefix. - Verbosisty flag default now globally configurable * Update cmd/build.go Co-authored-by: Lance Ball * Update cmd/build.go Co-authored-by: Lance Ball * Update cmd/build.go Co-authored-by: Lance Ball * move clearEnvs test function to test file * docs regen Co-authored-by: Lance Ball --- cmd/build.go | 197 ++++++----- cmd/build_test.go | 313 ++++++++++++++---- cmd/root.go | 15 + cmd/root_test.go | 2 + .../TestBuild_ConfigApplied/func/config.yaml | 3 + .../func/config.yaml | 3 + config/config.go | 58 +++- config/config_test.go | 97 ++++++ docs/reference/func_build.md | 7 +- 9 files changed, 518 insertions(+), 177 deletions(-) create mode 100644 cmd/testdata/TestBuild_ConfigApplied/func/config.yaml create mode 100644 cmd/testdata/TestBuild_ConfigPrecidence/func/config.yaml diff --git a/cmd/build.go b/cmd/build.go index 4bc3be6a..beaf32c1 100644 --- a/cmd/build.go +++ b/cmd/build.go @@ -66,58 +66,69 @@ EXAMPLES `, SuggestFor: []string{"biuld", "buidl", "built"}, PreRunE: bindEnv("image", "path", "builder", "registry", "confirm", "push", "builder-image", "platform"), + RunE: func(cmd *cobra.Command, args []string) error { + return runBuild(cmd, args, newClient) + }, } - // Config + // Global Config cfg, err := config.NewDefault() if err != nil { fmt.Fprintf(cmd.OutOrStdout(), "error loading config at '%v'. %v\n", config.File(), err) } // Function Context - // Load the value of the builder from the function at the effective path - // if it exists. - // This value takes precedence over the global config value, which encapsulates - // both the static default (builders.default) and any extant user setting in - // their global config file. - // The defaulting of path to cwd() can be removed when the open PR # - // is merged which updates the system to treat an empty path as indicating - // CWD by default. - builder := cfg.Builder - path := effectivePath() - if path == "" { - path = cwd() - } - if f, err := fn.NewFunction(path); err == nil && f.Build.Builder != "" { - // no errors loading the function at path, and it has a builder specified: - // The "function with context" takes precedence determining flag defaults - builder = f.Build.Builder + f, _ := fn.NewFunction(effectivePath()) + if f.Initialized() { + cfg = cfg.Apply(f) // defined values on f take precidence over cfg defaults } - cmd.Flags().StringP("builder", "b", builder, fmt.Sprintf("build strategy to use when creating the underlying image. Currently supported build strategies are %s.", KnownBuilders())) - cmd.Flags().StringP("builder-image", "", "", "builder image, either an as a an image name or a mapping name.\nSpecified value is stored in func.yaml (as 'builder' field) for subsequent builds. ($FUNC_BUILDER_IMAGE)") - cmd.Flags().BoolP("confirm", "c", cfg.Confirm, "Prompt to confirm all configuration options (Env: $FUNC_CONFIRM)") - cmd.Flags().StringP("image", "i", "", "Full image name in the form [registry]/[namespace]/[name]:[tag] (optional). This option takes precedence over --registry (Env: $FUNC_IMAGE)") - cmd.Flags().StringP("registry", "r", "", "Registry + namespace part of the image to build, ex 'quay.io/myuser'. The full image name is automatically determined (Env: $FUNC_REGISTRY)") - cmd.Flags().BoolP("push", "u", false, "Attempt to push the function image after being successfully built") - cmd.Flags().Lookup("push").NoOptDefVal = "true" // --push == --push=true - cmd.Flags().StringP("platform", "", "", "Target platform to build (e.g. linux/amd64).") + // Flags + // + // NOTE on falag defaults: + // Use the config value when available, as this will include global static + // defaults, user settings and the value from the function with context. + // Use the function struct for flag flags which are not globally configurable + // + // Globally-Configurable Flags: + // Options whose value may be defined globally may also exist on the + // contextually relevant function; sets are flattened above via cfg.Apply(f) + cmd.Flags().StringP("builder", "b", cfg.Builder, + fmt.Sprintf("build strategy to use when creating the underlying image. Currently supported build strategies are %s.", KnownBuilders())) + cmd.Flags().BoolP("confirm", "c", cfg.Confirm, + "Prompt to confirm all configuration options (Env: $FUNC_CONFIRM)") + cmd.Flags().StringP("registry", "r", cfg.Registry, + "Registry + namespace part of the image to build, ex 'quay.io/myuser'. The full image name is automatically determined (Env: $FUNC_REGISTRY)") + + // Function-Context Flags: + // Options whose value is available on the function with context only + // (persisted but not globally configurable) + builderImage := f.Build.BuilderImages[f.Build.Builder] + cmd.Flags().StringP("builder-image", "", builderImage, + "Specify a custom builder image for use by the builder other than its default. (Env: $FUNC_BUILDER_IMAGE)") + cmd.Flags().StringP("image", "i", f.Image, + "Full image name in the form [registry]/[namespace]/[name]:[tag] (optional). This option takes precedence over --registry (Env: $FUNC_IMAGE)") + + // Static Flags: + // Options which have static defaults only (not globally configurable nor + // persisted with the function) + cmd.Flags().BoolP("push", "u", false, + "Attempt to push the function image to the configured registry after being successfully built") + cmd.Flags().StringP("platform", "", "", + "Optionally specify a target platform, for example \"linux/amd64\" when using the s2i build strategy") setPathFlag(cmd) + // Tab Completion if err := cmd.RegisterFlagCompletionFunc("builder", CompleteBuilderList); err != nil { fmt.Println("internal: error while calling RegisterFlagCompletionFunc: ", err) } - if err := cmd.RegisterFlagCompletionFunc("builder-image", CompleteBuilderImageList); err != nil { fmt.Println("internal: error while calling RegisterFlagCompletionFunc: ", err) } + // Help Text cmd.SetHelpFunc(defaultTemplatedHelp) - cmd.RunE = func(cmd *cobra.Command, args []string) error { - return runBuild(cmd, args, newClient) - } - return cmd } @@ -126,41 +137,21 @@ func runBuild(cmd *cobra.Command, _ []string, newClient ClientFactory) (err erro return // see docker/creds potential mutation of auth.json } - // Populate a command config from environment variables, flags and potentially - // interactive user prompts if in confirm mode. cfg, err := newBuildConfig().Prompt() if err != nil { return } - // Validate the config if err = cfg.Validate(); err != nil { return } - // Load the Function at path, and if it is initialized, update it with - // pertinent values from the config. - // - // NOTE: the checks for .Changed and altered conditionals for defaults will - // be removed when Global Config is integrated, because the config object - // will at that time contain the final value for the attribute, taking into - // account whether or not the value was altered via flags or env variables. - // This condition is also only necessary for config members whose default - // value deviates from the zero value. f, err := fn.NewFunction(cfg.Path) if err != nil { return } - if !f.Initialized() { - return fmt.Errorf("'%v' does not contain an initialized function", cfg.Path) - } - if f.Registry == "" || cmd.Flags().Changed("registry") { - // Sets default AND accepts any user-provided overrides - f.Registry = cfg.Registry - } - if cfg.Image != "" { - f.Image = cfg.Image - } + f = cfg.Configure(f) // Updates f at path to include buil request values + // Checks if there is a difference between defined registry and its value used as a prefix in the image tag // In case of a mismatch a new image tag is created and used for build // Do not react if image tag has been changed outside configuration @@ -174,24 +165,9 @@ func runBuild(cmd *cobra.Command, _ []string, newClient ClientFactory) (err erro fmt.Fprintf(cmd.ErrOrStderr(), "Warning: function has current image '%s' which has a different registry than the currently configured registry '%s'. The new image tag will be '%s'. To use an explicit image, use --image.\n", f.Image, f.Registry, updImg) f.Image = updImg } - if f.Build.Builder == "" || cmd.Flags().Changed("builder") { - // Sets default AND accepts any user-provided overrides - f.Build.Builder = cfg.Builder - } - if cfg.Builder != "" { - f.Build.Builder = cfg.Builder - } - if cfg.BuilderImage != "" { - f.Build.BuilderImages[cfg.Builder] = cfg.BuilderImage - } - // Validate that a builder short-name was obtained, whether that be from - // the function's prior state, or the value of flags/environment. - if err = ValidateBuilder(f.Build.Builder); err != nil { - return - } - - // Choose a builder based on the value of the --builder flag + // Client + // Concrete implementations (ex builder) vary based on final effective config var builder fn.Builder if f.Build.Builder == builders.Pack { builder = buildpacks.NewBuilder( @@ -203,8 +179,7 @@ func runBuild(cmd *cobra.Command, _ []string, newClient ClientFactory) (err erro s2i.WithPlatform(cfg.Platform), s2i.WithVerbose(cfg.Verbose)) } else { - err = fmt.Errorf("builder '%v' is not recognized", f.Build.Builder) - return + return builders.ErrUnknownBuilder{Name: f.Build.Builder, Known: KnownBuilders()} } client, done := newClient(ClientConfig{Verbose: cfg.Verbose}, @@ -212,13 +187,13 @@ func runBuild(cmd *cobra.Command, _ []string, newClient ClientFactory) (err erro fn.WithBuilder(builder)) defer done() - // This preemptive write call will be unnecessary when the API is updated - // to use Function instances rather than file paths. For now it must write - // even if the command fails later. Not ideal. + // TODO(lkingland): this write will be unnecessary when the client API is + // udated to accept function structs rather than a path as argument. if err = f.Write(); err != nil { return } + // Build and (optionally) push if err = client.Build(cmd.Context(), cfg.Path); err != nil { return } @@ -226,10 +201,23 @@ func runBuild(cmd *cobra.Command, _ []string, newClient ClientFactory) (err erro err = client.Push(cmd.Context(), cfg.Path) } + // TODO(lkingland): when the above Build and Push calls are refactored to not + // write the function but instead take and return a function struct, use + // `reuturn f.Write()` below and remove from above such that function on disk + // is only written on success and thus is always in a known valid state unless + // manually edited. + // return f.Write() return } type buildConfig struct { + // Globals (builder, confirm, registry, verbose) + config.Global + + // BuilderImage is the image (name or mapping) to use for building. Usually + // set automatically. + BuilderImage string + // Image name in full, including registry, repo and tag (overrides // image name derivation based on registry and function name) Image string @@ -238,46 +226,44 @@ type buildConfig struct { // working directory of the process. Path string + // Platform ofr resultant image (s2i builder only) + Platform string + // Push the resulting image to the registry after building. Push bool - - // Registry at which interstitial build artifacts should be kept. - // This setting is ignored if Image is specified, which includes the full - Registry string - - // Verbose logging. - Verbose bool - - // Confirm: confirm values arrived upon from environment plus flags plus defaults, - // with interactive prompting (only applicable when attached to a TTY). - Confirm bool - - // Builder is the name of the subsystem that will complete the underlying - // build (Pack, s2i, remote pipeline, etc). Currently ad-hoc rather than - // an enumerated field. See the Client constructory for logic. - Builder string - - // BuilderImage is the image (name or mapping) to use for building. Usually - // set automatically. - BuilderImage string - - Platform string } +// newBuildConfig gathers options into a single build request. func newBuildConfig() buildConfig { return buildConfig{ + Global: config.Global{ + Builder: viper.GetString("builder"), + Confirm: viper.GetBool("confirm"), + Registry: registry(), // deferred defaulting + Verbose: viper.GetBool("verbose"), + }, + BuilderImage: viper.GetString("builder-image"), Image: viper.GetString("image"), Path: viper.GetString("path"), - Registry: registry(), - Verbose: viper.GetBool("verbose"), // defined on root - Confirm: viper.GetBool("confirm"), - Builder: viper.GetString("builder"), - BuilderImage: viper.GetString("builder-image"), - Push: viper.GetBool("push"), Platform: viper.GetString("platform"), + Push: viper.GetBool("push"), } } +// Configure the given function. Updates a function struct with all +// configurable values. Note that buildConfig already includes function's +// current values, as they were passed through vi flag defaults, so overwriting +// is a noop. +func (c buildConfig) Configure(f fn.Function) fn.Function { + f = c.Global.Configure(f) + if f.Build.Builder != "" && c.BuilderImage != "" { + f.Build.BuilderImages[f.Build.Builder] = c.BuilderImage + } + f.Image = c.Image + // Path, Platform and Push are not part of a function's state. + return f +} + // Prompt the user with value of config members, allowing for interaractive changes. // Skipped if not in an interactive terminal (non-TTY), or if --confirm false (agree to // all prompts) was set (default). @@ -346,7 +332,12 @@ func (c buildConfig) Prompt() (buildConfig, error) { // Validate the config passes an initial consistency check func (c buildConfig) Validate() (err error) { + // Builder value must refer to a known builder short name + if err = ValidateBuilder(c.Builder); err != nil { + return + } + // Platform is only supportd with the S2I builder at this time if c.Platform != "" && c.Builder != builders.S2I { err = errors.New("Only S2I builds currently support specifying platform") return diff --git a/cmd/build_test.go b/cmd/build_test.go index bd92765c..287d4ab5 100644 --- a/cmd/build_test.go +++ b/cmd/build_test.go @@ -5,15 +5,193 @@ import ( "fmt" "testing" - "gotest.tools/v3/assert" - fn "knative.dev/func" "knative.dev/func/builders" "knative.dev/func/mock" ) -// TestBuild_ImageFlag ensures that the image flag is used when specified, and -// is used in place of a configured registry. +// TestBuild_ConfigApplied ensures that the build command applies config +// settings at each level (static, global, function, envs, flags). +func TestBuild_ConfigApplied(t *testing.T) { + var ( + err error + home = fmt.Sprintf("%s/testdata/TestBuild_ConfigApplied", cwd()) + root = fromTempDirectory(t) + f = fn.Function{Runtime: "go", Root: root, Name: "f"} + pusher = mock.NewPusher() + clientFn = NewTestClient(fn.WithPusher(pusher)) + ) + t.Setenv("XDG_CONFIG_HOME", home) + + if err = fn.New().Create(f); err != nil { + t.Fatal(err) + } + + // Ensure the global config setting was loaded: Registry + // global config in ./testdata/TestBuild_ConfigApplied sets registry + if err = NewBuildCmd(clientFn).Execute(); err != nil { + t.Fatal(err) + } + if f, err = fn.NewFunction(root); err != nil { + t.Fatal(err) + } + if f.Registry != "registry.example.com/alice" { + t.Fatalf("expected registry 'registry.example.com/alice' got '%v'", f.Registry) + } + + // Ensure flags are evaluated + cmd := NewBuildCmd(clientFn) + cmd.SetArgs([]string{"--builder-image", "example.com/builder/image:v1.2.3"}) + if err = cmd.Execute(); err != nil { + t.Fatal(err) + } + if f, err = fn.NewFunction(root); err != nil { + t.Fatal(err) + } + if f.Build.BuilderImages[f.Build.Builder] != "example.com/builder/image:v1.2.3" { + t.Fatalf("expected builder image not set. Flags not evaluated? got %v", f.Build.BuilderImages[f.Build.Builder]) + } + + // Ensure function context loaded + // Update registry on the function and ensure it takes precidence (overrides) + // the global setting defined in home. + f.Registry = "registry.example.com/charlie" + if err := f.Write(); err != nil { + t.Fatal(err) + } + if err := NewBuildCmd(clientFn).Execute(); err != nil { + t.Fatal(err) + } + if f, err = fn.NewFunction(root); err != nil { + t.Fatal(err) + } + if f.Image != "registry.example.com/charlie/f:latest" { + t.Fatalf("expected image 'registry.example.com/charlie/f:latest' got '%v'", f.Image) + } + + // Ensure environment variables loaded: Push + // Test environment variable evaluation using FUNC_PUSH + t.Setenv("FUNC_PUSH", "true") + if err := NewBuildCmd(clientFn).Execute(); err != nil { + t.Fatal(err) + } + if f, err = fn.NewFunction(root); err != nil { + t.Fatal(err) + } + if !pusher.PushInvoked { + t.Fatalf("push was not invoked when FUNC_PUSH=true") + } + +} + +// TestBuild_ConfigPrecidence ensures that the correct precidence for config +// are applied: static < global < function context < envs < flags +func TestBuild_ConfigPrecidence(t *testing.T) { + var ( + err error + home = fmt.Sprintf("%s/testdata/TestBuild_ConfigPrecidence", cwd()) + builder = mock.NewBuilder() + clientFn = NewTestClient(fn.WithBuilder(builder)) + ) + + // Ensure static default applied via 'builder' + // (a degenerate case, mostly just ensuring config values are not wiped to a + // zero value struct, etc) + root := fromTempDirectory(t) + t.Setenv("XDG_CONFIG_HOME", home) // sets registry.example.com/global + f := fn.Function{Runtime: "go", Root: root, Name: "f"} + if err = fn.New().Create(f); err != nil { + t.Fatal(err) + } + if err := NewBuildCmd(clientFn).Execute(); err != nil { + t.Fatal(err) + } + if f, err = fn.NewFunction(root); err != nil { + t.Fatal(err) + } + if f.Build.Builder != builders.Default { + t.Fatalf("expected static default builder '%v', got '%v'", builders.Default, f.Build.Builder) + } + + // Ensure Global Config applied via config in ./testdata + root = fromTempDirectory(t) + t.Setenv("XDG_CONFIG_HOME", home) // sets registry.example.com/global + f = fn.Function{Runtime: "go", Root: root, Name: "f"} + if err := fn.New().Create(f); err != nil { + t.Fatal(err) + } + if err = NewBuildCmd(clientFn).Execute(); err != nil { + t.Fatal(err) + } + if f, err = fn.NewFunction(root); err != nil { + t.Fatal(err) + } + if f.Registry != "registry.example.com/global" { // from ./testdata + t.Fatalf("expected registry 'example.com/global', got '%v'", f.Registry) + } + + // Ensure Function context overrides global config + // The stanza above ensures the global config is applied. This stanza + // ensures that, if set on the function, it will take precidence. + root = fromTempDirectory(t) + t.Setenv("XDG_CONFIG_HOME", home) // sets registry=example.com/global + f = fn.Function{Runtime: "go", Root: root, Name: "f", + Registry: "example.com/function"} + if err := fn.New().Create(f); err != nil { + t.Fatal(err) + } + if err = NewBuildCmd(clientFn).Execute(); err != nil { + t.Fatal(err) + } + if f, err = fn.NewFunction(root); err != nil { + t.Fatal(err) + } + if f.Registry != "example.com/function" { + t.Fatalf("expected function's value for registry of 'example.com/function' to override global config setting of 'example.com/global', but got '%v'", f.Registry) + } + + // Ensure Environment Variable overrides function context. + root = fromTempDirectory(t) + t.Setenv("XDG_CONFIG_HOME", home) // sets registry.example.com/global + t.Setenv("FUNC_REGISTRY", "example.com/env") + f = fn.Function{Runtime: "go", Root: root, Name: "f", + Registry: "example.com/function"} + if err := fn.New().Create(f); err != nil { + t.Fatal(err) + } + if err := NewBuildCmd(clientFn).Execute(); err != nil { + t.Fatal(err) + } + if f, err = fn.NewFunction(root); err != nil { + t.Fatal(err) + } + if f.Registry != "example.com/env" { + t.Fatalf("expected FUNC_REGISTRY=example.com/env to override function's value of 'example.com/function', but got '%v'", f.Registry) + } + + // Ensure flags override environment variables. + root = fromTempDirectory(t) + t.Setenv("XDG_CONFIG_HOME", home) // sets registry=example.com/global + t.Setenv("FUNC_REGISTRY", "example.com/env") + f = fn.Function{Runtime: "go", Root: root, Name: "f", + Registry: "example.com/function"} + if err := fn.New().Create(f); err != nil { + t.Fatal(err) + } + cmd := NewBuildCmd(clientFn) + cmd.SetArgs([]string{"--registry=example.com/flag"}) + if err := cmd.Execute(); err != nil { + t.Fatal(err) + } + if f, err = fn.NewFunction(root); err != nil { + t.Fatal(err) + } + if f.Registry != "example.com/flag" { + t.Fatalf("expected flag 'example.com/flag' to take precidence over env var, but got '%v'", f.Registry) + } +} + +// TestBuild_ImageFlag ensures that the image flag is used when specified. func TestBuild_ImageFlag(t *testing.T) { var ( args = []string{"--image", "docker.io/tigerteam/foo"} @@ -155,74 +333,83 @@ func TestBuild_Push(t *testing.T) { } } -type buildWithRegistryTestCase struct { - desc string - testFn fn.Function - testFnArgs []string - expRegistry string - expImage string -} - -func TestBuild_RegistryHandling(t *testing.T) { - root := fromTempDirectory(t) - for i, tc := range []buildWithRegistryTestCase{ - +// TestBuild_Registry ensures that a function's registry member is kept in +// sync with the image tag. +// During normal operation (using the client API) a function's state on disk +// will be in a valid state, but it is possible that a function could be +// manually edited, necessitating some kind of consistency recovery (as +// preferable to just an error). +func TestBuild_Registry(t *testing.T) { + tests := []struct { + name string // name of the test + f fn.Function // function initial state + args []string // command arguments + expectedRegistry string // expected value after build + expectedImage string // expected value after build + }{ { - desc: "should update func.yaml's image tag if mismatch with func.yaml's registry", - testFn: fn.Function{ - Runtime: "go", - Root: root + "/1", - Registry: TestRegistry, // defined as "example.com/alice" - Image: "docker.io/tigerteam/foo", // image uses different registry in its tag, so it has to be updated + // Registry function member takes precidence, updating image member + // when out of sync. + name: "registry member mismatch", + f: fn.Function{ + Registry: "registry.example.com/alice", + Image: "registry.example.com/bob/f:latest", }, - testFnArgs: []string{"--path", root + "/1"}, - expRegistry: TestRegistry, - expImage: TestRegistry + "/foo", + args: []string{}, + expectedRegistry: "registry.example.com/alice", + expectedImage: "registry.example.com/alice/f:latest", }, { - desc: "should update func.yaml's image tag and registry if mismatch with --registry flag", - testFn: fn.Function{ - Runtime: "go", - Root: root + "/2", - Registry: TestRegistry, - Image: "docker.io/tigerteam/foo", + // Registry flag takes highest precidence, affecting both the registry + // member and the resultant image member and therefore affects subsequent + // builds. + name: "registry flag updates", + f: fn.Function{ + Registry: "registry.example.com/alice", + Image: "registry.example.com/bob/f:latest", }, - testFnArgs: []string{"--path", root + "/2", "--registry", "example.com/test"}, // registry flag should overwrite func.yaml's image and registry - expRegistry: "example.com/test", - expImage: "example.com/test/foo", + args: []string{"--registry=registry.example.com/charlie"}, + expectedRegistry: "registry.example.com/charlie", + expectedImage: "registry.example.com/charlie/f:latest", }, - { - desc: "should NOT update func.yaml's registry if --image flag provided", - testFn: fn.Function{ - Runtime: "go", - Root: root + "/3", - Registry: TestRegistry, - Image: "docker.io/tigerteam/foo", + // Image flag takes highest precidence, but is an override such that the + // resultant image member is updated but the registry member is not + // updated (subsequent builds will not be affected). + name: "image flag overrides", + f: fn.Function{ + Registry: "registry.example.com/alice", + Image: "registry.example.com/bob/f:latest", }, - testFnArgs: []string{"--path", root + "/3", "--image", "example.com/test/boo"}, // image flag should NOT overwrite func.yaml's registry - expRegistry: TestRegistry, - expImage: "example.com/test/boo", + args: []string{"--image=registry.example.com/charlie/f:latest"}, + expectedRegistry: "registry.example.com/alice", // not updated + expectedImage: "registry.example.com/charlie/f:latest", // updated }, - } { - var builder = mock.NewBuilder() - cmd := NewBuildCmd(NewTestClient(fn.WithBuilder(builder))) - cmd.SetArgs(tc.testFnArgs) - - tci := i + 1 - t.Logf("Test case %d: %s", tci, tc.desc) - - err := fn.New().Create(tc.testFn) - assert.Assert(t, err == nil) - - err = cmd.Execute() - assert.Assert(t, err == nil) - - f, err := fn.NewFunction(tc.testFn.Root) - assert.Assert(t, err == nil) - - assert.Assert(t, f.Registry == tc.expRegistry, fmt.Sprintf("Test case %d: expected registry to be '"+tc.expRegistry+"', but got '%s'", tci, f.Registry)) - assert.Assert(t, f.Image == tc.expImage, fmt.Sprintf("Test case %d: expected image to be '"+tc.expImage+"', but got '%s'", tci, f.Image)) + } + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + root := fromTempDirectory(t) + test.f.Runtime = "go" + test.f.Name = "f" + if err := fn.New().Create(test.f); err != nil { + t.Fatal(err) + } + cmd := NewBuildCmd(NewTestClient()) + cmd.SetArgs(test.args) + if err := cmd.Execute(); err != nil { + t.Fatal(err) + } + f, err := fn.NewFunction(root) + if err != nil { + t.Fatal(err) + } + if f.Registry != test.expectedRegistry { + t.Fatalf("expected registry '%v', got '%v'", test.expectedRegistry, f.Registry) + } + if f.Image != test.expectedImage { + t.Fatalf("expected image '%v', got '%v'", test.expectedImage, f.Image) + } + }) } } diff --git a/cmd/root.go b/cmd/root.go index 818ebd89..581a51f7 100644 --- a/cmd/root.go +++ b/cmd/root.go @@ -7,6 +7,7 @@ import ( "os" "path/filepath" "strings" + "testing" "text/template" "time" @@ -180,6 +181,8 @@ func bindEnv(flags ...string) bindFunc { return } } + viper.AutomaticEnv() // read in environment variables for FUNC_ + viper.SetEnvPrefix("func") // ensure that all have the prefix return } } @@ -466,3 +469,15 @@ func defaultTemplatedHelp(cmd *cobra.Command, args []string) { fmt.Fprintf(cmd.ErrOrStderr(), "unable to display help text: %v", err) } } + +// clearEnvs sets all environment variables with the prefix of FUNC_ to +// empty (unsets) for the duration of the test t. +func clearEnvs(t *testing.T) { + t.Helper() + for _, v := range os.Environ() { + if strings.HasPrefix(v, "FUNC_") { + parts := strings.SplitN(v, "=", 2) + t.Setenv(parts[0], "") + } + } +} diff --git a/cmd/root_test.go b/cmd/root_test.go index ce672855..94b3782d 100644 --- a/cmd/root_test.go +++ b/cmd/root_test.go @@ -365,6 +365,8 @@ func piped(t *testing.T) func() string { // an environment clean of developer's settings for use during CLI testing. func fromTempDirectory(t *testing.T) string { t.Helper() + clearEnvs(t) + // We have to define KUBECONFIG, or the file at ~/.kube/config (if extant) // will be used (disrupting tests by using the current user's environment). // The test kubeconfig set below has the current namespace set to 'func' diff --git a/cmd/testdata/TestBuild_ConfigApplied/func/config.yaml b/cmd/testdata/TestBuild_ConfigApplied/func/config.yaml new file mode 100644 index 00000000..520516f7 --- /dev/null +++ b/cmd/testdata/TestBuild_ConfigApplied/func/config.yaml @@ -0,0 +1,3 @@ +verbose: true +confirm: true +registry: registry.example.com/alice diff --git a/cmd/testdata/TestBuild_ConfigPrecidence/func/config.yaml b/cmd/testdata/TestBuild_ConfigPrecidence/func/config.yaml new file mode 100644 index 00000000..fa104595 --- /dev/null +++ b/cmd/testdata/TestBuild_ConfigPrecidence/func/config.yaml @@ -0,0 +1,3 @@ +verbose: true +confirm: true +registry: registry.example.com/global diff --git a/config/config.go b/config/config.go index f83c13f1..91fa7434 100644 --- a/config/config.go +++ b/config/config.go @@ -6,6 +6,7 @@ import ( "path/filepath" "gopkg.in/yaml.v2" + fn "knative.dev/func" "knative.dev/func/builders" "knative.dev/func/k8s" "knative.dev/func/openshift" @@ -36,18 +37,19 @@ func DefaultNamespace() (namespace string) { } // Global configuration settings. -type Config struct { +type Global struct { Builder string `yaml:"builder,omitempty"` Confirm bool `yaml:"confirm,omitempty"` Language string `yaml:"language,omitempty"` Namespace string `yaml:"namespace,omitempty"` Registry string `yaml:"registry,omitempty"` + Verbose bool `yaml:"verbose,omitempty"` } // New Config struct with all members set to static defaults. See NewDefaults // for one which further takes into account the optional config file. -func New() Config { - return Config{ +func New() Global { + return Global{ Builder: DefaultBuilder, Language: DefaultLanguage, Namespace: DefaultNamespace(), @@ -58,7 +60,7 @@ func New() Config { // RegistyDefault is a convenience method for deferred calculation of a // default registry taking into account both the global config file and cluster // detection. -func (c Config) RegistryDefault() string { +func (c Global) RegistryDefault() string { // If defined, the user's choice for global registry default value is used if c.Registry != "" { return c.Registry @@ -77,7 +79,7 @@ func (c Config) RegistryDefault() string { // usually ~/.config/func). // // The config path is not required to be present. -func NewDefault() (cfg Config, err error) { +func NewDefault() (cfg Global, err error) { cfg = New() cp := File() bb, err := os.ReadFile(cp) @@ -92,7 +94,7 @@ func NewDefault() (cfg Config, err error) { } // Load the config exactly as it exists at path (no static defaults) -func Load(path string) (c Config, err error) { +func Load(path string) (c Global, err error) { bb, err := os.ReadFile(path) if err != nil { return c, fmt.Errorf("error reading global config: %v", err) @@ -102,11 +104,53 @@ func Load(path string) (c Config, err error) { } // Write the config to the given path -func (c Config) Write(path string) (err error) { +func (c Global) Write(path string) (err error) { bb, _ := yaml.Marshal(&c) // Marshaling no longer errors; this is back compat return os.WriteFile(path, bb, os.ModePerm) } +// Apply populated values from a function to the config. +// The resulting config is global settings overridden by a given function. +func (c Global) Apply(f fn.Function) Global { + // With no way to automate this mapping easily (even with reflection) because + // the function now has a complex structure consiting of XSpec sub-structs, + // and in some cases has differing member names (language). While this is + // yes a bit tedious, manually mapping each member (if defined) is simple, + // easy to understand and support; with both mapping direction (Apply and + // Configure) in one central place here... with tests. + if f.Build.Builder != "" { + c.Builder = f.Build.Builder + } + if f.Runtime != "" { + c.Language = f.Runtime + } + if f.Deploy.Namespace != "" { + c.Namespace = f.Deploy.Namespace + } + if f.Registry != "" { + c.Registry = f.Registry + } + return c +} + +// Configure a function with poipulated values of the config. +// The resulting function is the function overridden by values on config. +func (c Global) Configure(f fn.Function) fn.Function { + if c.Builder != "" { + f.Build.Builder = c.Builder + } + if c.Language != "" { + f.Runtime = c.Language + } + if c.Namespace != "" { + f.Deploy.Namespace = c.Namespace + } + if c.Registry != "" { + f.Registry = c.Registry + } + return f +} + // Dir is derived in the following order, from lowest // to highest precedence. // 1. The default path is the zero value, indicating "no config path available", diff --git a/config/config_test.go b/config/config_test.go index 1cc85bb4..1d03cb2f 100644 --- a/config/config_test.go +++ b/config/config_test.go @@ -5,6 +5,7 @@ import ( "path/filepath" "testing" + fn "knative.dev/func" "knative.dev/func/config" . "knative.dev/func/testing" @@ -196,3 +197,99 @@ func TestDefaultNamespace(t *testing.T) { t.Fatalf("expected default namespace of 'func' when that is the active k8s namespace. Got '%v'", config.DefaultNamespace()) } } + +// TestApply ensures that applying a function as context to a config results +// in every member of config in the intersection of the two sets, global config +// and function, to be set to the values of the function. +// (See the associated cfg.Configure) +func TestApply(t *testing.T) { + // Yes, every member needs to be painstakingly enumerated by hand, because + // the sets are not equivalent. Not all global settings have an associated + // member on the function (example: confirm), and not all members of a + // function are globally configurable (example: image). + f := fn.Function{ + Build: fn.BuildSpec{ + Builder: "builder", + }, + Deploy: fn.DeploySpec{ + Namespace: "namespace", + }, + Runtime: "runtime", + Registry: "registry", + } + cfg := config.Global{}.Apply(f) + + if cfg.Builder != "builder" { + t.Error("apply missing map of f.Build.Builder") + } + if cfg.Language != "runtime" { + t.Error("apply missing map of f.Runtime ") + } + if cfg.Namespace != "namespace" { + t.Error("apply missing map of f.Namespace") + } + if cfg.Registry != "registry" { + t.Error("apply missing map of f.Registry") + } + + // empty values in the function context should not zero out + // populated values in the global config when applying. + cfg.Apply(fn.Function{}) + if cfg.Builder == "" { + t.Error("empty f.Build.Builder should not be mapped") + } + if cfg.Language == "" { + t.Error("empty f.Runtime should not be mapped") + } + if cfg.Namespace == "" { + t.Error("empty f.Namespace should not be mapped") + } + if cfg.Registry == "" { + t.Error("empty f.Registry should not be mapped") + } +} + +// TestConfigyre ensures that configuring a function results in every member +// of the function in the intersection of the two sets, global config and function +// members, to be set to the values of the config. +// (See the associated cfg.Apply) +func TestConfigure(t *testing.T) { + f := fn.Function{} + cfg := config.Global{ + Builder: "builder", + Language: "runtime", + Namespace: "namespace", + Registry: "registry", + } + f = cfg.Configure(f) + + if f.Build.Builder != "builder" { + t.Error("configure missing map for f.Build.Builder") + } + if f.Deploy.Namespace != "namespace" { + t.Error("configure missing map for f.Deploy.Namespace") + } + if f.Runtime != "runtime" { + t.Error("configure missing map for f.Language") + } + if f.Registry != "registry" { + t.Error("configure missing map for f.Registry") + } + + // empty values in the global config shoul not zero out function values + // when configuring. + f = config.Global{}.Configure(f) + if f.Build.Builder == "" { + t.Error("empty cfg.Builder should not mutate f") + } + if f.Deploy.Namespace == "" { + t.Error("empty cfg.Namespace should not mutate f") + } + if f.Runtime == "" { + t.Error("empty cfg.Runtime should not mutate f") + } + if f.Registry == "" { + t.Error("empty cfg.Registry should not mutate f") + } + +} diff --git a/docs/reference/func_build.md b/docs/reference/func_build.md index 5034c06c..040476b5 100644 --- a/docs/reference/func_build.md +++ b/docs/reference/func_build.md @@ -56,14 +56,13 @@ func build ``` -b, --builder string build strategy to use when creating the underlying image. Currently supported build strategies are "pack" and "s2i". (default "pack") - --builder-image string builder image, either an as a an image name or a mapping name. - Specified value is stored in func.yaml (as 'builder' field) for subsequent builds. ($FUNC_BUILDER_IMAGE) + --builder-image string Specify a custom builder image for use by the builder other than its default. (Env: $FUNC_BUILDER_IMAGE) -c, --confirm Prompt to confirm all configuration options (Env: $FUNC_CONFIRM) -h, --help help for build -i, --image string Full image name in the form [registry]/[namespace]/[name]:[tag] (optional). This option takes precedence over --registry (Env: $FUNC_IMAGE) -p, --path string Path to the project directory. Default is current working directory (Env: $FUNC_PATH) - --platform string Target platform to build (e.g. linux/amd64). - -u, --push Attempt to push the function image after being successfully built + --platform string Optionally specify a target platform, for example "linux/amd64" when using the s2i build strategy + -u, --push Attempt to push the function image to the configured registry after being successfully built -r, --registry string Registry + namespace part of the image to build, ex 'quay.io/myuser'. The full image name is automatically determined (Env: $FUNC_REGISTRY) ```