From fb30c908d30c221217cbd2d18aa0ccce50e45938 Mon Sep 17 00:00:00 2001 From: Luke Kingland Date: Fri, 5 Apr 2024 09:12:38 +0900 Subject: [PATCH] fix: namespace logic cleanup and test isolation - Pulls logic for defaulting to active namespace (K8S) moved UP to CLI during flag default calculation. - Pushes logic of deciding between f.Namespace vs f.Deploy.Namespace down into deployer implementations. - Updates some tests which needed to have their environment cleared. - Removes namespaces as a state variable on various structures, shifting it to an argument. - Moves FromTempDirectory to testing package for use outside cmd. --- cmd/build_test.go | 3 +- cmd/client.go | 20 +- cmd/client_test.go | 26 +- cmd/completion_util.go | 4 +- cmd/config_git_remove.go | 6 +- cmd/config_git_set.go | 8 +- cmd/create_test.go | 13 +- cmd/delete.go | 84 ++--- cmd/delete_test.go | 51 +-- cmd/deploy.go | 47 +-- cmd/deploy_test.go | 325 +++++++++--------- cmd/describe.go | 75 ++-- cmd/describe_test.go | 79 +---- cmd/environment.go | 4 +- cmd/invoke.go | 2 +- cmd/invoke_test.go | 31 +- cmd/languages_test.go | 6 +- cmd/list.go | 55 +-- cmd/list_test.go | 84 +++-- cmd/repository_test.go | 8 +- cmd/root.go | 43 +++ cmd/root_test.go | 96 ++++-- cmd/run_test.go | 3 +- cmd/subscribe_test.go | 11 +- cmd/templates_test.go | 11 +- .../Test_defaultNamespace/func/config.yaml | 1 + .../kubeconfig | 0 cmd/testdata/Test_namespace/kubeconfig | 25 -- pkg/config/config.go | 22 +- pkg/config/config_test.go | 43 +-- pkg/functions/client.go | 189 +++++----- pkg/functions/client_int_test.go | 61 ++-- pkg/functions/client_test.go | 48 +-- pkg/functions/function.go | 4 +- pkg/functions/instances.go | 2 +- pkg/knative/deployer.go | 57 +-- pkg/knative/deployer_test.go | 81 +---- pkg/knative/describer.go | 33 +- pkg/knative/integration_test.go | 12 +- pkg/knative/lister.go | 41 +-- pkg/mock/deployer.go | 20 +- pkg/mock/describer.go | 8 +- pkg/mock/lister.go | 8 +- pkg/mock/pipelines_provider.go | 21 +- pkg/pipelines/tekton/pipelines_provider.go | 8 +- pkg/testing/testing.go | 39 +++ 46 files changed, 877 insertions(+), 941 deletions(-) create mode 100644 cmd/testdata/Test_defaultNamespace/func/config.yaml rename cmd/testdata/{TestDeploy_NamespaceDefaults/TestDeploy_NamespaceRedeployWarning => Test_defaultNamespace}/kubeconfig (100%) delete mode 100644 cmd/testdata/Test_namespace/kubeconfig diff --git a/cmd/build_test.go b/cmd/build_test.go index 812371d4f6..02db1620a0 100644 --- a/cmd/build_test.go +++ b/cmd/build_test.go @@ -6,6 +6,7 @@ import ( fn "knative.dev/func/pkg/functions" "knative.dev/func/pkg/mock" + . "knative.dev/func/pkg/testing" ) // TestBuild_BuilderPersists ensures that the builder chosen is read from @@ -100,7 +101,7 @@ func TestBuild_Authentication(t *testing.T) { // - Push not triggered after an unsuccessful build // - Push can be disabled func TestBuild_Push(t *testing.T) { - root := fromTempDirectory(t) + root := FromTempDirectory(t) f := fn.Function{ Root: root, diff --git a/cmd/client.go b/cmd/client.go index 6ff87bc232..bb6bd96ea6 100644 --- a/cmd/client.go +++ b/cmd/client.go @@ -21,10 +21,15 @@ import ( // These are the minimum settings necessary to create the default client // instance which has most subsystems initialized. type ClientConfig struct { - // Namespace in the remote cluster to use for any client commands which + // PipelinesNamespace in the remote cluster to use for any client commands which // touch the remote. Optional. Empty namespace indicates the namespace // currently configured in the client's connection should be used. - Namespace string + // + // DEPRECATED: This is being removed. Individual commands should use + // either a supplied --namespace flag, the current active k8s context, + // the global default (if defined) or the static default "default", in + // that order. + PipelinesNamespace string // Verbose logging. By default, logging output is kept to the bare minimum. // Use this flag to configure verbose logging throughout. @@ -62,16 +67,16 @@ func NewClient(cfg ClientConfig, options ...fn.Option) (*fn.Client, func()) { var ( t = newTransport(cfg.InsecureSkipVerify) // may provide a custom impl which proxies c = newCredentialsProvider(config.Dir(), t) // for accessing registries - d = newKnativeDeployer(cfg.Namespace, cfg.Verbose) - pp = newTektonPipelinesProvider(cfg.Namespace, c, cfg.Verbose) + d = newKnativeDeployer(cfg.Verbose) + pp = newTektonPipelinesProvider(cfg.PipelinesNamespace, c, cfg.Verbose) o = []fn.Option{ // standard (shared) options for all commands fn.WithVerbose(cfg.Verbose), fn.WithTransport(t), fn.WithRepositoriesPath(config.RepositoriesPath()), fn.WithBuilder(buildpacks.NewBuilder(buildpacks.WithVerbose(cfg.Verbose))), fn.WithRemover(knative.NewRemover(cfg.Verbose)), - fn.WithDescriber(knative.NewDescriber(cfg.Namespace, cfg.Verbose)), - fn.WithLister(knative.NewLister(cfg.Namespace, cfg.Verbose)), + fn.WithDescriber(knative.NewDescriber(cfg.Verbose)), + fn.WithLister(knative.NewLister(cfg.Verbose)), fn.WithDeployer(d), fn.WithPipelinesProvider(pp), fn.WithPusher(docker.NewPusher( @@ -128,9 +133,8 @@ func newTektonPipelinesProvider(namespace string, creds docker.CredentialsProvid return tekton.NewPipelinesProvider(options...) } -func newKnativeDeployer(namespace string, verbose bool) fn.Deployer { +func newKnativeDeployer(verbose bool) fn.Deployer { options := []knative.DeployerOpt{ - knative.WithDeployerNamespace(namespace), knative.WithDeployerVerbose(verbose), knative.WithDeployerDecorator(deployDecorator{}), } diff --git a/cmd/client_test.go b/cmd/client_test.go index b7a63f1b5a..8be224f13c 100644 --- a/cmd/client_test.go +++ b/cmd/client_test.go @@ -8,8 +8,6 @@ import ( "knative.dev/func/pkg/mock" ) -const namespace = "func" - // Test_NewTestClient ensures that the convenience method for // constructing a mocked client for testing properly considers options: // options provided to the factory constructor are considered exaustive, @@ -24,30 +22,34 @@ func Test_NewTestClient(t *testing.T) { // Factory constructor options which should be used when invoking later clientFn := NewTestClient(fn.WithRemover(remover)) + // Factory should ignore options provided when invoking client, _ := clientFn(ClientConfig{}, fn.WithDescriber(describer)) - // Trigger an invocation of the mocks - err := client.Remove(context.Background(), fn.Function{Name: "test", Deploy: fn.DeploySpec{Namespace: namespace}}, true) - if err != nil { - t.Fatal(err) - } - f, err := fn.NewFunction("") + // Trigger an invocation of the mocks by running the associated client + // methods which depend on them + err := client.Remove(context.Background(), "myfunc", "myns", fn.Function{}, true) if err != nil { t.Fatal(err) } - _, err = client.Describe(context.Background(), "test", f) + _, err = client.Describe(context.Background(), "myfunc", "myns", fn.Function{}) if err != nil { t.Fatal(err) } - // Ensure the first set of options, held on the factory, were used + // Ensure the first set of options, held on the factory (the mock remover) + // is invoked. if !remover.RemoveInvoked { t.Fatalf("factory (outer) options not carried through to final client instance") } - // Ensure the second set of options, provided when constructing the - // client using the factory, were ignored + // Ensure the second set of options, provided when constructing the client + // using the factory, are ignored. if describer.DescribeInvoked { t.Fatalf("test client factory should ignore options when invoked.") } + + // This ensures that the NewTestClient function, when provided a set + // of optional implementations (mocks) will override any which are set + // by commands, allowing tests to "force" a command to use the mocked + // implementations. } diff --git a/cmd/completion_util.go b/cmd/completion_util.go index 312abff9da..7a4df6b848 100644 --- a/cmd/completion_util.go +++ b/cmd/completion_util.go @@ -15,9 +15,9 @@ import ( ) func CompleteFunctionList(cmd *cobra.Command, args []string, toComplete string) (strings []string, directive cobra.ShellCompDirective) { - lister := knative.NewLister("", false) + lister := knative.NewLister(false) - list, err := lister.List(cmd.Context()) + list, err := lister.List(cmd.Context(), "") if err != nil { directive = cobra.ShellCompDirectiveError return diff --git a/cmd/config_git_remove.go b/cmd/config_git_remove.go index 0d0c74cb5b..9294aa5710 100644 --- a/cmd/config_git_remove.go +++ b/cmd/config_git_remove.go @@ -69,7 +69,7 @@ type configGitRemoveConfig struct { // working directory of the process. Path string - Namespace string + PipelinesNamespace string // informs whether any specific flag for deleting only a subset of resources has been set flagSet bool @@ -93,7 +93,7 @@ func newConfigGitRemoveConfig(_ *cobra.Command) (c configGitRemoveConfig) { } c = configGitRemoveConfig{ - Namespace: viper.GetString("namespace"), + PipelinesNamespace: viper.GetString("namespace"), flagSet: flagSet, @@ -181,7 +181,7 @@ func runConfigGitRemoveCmd(cmd *cobra.Command, newClient ClientFactory) (err err return } - client, done := newClient(ClientConfig{Namespace: cfg.Namespace, Verbose: cfg.Verbose}) + client, done := newClient(ClientConfig{PipelinesNamespace: cfg.PipelinesNamespace, Verbose: cfg.Verbose}) defer done() return client.RemovePAC(cmd.Context(), f, cfg.metadata) diff --git a/cmd/config_git_set.go b/cmd/config_git_set.go index 8459de6a80..21b01142e6 100644 --- a/cmd/config_git_set.go +++ b/cmd/config_git_set.go @@ -93,7 +93,7 @@ func NewConfigGitSetCmd(newClient ClientFactory) *cobra.Command { type configGitSetConfig struct { buildConfig // further embeds config.Global - Namespace string + PipelinesNamespace string GitProvider string GitURL string @@ -126,8 +126,8 @@ func newConfigGitSetConfig(_ *cobra.Command) (c configGitSetConfig) { } c = configGitSetConfig{ - buildConfig: newBuildConfig(), - Namespace: viper.GetString("namespace"), + buildConfig: newBuildConfig(), + PipelinesNamespace: viper.GetString("namespace"), GitURL: viper.GetString("git-url"), GitRevision: viper.GetString("git-branch"), @@ -307,7 +307,7 @@ func runConfigGitSetCmd(cmd *cobra.Command, newClient ClientFactory) (err error) return } - client, done := newClient(ClientConfig{Namespace: cfg.Namespace, Verbose: cfg.Verbose}, fn.WithRegistry(cfg.Registry)) + client, done := newClient(ClientConfig{PipelinesNamespace: cfg.PipelinesNamespace, Verbose: cfg.Verbose}, fn.WithRegistry(cfg.Registry)) defer done() return client.ConfigurePAC(cmd.Context(), f, cfg.metadata) diff --git a/cmd/create_test.go b/cmd/create_test.go index 9720c2e735..3b44d8d104 100644 --- a/cmd/create_test.go +++ b/cmd/create_test.go @@ -4,13 +4,14 @@ import ( "errors" "testing" + . "knative.dev/func/pkg/testing" "knative.dev/func/pkg/utils" ) // TestCreate_Execute ensures that an invocation of create with minimal settings // and valid input completes without error; degenerate case. func TestCreate_Execute(t *testing.T) { - _ = fromTempDirectory(t) + _ = FromTempDirectory(t) cmd := NewCreateCmd(NewClient) cmd.SetArgs([]string{"--language", "go", "myfunc"}) @@ -23,7 +24,7 @@ func TestCreate_Execute(t *testing.T) { // TestCreate_NoRuntime ensures that an invocation of create must be // done with a runtime. func TestCreate_NoRuntime(t *testing.T) { - _ = fromTempDirectory(t) + _ = FromTempDirectory(t) cmd := NewCreateCmd(NewClient) cmd.SetArgs([]string{"myfunc"}) // Do not use test command args @@ -38,7 +39,7 @@ func TestCreate_NoRuntime(t *testing.T) { // TestCreate_WithNoRuntime ensures that an invocation of create must be // done with one of the valid runtimes only. func TestCreate_WithInvalidRuntime(t *testing.T) { - _ = fromTempDirectory(t) + _ = FromTempDirectory(t) cmd := NewCreateCmd(NewClient) cmd.SetArgs([]string{"--language", "invalid", "myfunc"}) @@ -53,7 +54,7 @@ func TestCreate_WithInvalidRuntime(t *testing.T) { // TestCreate_InvalidTemplate ensures that an invocation of create must be // done with one of the valid templates only. func TestCreate_InvalidTemplate(t *testing.T) { - _ = fromTempDirectory(t) + _ = FromTempDirectory(t) cmd := NewCreateCmd(NewClient) cmd.SetArgs([]string{"--language", "go", "--template", "invalid", "myfunc"}) @@ -68,7 +69,7 @@ func TestCreate_InvalidTemplate(t *testing.T) { // TestCreate_ValidatesName ensures that the create command only accepts // DNS-1123 labels for function name. func TestCreate_ValidatesName(t *testing.T) { - _ = fromTempDirectory(t) + _ = FromTempDirectory(t) // Execute the command with a function name containing invalid characters and // confirm the expected error is returned @@ -84,7 +85,7 @@ func TestCreate_ValidatesName(t *testing.T) { // TestCreate_ConfigOptional ensures that the system can be used without // any additional configuration being required. func TestCreate_ConfigOptional(t *testing.T) { - _ = fromTempDirectory(t) + _ = FromTempDirectory(t) t.Setenv("XDG_CONFIG_HOME", t.TempDir()) diff --git a/cmd/delete.go b/cmd/delete.go index 2834fee839..75cd71aa3b 100644 --- a/cmd/delete.go +++ b/cmd/delete.go @@ -28,7 +28,7 @@ No local files are deleted. {{rootCmdUse}} delete # Undeploy the function 'myfunc' in namespace 'apps' -{{rootCmdUse}} delete -n apps myfunc +{{rootCmdUse}} delete myfunc --namespace apps `, SuggestFor: []string{"remove", "del"}, Aliases: []string{"rm"}, @@ -47,7 +47,7 @@ No local files are deleted. } // Flags - cmd.Flags().StringP("namespace", "n", cfg.Namespace, "The namespace in which to delete. ($FUNC_NAMESPACE)") + cmd.Flags().StringP("namespace", "n", defaultNamespace(fn.Function{}, false), "The namespace when deleting by name. ($FUNC_NAMESPACE)") cmd.Flags().StringP("all", "a", "true", "Delete all resources created for a function, eg. Pipelines, Secrets, etc. ($FUNC_ALL) (allowed values: \"true\", \"false\")") addConfirmFlag(cmd, cfg.Confirm) addPathFlag(cmd) @@ -57,77 +57,63 @@ No local files are deleted. } func runDelete(cmd *cobra.Command, args []string, newClient ClientFactory) (err error) { - cfg, err := newDeleteConfig(args).Prompt() + cfg, err := newDeleteConfig(cmd, args) if err != nil { return } - - // check that name is defined when deleting a Function in specific namespace - if cfg.Name == "" && cfg.Namespace != "" { - return fmt.Errorf("function name is required when namespace is specified") + if cfg, err = cfg.Prompt(); err != nil { + return } - var function fn.Function - // initialize namespace from the config - var namespace = cfg.Namespace + client, done := newClient(ClientConfig{Verbose: cfg.Verbose}) + defer done() - // Initialize func with explicit name (when provided) - if len(args) > 0 && args[0] != "" { - pathChanged := cmd.Flags().Changed("path") - if pathChanged { - return fmt.Errorf("only one of --path and [NAME] should be provided") - } - function = fn.Function{ - Name: args[0], - } - } else { - function, err = fn.NewFunction(cfg.Path) + if cfg.Name != "" { // Delete by name if provided + return client.Remove(cmd.Context(), cfg.Name, cfg.Namespace, fn.Function{}, cfg.All) + } else { // Otherwise; delete the function at path (cwd by default) + f, err := fn.NewFunction(cfg.Path) if err != nil { - return - } - - // Check if the function has been initialized - if !function.Initialized() { - return fn.NewErrNotInitialized(function.Root) - } - - // use the function's extant namespace -- already deployed function - if !cmd.Flags().Changed("namespace") && function.Deploy.Namespace != "" { - namespace = function.Deploy.Namespace + return err } + return client.Remove(cmd.Context(), "", "", f, cfg.All) } - - // Create a client instance from the now-final config - client, done := newClient(ClientConfig{Namespace: namespace, Verbose: cfg.Verbose}) - defer done() - - function.Deploy.Namespace = namespace - // Invoke remove using the concrete client impl - return client.Remove(cmd.Context(), function, cfg.DeleteAll) } type deleteConfig struct { Name string Namespace string Path string - DeleteAll bool + All bool Verbose bool } // newDeleteConfig returns a config populated from the current execution context // (args, flags and environment variables) -func newDeleteConfig(args []string) deleteConfig { +func newDeleteConfig(cmd *cobra.Command, args []string) (cfg deleteConfig, err error) { var name string if len(args) > 0 { name = args[0] } - return deleteConfig{ - Path: viper.GetString("path"), + cfg = deleteConfig{ + All: viper.GetBool("all"), + Name: name, // args[0] or derived Namespace: viper.GetString("namespace"), - DeleteAll: viper.GetBool("all"), - Name: deriveName(name, viper.GetString("path")), // args[0] or derived - Verbose: viper.GetBool("verbose"), // defined on root + Path: viper.GetString("path"), + Verbose: viper.GetBool("verbose"), // defined on root + } + if cfg.Name == "" && cmd.Flags().Changed("namespace") { + // logicially inconsistent to supply only a namespace. + // Either use the function's local state in its entirety, or specify + // both a name and a namespace to ignore any local function source. + err = fmt.Errorf("must also specify a name when specifying namespace.") + } + if cfg.Name != "" && cmd.Flags().Changed("path") { + // logically inconsistent to provide both a name and a path to source. + // Either use the function's local state on disk (--path), or specify + // a name and a namespace to ignore any local function source. + err = fmt.Errorf("only one of --path and [NAME] should be provided") } + return } // Prompt the user with value of config members, allowing for interaractive changes. @@ -151,7 +137,7 @@ func (c deleteConfig) Prompt() (deleteConfig, error) { Name: "all", Prompt: &survey.Confirm{ Message: "Do you want to delete all resources?", - Default: c.DeleteAll, + Default: c.All, }, }, } @@ -166,7 +152,7 @@ func (c deleteConfig) Prompt() (deleteConfig, error) { } dc.Name = answers.Name - dc.DeleteAll = answers.All + dc.All = answers.All return dc, err } diff --git a/cmd/delete_test.go b/cmd/delete_test.go index 50f3ad8233..838ac4a9cc 100644 --- a/cmd/delete_test.go +++ b/cmd/delete_test.go @@ -7,46 +7,52 @@ import ( fn "knative.dev/func/pkg/functions" "knative.dev/func/pkg/mock" + . "knative.dev/func/pkg/testing" ) // TestDelete_Default ensures that the deployed function is deleted correctly -// with default options +// with default options and the default situation: running "delete" from +// within the same directory of the function which is to be deleted. func TestDelete_Default(t *testing.T) { var ( - root = fromTempDirectory(t) - namespace = "myns" - remover = mock.NewRemover() err error + root = FromTempDirectory(t) + name = "myfunc" + namespace = "testns" + remover = mock.NewRemover() + ctx = context.Background() ) - remover.RemoveFn = func(_, ns string) error { + // Remover which confirms the name and namespace received are those + // originally requested via the CLI flags. + remover.RemoveFn = func(n, ns string) error { + if n != name { + t.Errorf("expected name '%v', got '%v'", name, n) + } if ns != namespace { - t.Fatalf("expected delete namespace '%v', got '%v'", namespace, ns) + t.Errorf("expected namespace '%v', got '%v'", namespace, ns) } return nil } - // Ensure the extant function's namespace is used + // A function which will be created in the requested namespace f := fn.Function{ - Root: root, - Runtime: "go", - Registry: TestRegistry, - Name: "testname", - Deploy: fn.DeploySpec{ - Namespace: namespace, //simulate deployed Function - }, + Runtime: "go", + Name: name, + Namespace: namespace, + Root: root, + Registry: TestRegistry, } - if f, err = fn.New().Init(f); err != nil { + if _, f, err = fn.New().New(ctx, f); err != nil { t.Fatal(err) } - if err = f.Write(); err != nil { t.Fatal(err) } cmd := NewDeleteCmd(NewTestClient(fn.WithRemover(remover))) - cmd.SetArgs([]string{}) //dont give any arguments to 'func delete' -- default + cmd.SetArgs([]string{}) if err := cmd.Execute(); err != nil { t.Fatal(err) } @@ -61,7 +67,7 @@ func TestDelete_Default(t *testing.T) { // function explicitly as an argument invokes the remover appropriately. func TestDelete_ByName(t *testing.T) { var ( - root = fromTempDirectory(t) + root = FromTempDirectory(t) testname = "testname" // explicit name for the function testnamespace = "testnamespace" // explicit namespace for the function remover = mock.NewRemover() // with a mock remover @@ -98,7 +104,6 @@ func TestDelete_ByName(t *testing.T) { // with a mocked remover. cmd := NewDeleteCmd(NewTestClient(fn.WithRemover(remover))) cmd.SetArgs([]string{testname}) // run: func delete - if err := cmd.Execute(); err != nil { t.Fatal(err) } @@ -141,7 +146,7 @@ func TestDelete_Namespace(t *testing.T) { // ignores the the function on disk func TestDelete_NamespaceFlagPriority(t *testing.T) { var ( - root = fromTempDirectory(t) + root = FromTempDirectory(t) namespace = "myns" namespace2 = "myns2" remover = mock.NewRemover() @@ -184,7 +189,7 @@ func TestDelete_NamespaceFlagPriority(t *testing.T) { // TestDelete_NamespaceWithoutNameFails ensures that providing wrong argument // combination fails nice and fast (no name of the Function) func TestDelete_NamespaceWithoutNameFails(t *testing.T) { - _ = fromTempDirectory(t) + _ = FromTempDirectory(t) cmd := NewDeleteCmd(NewTestClient()) cmd.SetArgs([]string{"--namespace=myns"}) @@ -196,7 +201,7 @@ func TestDelete_NamespaceWithoutNameFails(t *testing.T) { // TestDelete_ByProject ensures that running delete with a valid project as its // context invokes remove and with the correct name (reads name from func.yaml) func TestDelete_ByProject(t *testing.T) { - _ = fromTempDirectory(t) + _ = FromTempDirectory(t) // Write a func.yaml config which specifies a name funcYaml := `name: bar @@ -248,7 +253,7 @@ func TestDelete_ByPath(t *testing.T) { // A mock remover which will be sampled to ensure it is not invoked. remover = mock.NewRemover() - root = fromTempDirectory(t) + root = FromTempDirectory(t) err error namespace = "func" ) diff --git a/cmd/deploy.go b/cmd/deploy.go index 93610d8db3..57089c20ad 100644 --- a/cmd/deploy.go +++ b/cmd/deploy.go @@ -153,8 +153,6 @@ EXAMPLES cmd.Flags().StringP("registry", "r", cfg.Registry, "Container registry + registry namespace. (ex 'ghcr.io/myuser'). The full image name is automatically determined using this along with function name. ($FUNC_REGISTRY)") cmd.Flags().Bool("registry-insecure", cfg.RegistryInsecure, "Disable HTTPS when communicating to the registry ($FUNC_REGISTRY_INSECURE)") - cmd.Flags().StringP("namespace", "n", cfg.Namespace, - "Deploy into a specific namespace. Will use function's current namespace by default if already deployed, and the currently active namespace if it can be determined. ($FUNC_NAMESPACE)") // Function-Context Flags: // Options whose value is available on the function with context only @@ -200,6 +198,8 @@ EXAMPLES cmd.Flags().StringP("token", "", "", "Token to use when pushing to the registry.") cmd.Flags().BoolP("build-timestamp", "", false, "Use the actual time as the created time for the docker image. This is only useful for buildpacks builder.") + cmd.Flags().StringP("namespace", "n", defaultNamespace(f, false), + "Deploy into a specific namespace. Will use the function's current namespace by default if already deployed, and the currently active context if it can be determined. ($FUNC_NAMESPACE)") // Temporarily Hidden Basic Auth Flags // Username, Password and Token flags, which plumb through basic auth, are @@ -253,30 +253,36 @@ func runDeploy(cmd *cobra.Command, newClient ClientFactory) (err error) { } cmd.SetContext(cfg.WithValues(cmd.Context())) // Some optional settings are passed via context - // If using Openshift registry AND redeploying Function, update image registry - if f.Namespace != "" && f.Namespace != f.Deploy.Namespace && f.Deploy.Namespace != "" { - // when running openshift, namespace is tied to registry, override on --namespace change - // The most default part of registry (in buildConfig) checks 'k8s.IsOpenShift()' and if true, - // sets default registry by current namespace - - // If Function is being moved to different namespace in Openshift -- update registry - if k8s.IsOpenShift() { - // this name is based of k8s package - f.Registry = "image-registry.openshift-image-registry.svc:5000/" + f.Namespace - if cfg.Verbose { - fmt.Fprintf(cmd.OutOrStdout(), "Info: Overriding openshift registry to %s\n", f.Registry) - } + changingNamespace := func(f fn.Function) bool { + // We're changing namespace if: + return f.Deploy.Namespace != "" && // it's already deployed + f.Namespace != "" && // a specific (new) namespace is requested + (f.Namespace != f.Deploy.Namespace) // and it's different + } + + // If we're changing namespace in an OpenShift cluster, we have to + // also update the registry because there is a registry per namespace, + // and their name includes the namespace. + // This saves needing a manual flag ``--registyry={destination namespace registry}`` + if changingNamespace(f) && k8s.IsOpenShift() { + // TODO(lkingland): this appears to force use of the openshift + // internal registry. + f.Registry = "image-registry.openshift-image-registry.svc:5000/" + f.Namespace + if cfg.Verbose { + fmt.Fprintf(cmd.OutOrStdout(), "Info: Overriding openshift registry to %s\n", f.Registry) } } // Informative non-error messages regarding the final deployment request printDeployMessages(cmd.OutOrStdout(), f) + // Get options based on the value of the config such as concrete impls + // of builders and pushers based on the value of the --builder flag clientOptions, err := cfg.clientOptions() if err != nil { return } - client, done := newClient(ClientConfig{Namespace: f.Namespace, Verbose: cfg.Verbose}, clientOptions...) + client, done := newClient(ClientConfig{Verbose: cfg.Verbose}, clientOptions...) defer done() // Deploy @@ -503,8 +509,8 @@ type deployConfig struct { // newDeployConfig creates a buildConfig populated from command flags and // environment variables; in that precedence. -func newDeployConfig(cmd *cobra.Command) (c deployConfig) { - c = deployConfig{ +func newDeployConfig(cmd *cobra.Command) deployConfig { + cfg := deployConfig{ buildConfig: newBuildConfig(), Build: viper.GetString("build"), Env: viper.GetStringSlice("env"), @@ -522,10 +528,11 @@ func newDeployConfig(cmd *cobra.Command) (c deployConfig) { // results and appears to be an open issue since 2017: // https://github.com/spf13/viper/issues/380 var err error - if c.Env, err = cmd.Flags().GetStringArray("env"); err != nil { + if cfg.Env, err = cmd.Flags().GetStringArray("env"); err != nil { fmt.Fprintf(cmd.OutOrStdout(), "error reading envs: %v", err) } - return + + return cfg } // Configure the given function. Updates a function struct with all diff --git a/cmd/deploy_test.go b/cmd/deploy_test.go index 05f5cae4f5..8d34446fb7 100644 --- a/cmd/deploy_test.go +++ b/cmd/deploy_test.go @@ -13,13 +13,11 @@ import ( "github.com/ory/viper" "github.com/spf13/cobra" - apiErrors "k8s.io/apimachinery/pkg/api/errors" - "k8s.io/apimachinery/pkg/runtime/schema" "knative.dev/func/pkg/builders" - "knative.dev/func/pkg/config" fn "knative.dev/func/pkg/functions" "knative.dev/func/pkg/k8s" "knative.dev/func/pkg/mock" + . "knative.dev/func/pkg/testing" ) // commandConstructor is used to share test implementations between commands @@ -35,7 +33,7 @@ func TestDeploy_BuilderPersists(t *testing.T) { func testBuilderPersists(cmdFn commandConstructor, t *testing.T) { t.Helper() - root := fromTempDirectory(t) + root := FromTempDirectory(t) if _, err := fn.New().Init(fn.Function{Runtime: "go", Root: root}); err != nil { t.Fatal(err) @@ -123,7 +121,7 @@ func TestDeploy_BuilderValidated(t *testing.T) { func testBuilderValidated(cmdFn commandConstructor, t *testing.T) { t.Helper() - root := fromTempDirectory(t) + root := FromTempDirectory(t) if _, err := fn.New().Init(fn.Function{Runtime: "go", Root: root}); err != nil { t.Fatal(err) @@ -149,7 +147,7 @@ func testConfigApplied(cmdFn commandConstructor, t *testing.T) { var ( err error home = fmt.Sprintf("%s/testdata/TestX_ConfigApplied", cwd()) - root = fromTempDirectory(t) + root = FromTempDirectory(t) f = fn.Function{Runtime: "go", Root: root, Name: "f"} pusher = mock.NewPusher() clientFn = NewTestClient(fn.WithPusher(pusher)) @@ -235,7 +233,7 @@ func testConfigPrecedence(cmdFn commandConstructor, t *testing.T) { // 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) + root := FromTempDirectory(t) t.Setenv("XDG_CONFIG_HOME", home) // sets registry.example.com/global f := fn.Function{Runtime: "go", Root: root, Name: "f"} if f, err = fn.New().Init(f); err != nil { @@ -252,7 +250,7 @@ func testConfigPrecedence(cmdFn commandConstructor, t *testing.T) { } // Ensure Global Config applied via config in ./testdata - root = fromTempDirectory(t) + root = FromTempDirectory(t) t.Setenv("XDG_CONFIG_HOME", home) // sets registry.example.com/global f = fn.Function{Runtime: "go", Root: root, Name: "f"} f, err = fn.New().Init(f) @@ -272,7 +270,7 @@ func testConfigPrecedence(cmdFn commandConstructor, t *testing.T) { // 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) + 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"} @@ -291,7 +289,7 @@ func testConfigPrecedence(cmdFn commandConstructor, t *testing.T) { } // Ensure Environment Variable overrides function context. - root = fromTempDirectory(t) + 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", @@ -311,7 +309,7 @@ func testConfigPrecedence(cmdFn commandConstructor, t *testing.T) { } // Ensure flags override environment variables. - root = fromTempDirectory(t) + 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", @@ -340,7 +338,7 @@ func TestDeploy_Default(t *testing.T) { } func testDefault(cmdFn commandConstructor, t *testing.T) { - root := fromTempDirectory(t) + root := FromTempDirectory(t) // A Function with the minimum required values for deployment populated. f := fn.Function{ @@ -370,7 +368,7 @@ func testDefault(cmdFn commandConstructor, t *testing.T) { // - Flags provided with the minus '-' suffix are treated as a removal func TestDeploy_Envs(t *testing.T) { var ( - root = fromTempDirectory(t) + root = FromTempDirectory(t) ptr = func(s string) *string { return &s } // TODO: remove pointers from Envs. f fn.Function cmd *cobra.Command @@ -462,7 +460,7 @@ func TestDeploy_FunctionContext(t *testing.T) { func testFunctionContext(cmdFn commandConstructor, t *testing.T) { t.Helper() - root := fromTempDirectory(t) + root := FromTempDirectory(t) f, err := fn.New().Init(fn.Function{Runtime: "go", Root: root, Registry: TestRegistry}) if err != nil { @@ -515,7 +513,7 @@ func testFunctionContext(cmdFn commandConstructor, t *testing.T) { // TestDeploy_GitArgsPersist ensures that the git flags, if provided, are // persisted to the Function for subsequent deployments. func TestDeploy_GitArgsPersist(t *testing.T) { - root := fromTempDirectory(t) + root := FromTempDirectory(t) var ( url = "https://example.com/user/repo" @@ -558,7 +556,7 @@ func TestDeploy_GitArgsPersist(t *testing.T) { // TestDeploy_GitArgsUsed ensures that any git values provided as flags are used // when invoking a remote deployment. func TestDeploy_GitArgsUsed(t *testing.T) { - root := fromTempDirectory(t) + root := FromTempDirectory(t) var ( url = "https://example.com/user/repo" @@ -602,7 +600,7 @@ func TestDeploy_GitArgsUsed(t *testing.T) { // TestDeploy_GitURLBranch ensures that a --git-url which specifies the branch // in the URL is equivalent to providing --git-branch func TestDeploy_GitURLBranch(t *testing.T) { - root := fromTempDirectory(t) + root := FromTempDirectory(t) f, err := fn.New().Init(fn.Function{Runtime: "go", Root: root}) if err != nil { @@ -648,7 +646,7 @@ func TestDeploy_ImageAndRegistry(t *testing.T) { func testImageAndRegistry(cmdFn commandConstructor, t *testing.T) { t.Helper() - root := fromTempDirectory(t) + root := FromTempDirectory(t) _, err := fn.New().Init(fn.Function{Runtime: "go", Root: root}) if err != nil { @@ -721,7 +719,7 @@ func testImageFlag(cmdFn commandConstructor, t *testing.T) { args = []string{"--image", "docker.io/tigerteam/foo"} builder = mock.NewBuilder() ) - root := fromTempDirectory(t) + root := FromTempDirectory(t) _, err := fn.New().Init(fn.Function{Runtime: "go", Root: root}) if err != nil { @@ -792,7 +790,7 @@ func TestDeploy_ImageWithDigestErrors(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { // Move into a new temp directory - root := fromTempDirectory(t) + root := FromTempDirectory(t) // Create a new Function in the temp directory _, err := fn.New().Init(fn.Function{Runtime: "go", Root: root}) @@ -842,7 +840,7 @@ func TestDeploy_ImageWithDigestErrors(t *testing.T) { // should happen; f.Deploy.Image should be populated because the image should // just be deployed as is (since it already has digest) func TestDeploy_ImageWithDigestDoesntPopulateBuild(t *testing.T) { - root := fromTempDirectory(t) + root := FromTempDirectory(t) // image with digest (well almost, atleast in length and syntax) const img = "docker.io/4141gauron3268@sha256:XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX" // Create a new Function in the temp directory @@ -876,7 +874,7 @@ func TestDeploy_InvalidRegistry(t *testing.T) { func testInvalidRegistry(cmdFn commandConstructor, t *testing.T) { t.Helper() - root := fromTempDirectory(t) + root := FromTempDirectory(t) f := fn.Function{ Root: root, @@ -900,11 +898,13 @@ func testInvalidRegistry(cmdFn commandConstructor, t *testing.T) { // TestDeploy_Namespace ensures that the namespace provided to the client // for use when describing a function is set -// 1. The flag /env variable if provided +// 1. The user's current active namespace by default // 2. The namespace of the function at path if provided -// 3. The user's current active namespace +// 3. The flag /env variable if provided has highest precedence func TestDeploy_Namespace(t *testing.T) { - root := fromTempDirectory(t) + root := FromTempDirectory(t) // clears most envs, sets test KUBECONFIG + + testClientFn := NewTestClient(fn.WithDeployer(mock.NewDeployer())) // A function which will be repeatedly, mockingly deployed f := fn.Function{Root: root, Runtime: "go", Registry: TestRegistry} @@ -913,107 +913,116 @@ func TestDeploy_Namespace(t *testing.T) { t.Fatal(err) } - // The mock deployer responds that the given function was deployed - // to the namespace indicated in f.Deploy.Namespace or "default" if empty - // (it does not actually consider the current kubernetes context) - deployer := mock.NewDeployer() - - cmd := NewDeployCmd(NewTestClient(fn.WithDeployer(deployer))) + // Deploy it to the default namespace, which is taken from + // the test KUBECONFIG found in testdata/default_kubeconfig + cmd := NewDeployCmd(testClientFn) cmd.SetArgs([]string{}) if err := cmd.Execute(); err != nil { t.Fatal(err) } f, _ = fn.NewFunction(root) - if f.Deploy.Namespace != "default" { - t.Fatalf("expected namespace 'default', got '%v'", f.Deploy.Namespace) + if f.Deploy.Namespace != "func" { + t.Fatalf("expected namespace 'func', got '%v'", f.Deploy.Namespace) } - // Change the function's active namespace and ensure it is used, preempting - // the 'default' namespace from the mock - f, err = fn.NewFunction(root) - if err != nil { + // Deploy to a new namespace + cmd = NewDeployCmd(testClientFn) + cmd.SetArgs([]string{"--namespace=newnamespace"}) + if err := cmd.Execute(); err != nil { t.Fatal(err) } - f.Deploy.Namespace = "alreadyDeployed" - if err := f.Write(); err != nil { - t.Fatal(err) + f, _ = fn.NewFunction(root) + if f.Deploy.Namespace != "newnamespace" { + t.Fatalf("expected namespace 'newnamespace', got '%v'", f.Deploy.Namespace) } - cmd = NewDeployCmd(NewTestClient(fn.WithDeployer(deployer))) + + // Redeploy and confirm it retains being in the new namespace + // (does not revert to using the static default "default" nor the + // current context's "func") + cmd = NewDeployCmd(testClientFn) cmd.SetArgs([]string{}) if err := cmd.Execute(); err != nil { t.Fatal(err) } f, _ = fn.NewFunction(root) - if f.Deploy.Namespace != "alreadyDeployed" { - t.Fatalf("expected namespace 'alreadyDeployed', got '%v'", f.Deploy.Namespace) + if f.Deploy.Namespace != "newnamespace" { + t.Fatalf("expected deploy to retain namespace 'newnamespace', got '%v'", f.Deploy.Namespace) } // Ensure an explicit name (a flag) is taken with highest precedence - cmd = NewDeployCmd(NewTestClient(fn.WithDeployer(deployer))) - cmd.SetArgs([]string{"--namespace=newNamespace"}) + // overriding both the default and the "previously deployed" value. + cmd = NewDeployCmd(testClientFn) + cmd.SetArgs([]string{"--namespace=thirdnamespace"}) if err := cmd.Execute(); err != nil { t.Fatal(err) } f, _ = fn.NewFunction(root) - if f.Deploy.Namespace != "newNamespace" { + if f.Deploy.Namespace != "thirdnamespace" { t.Fatalf("expected namespace 'newNamespace', got '%v'", f.Deploy.Namespace) } - } -// TestDeploy_NamespaceDefaults ensures that when not specified, a users's -// active kubernetes context is used for the namespace if available. -func TestDeploy_NamespaceDefaults(t *testing.T) { +// TestDeploy_NamespaceDefaultsToK8sContext ensures that when not specified, a +// users's active kubernetes context is used for the namespace if available. +func TestDeploy_NamespaceDefaultsToK8sContext(t *testing.T) { kubeconfig := filepath.Join(cwd(), "testdata", "TestDeploy_NamespaceDefaults/kubeconfig") expected := "mynamespace" - root := fromTempDirectory(t) // clears envs and cds to empty root + root := FromTempDirectory(t) // clears envs and cds to empty root t.Setenv("KUBECONFIG", kubeconfig) // Create a new function - f, err := fn.New().Init(fn.Function{Runtime: "go", Root: root}) + f, err := fn.New().Init(fn.Function{Runtime: "go", Root: root, Registry: TestRegistry}) if err != nil { t.Fatal(err) } + // Defensive state check if f.Deploy.Namespace != "" { t.Fatalf("newly created functions should not have a namespace set until deployed. Got '%v'", f.Deploy.Namespace) } // a deployer which actually uses config.DefaultNamespace // This is not the default implementation of mock.NewDeployer as this would - // be likely to be confusing, since it would vary on developer machines + // likely be confusing because it would vary on developer machines // unless they remember to clear local envs, such as is done here within // fromTempDirectory(t). To avert this potential confusion, the use of // config.DefaultNamespace() is kept local to this test. deployer := mock.NewDeployer() deployer.DeployFn = func(_ context.Context, f fn.Function) (result fn.DeploymentResult, err error) { - // deployer implementations shuld have integration tests which confirm - // this logic: - if f.Deploy.Namespace != "" { - result.Namespace = f.Deploy.Namespace - } else { - result.Namespace = config.DefaultNamespace() - } + // Mock confirmation the function was deployed to the namespace requested + // This test is a new, single-deploy, so f.Namespace + // (not f.Deploy.Namespace) is to be used + result.Namespace = f.Namespace return + + // NOTE: The below logic is expected of all deployers at this time, + // but is not necessary for this test. + // Deployer implementations should have integration tests which confirm + // this minimim namespace resolution logic is respected: + /* + if f.Namespace != "" { + // We deployed to the requested namespace + result.Namespace = f.Namespace + } else if f.Deploy.Namespace != "" { + // We redeployed to the currently deployed namespace + result.Namespace = f.Namespace + } else { + // We deployed to the default namespace, considering both + // active kube context, global config and the static default. + result.Namespace = defaultNamespace(false) + } + return + */ } - // New deploy command that will not actually deploy or build (mocked) - cmd := NewDeployCmd(NewTestClient( - fn.WithDeployer(deployer), - fn.WithBuilder(mock.NewBuilder()), - fn.WithPipelinesProvider(mock.NewPipelinesProvider()), - fn.WithRegistry(TestRegistry), - )) + // Execute a deploycommand with everything mocked + cmd := NewDeployCmd(NewTestClient(fn.WithDeployer(deployer))) cmd.SetArgs([]string{}) - - // Execute, capturing stderr - stderr := strings.Builder{} - cmd.SetErr(&stderr) if err := cmd.Execute(); err != nil { t.Fatal(err) } - // Assert the function has been updated to be in namespace from the profile + // Assert the function has been updated f, err = fn.NewFunction(root) if err != nil { t.Fatal(err) @@ -1033,7 +1042,7 @@ func TestDeploy_NamespaceDefaults(t *testing.T) { func TestDeploy_NamespaceRedeployWarning(t *testing.T) { // Change profile to one whose current profile is 'test-ns-deploy' kubeconfig := filepath.Join(cwd(), "testdata", "TestDeploy_NamespaceRedeployWarning/kubeconfig") - root := fromTempDirectory(t) + root := FromTempDirectory(t) t.Setenv("KUBECONFIG", kubeconfig) // Create a Function which appears to have been deployed to 'funcns' @@ -1049,8 +1058,6 @@ func TestDeploy_NamespaceRedeployWarning(t *testing.T) { // Redeploy the function without specifying namespace. cmd := NewDeployCmd(NewTestClient( - fn.WithDeployer(mock.NewDeployer()), - fn.WithBuilder(mock.NewBuilder()), fn.WithPipelinesProvider(mock.NewPipelinesProvider()), fn.WithRegistry(TestRegistry), )) @@ -1084,7 +1091,7 @@ func TestDeploy_NamespaceRedeployWarning(t *testing.T) { // Also implicitly checks that the --namespace flag takes precedence over // the namespace of a previously deployed Function. func TestDeploy_NamespaceUpdateWarning(t *testing.T) { - root := fromTempDirectory(t) + root := FromTempDirectory(t) // Create a Function which appears to have been deployed to 'myns' f := fn.Function{ @@ -1144,12 +1151,13 @@ func TestDeploy_NamespaceUpdateWarning(t *testing.T) { // TestDeploy_BasicRedeploy simply ensures that redeploy works and doesnt brake // using standard deploy method when desired namespace is deleted. func TestDeploy_BasicRedeployInCorrectNamespace(t *testing.T) { - root := fromTempDirectory(t) + root := FromTempDirectory(t) + expected := "testnamespace" - // Create a Function which appears to have been deployed to 'myns' f := fn.Function{ - Runtime: "go", - Root: root, + Runtime: "go", + Root: root, + Registry: TestRegistry, } f, err := fn.New().Init(f) if err != nil { @@ -1157,34 +1165,25 @@ func TestDeploy_BasicRedeployInCorrectNamespace(t *testing.T) { } // Redeploy the function, specifying 'newns' - cmd := NewDeployCmd(NewTestClient( - fn.WithDeployer(mock.NewDeployer()), - fn.WithRegistry(TestRegistry), - )) - - cmd.SetArgs([]string{"--namespace=mydns"}) + cmd := NewDeployCmd(NewTestClient(fn.WithDeployer(mock.NewDeployer()))) + cmd.SetArgs([]string{"--namespace", expected}) if err := cmd.Execute(); err != nil { t.Fatal(err) } f, _ = fn.NewFunction(root) - if f.Deploy.Namespace == "" { - t.Fatal("expected deployed namespace to be specified after deploy") + if f.Deploy.Namespace != expected { + t.Fatalf("expected deployed namespace %q, got %q", expected, f.Deploy.Namespace) } // get rid of desired namespace -- should still deploy as usual, now taking // the "already deployed" namespace - cmd.SetArgs([]string{"--namespace="}) + cmd.SetArgs([]string{}) if err = cmd.Execute(); err != nil { t.Fatal(err) } - f, _ = fn.NewFunction(root) - if f.Namespace != "" { - t.Fatalf("no desired namespace should be specified but is %s", f.Namespace) - } - - if f.Deploy.Namespace == "" { + if f.Deploy.Namespace != expected { t.Fatal("expected deployed namespace to be specified after second deploy") } } @@ -1192,11 +1191,13 @@ func TestDeploy_BasicRedeployInCorrectNamespace(t *testing.T) { // TestDeploy_BasicRedeployPipelines simply ensures that deploy 2 times works // and doesnt brake using pipelines func TestDeploy_BasicRedeployPipelinesCorrectNamespace(t *testing.T) { - root := fromTempDirectory(t) + root := FromTempDirectory(t) + // Create a Function which appears to have been deployed to 'myns' f := fn.Function{ - Runtime: "go", - Root: root, + Runtime: "go", + Root: root, + Registry: TestRegistry, } f, err := fn.New().Init(f) if err != nil { @@ -1207,7 +1208,6 @@ func TestDeploy_BasicRedeployPipelinesCorrectNamespace(t *testing.T) { cmd := NewDeployCmd(NewTestClient( fn.WithBuilder(mock.NewBuilder()), fn.WithPipelinesProvider(mock.NewPipelinesProvider()), - fn.WithRegistry(TestRegistry), )) cmd.SetArgs([]string{"--remote", "--namespace=myfuncns"}) @@ -1227,7 +1227,7 @@ func TestDeploy_BasicRedeployPipelinesCorrectNamespace(t *testing.T) { f, _ = fn.NewFunction(root) if f.Namespace != "" { - t.Fatal("desired ns should be empty") + t.Fatalf("expected empty f.Namespace , got %q", f.Namespace) } if f.Deploy.Namespace != "myfuncns" { t.Fatalf("deployed ns should NOT have changed but is '%s'\n", f.Deploy.Namespace) @@ -1299,7 +1299,7 @@ func testRegistry(cmdFn commandConstructor, t *testing.T) { } for _, test := range tests { t.Run(test.name, func(t *testing.T) { - root := fromTempDirectory(t) + root := FromTempDirectory(t) test.f.Runtime = "go" test.f.Name = "f" f, err := fn.New().Init(test.f) @@ -1334,7 +1334,7 @@ func TestDeploy_RegistryLoads(t *testing.T) { func testRegistryLoads(cmdFn commandConstructor, t *testing.T) { t.Helper() - root := fromTempDirectory(t) + root := FromTempDirectory(t) f := fn.Function{ Root: root, @@ -1374,7 +1374,7 @@ func TestDeploy_RegistryOrImageRequired(t *testing.T) { func testRegistryOrImageRequired(cmdFn commandConstructor, t *testing.T) { t.Helper() - root := fromTempDirectory(t) + root := FromTempDirectory(t) _, err := fn.New().Init(fn.Function{Runtime: "go", Root: root}) if err != nil { @@ -1414,7 +1414,7 @@ func testAuthentication(cmdFn commandConstructor, t *testing.T) { // the system and credential helpers (Docker, ecs, acs) t.Helper() - root := fromTempDirectory(t) + root := FromTempDirectory(t) _, err := fn.New().Init(fn.Function{Runtime: "go", Root: root, Registry: TestRegistry}) if err != nil { t.Fatal(err) @@ -1500,7 +1500,7 @@ func TestDeploy_RemoteBuildURLPermutations(t *testing.T) { // returns a single test function for one possible permutation of the flags. newTestFn := func(remote, build, url string) func(t *testing.T) { return func(t *testing.T) { - root := fromTempDirectory(t) + root := FromTempDirectory(t) // Create a new Function in the temp directory if _, err := fn.New().Init(fn.Function{Runtime: "go", Root: root}); err != nil { @@ -1620,7 +1620,7 @@ func TestDeploy_RemoteBuildURLPermutations(t *testing.T) { // TestDeploy_RemotePersists ensures that the remote field is read from // the function by default, and is able to be overridden by flags/env vars. func TestDeploy_RemotePersists(t *testing.T) { - root := fromTempDirectory(t) + root := FromTempDirectory(t) f, err := fn.New().Init(fn.Function{Runtime: "node", Root: root}) if err != nil { @@ -1682,7 +1682,7 @@ func TestDeploy_RemotePersists(t *testing.T) { // line causes the pertinent value to be zeroed out. func TestDeploy_UnsetFlag(t *testing.T) { // From a temp directory - root := fromTempDirectory(t) + root := FromTempDirectory(t) // Create a function f := fn.Function{Runtime: "go", Root: root, Registry: TestRegistry} @@ -1746,7 +1746,7 @@ func Test_ValidateBuilder(t *testing.T) { // TestReDeploy_OnRegistryChange tests that after deployed image with registry X, // subsequent deploy with registry Y triggers build func TestReDeploy_OnRegistryChange(t *testing.T) { - root := fromTempDirectory(t) + root := FromTempDirectory(t) // Create a basic go Function f := fn.Function{ @@ -1795,72 +1795,78 @@ func TestReDeploy_OnRegistryChange(t *testing.T) { // TestReDeploy_OnRegistryChangeWithBuildFalse should fail with function not // being built because the registry has changed func TestReDeploy_OnRegistryChangeWithBuildFalse(t *testing.T) { - root := fromTempDirectory(t) + root := FromTempDirectory(t) + registry := "example.com/bob" - // Create a basic go Function f := fn.Function{ - Runtime: "go", - Root: root, + Runtime: "go", + Root: root, + Registry: TestRegistry, } - _, err := fn.New().Init(f) + _, f, err := fn.New().New(context.Background(), f) if err != nil { t.Fatal(err) } - - // create build cmd - cmdBuild := NewBuildCmd(NewTestClient(fn.WithBuilder(mock.NewBuilder()))) - cmdBuild.SetArgs([]string{"--registry=" + TestRegistry}) - - // First: prebuild Function - if err := cmdBuild.Execute(); err != nil { + if err := f.Write(); err != nil { t.Fatal(err) } - // change registry and deploy again - newRegistry := "example.com/fred" - - cmd := NewDeployCmd(NewTestClient( - fn.WithDeployer(mock.NewDeployer()), - )) - - cmd.SetArgs([]string{"--registry=" + newRegistry, "--build=false"}) - - // Second: Deploy with different registry and expect 'not built' error because - // registry has changed but build is disabled + // Deploying again with a new registry should trigger "not built" error + // if specifying --build=false + cmd := NewDeployCmd(NewTestClient()) + cmd.SetArgs([]string{"--registry", registry, "--build=false"}) if err := cmd.Execute(); err == nil { - t.Fatal(err) + // TODO: should be a typed error which can be checked. This will + // succeed even if an unrelated error is thrown. + t.Fatalf("expected error 'not built' not received") } - // ASSERT - expectF, err := fn.NewFunction(root) - if err != nil { - t.Fatal("couldnt load function from path") + // Assert that the requested change did not take effect + if f, err = fn.NewFunction(root); err != nil { + t.Fatal(err) } - - if !strings.Contains(expectF.Build.Image, TestRegistry) { + if strings.Contains(f.Build.Image, registry) { t.Fatal("expected registry to NOT change since --build=false") } } -// TestDeploy_NoErrorOnOldFunctionNotFound assures that no error is given when old Function's -// service is not available (is already deleted manually or the namespace doesnt exist etc.) +// TestDeploy_NoErrorOnOldFunctionNotFound assures that no error is given when +// old Function's service is not available (is already deleted manually or the +// namespace doesnt exist etc.) func TestDeploy_NoErrorOnOldFunctionNotFound(t *testing.T) { var ( - root = fromTempDirectory(t) + root = FromTempDirectory(t) nsOne = "nsone" nsTwo = "nstwo" remover = mock.NewRemover() ) - // Simulate remover error + // A remover which can not find the old instance remover.RemoveFn = func(n, ns string) error { - return apiErrors.NewNotFound(schema.GroupResource{Group: "", Resource: "Namespace"}, nsOne) + // Note that the knative remover explicitly checks for + // if it received an apiErrors.IsNotFound(err) and if so returns + // a fn.ErrFunctionNotFound. This test implementation is dependent + // on that. This is a change from the original implementation which + // directly returned a knative erorr with: + // return apiErrors.NewNotFound(schema.GroupResource{Group: "", Resource: "Namespace"}, nsOne) + if ns == nsOne { + // Fabricate a not-found error. For example if the function + // or its namespace had been manually removed + return fn.ErrFunctionNotFound + } + return nil } + clientFn := NewTestClient( + fn.WithDeployer(mock.NewDeployer()), + fn.WithRemover(remover), + fn.WithPipelinesProvider(mock.NewPipelinesProvider()), + ) // Create a basic go Function f := fn.Function{ - Runtime: "go", - Root: root, + Runtime: "go", + Root: root, + Registry: TestRegistry, } _, err := fn.New().Init(f) if err != nil { @@ -1868,30 +1874,23 @@ func TestDeploy_NoErrorOnOldFunctionNotFound(t *testing.T) { } // Deploy the function to ns "nsone" - cmd := NewDeployCmd(NewTestClient( - fn.WithDeployer(mock.NewDeployer()), - fn.WithRegistry(TestRegistry), - fn.WithRemover(remover))) - - cmd.SetArgs([]string{fmt.Sprintf("--namespace=%s", nsOne)}) - err = cmd.Execute() - if err != nil { + cmd := NewDeployCmd(clientFn) + cmd.SetArgs([]string{"--namespace", nsOne}) + if err = cmd.Execute(); err != nil { t.Fatal(err) } // Second Deploy with different namespace - cmd.SetArgs([]string{fmt.Sprintf("--namespace=%s", nsTwo)}) - - err = cmd.Execute() - - // possible TODO: catch the os.Stderr output and check that this is printed out - // and if this is implemented, probably change the name to *_WarnOnFunction - // expectedWarning := fmt.Sprintf("Warning: Cant undeploy Function in namespace '%s' - service not found. Namespace/Service might be deleted already", nsOne) + cmd = NewDeployCmd(clientFn) + cmd.SetArgs([]string{"--namespace", nsTwo}) + if err = cmd.Execute(); err != nil { + // possible TODO: catch the os.Stderr output and check that this is printed out + // and if this is implemented, probably change the name to *_WarnOnFunction + // expectedWarning := fmt.Sprintf("Warning: Cant undeploy Function in namespace '%s' - service not found. Namespace/Service might be deleted already", nsOne) - // ASSERT + // ASSERT - // Needs to pass since the error is set to nil for NotFound error - if err != nil { + // Needs to pass since the error is set to nil for NotFound error t.Fatal(err) } } diff --git a/cmd/describe.go b/cmd/describe.go index d5419b8a2a..dbadb5c873 100644 --- a/cmd/describe.go +++ b/cmd/describe.go @@ -49,7 +49,7 @@ the current directory or from the directory specified with --path. // Flags cmd.Flags().StringP("output", "o", "human", "Output format (human|plain|json|xml|yaml|url) ($FUNC_OUTPUT)") - cmd.Flags().StringP("namespace", "n", cfg.Namespace, "The namespace in which to look for the named function. ($FUNC_NAMESPACE)") + cmd.Flags().StringP("namespace", "n", defaultNamespace(fn.Function{}, false), "The namespace in which to look for the named function. ($FUNC_NAMESPACE)") addPathFlag(cmd) addVerboseFlag(cmd, cfg.Verbose) @@ -61,43 +61,33 @@ the current directory or from the directory specified with --path. } func runDescribe(cmd *cobra.Command, args []string, newClient ClientFactory) (err error) { - cfg := newDescribeConfig(args) - - if err = cfg.Validate(cmd); err != nil { + cfg, err := newDescribeConfig(cmd, args) + if err != nil { return } + // TODO cfg.Prompt() - var f fn.Function + client, done := newClient(ClientConfig{Verbose: cfg.Verbose}) + defer done() - if cfg.Name == "" { - if f, err = fn.NewFunction(cfg.Path); err != nil { - return + var details fn.Instance + if cfg.Name != "" { // Describe by name if provided + details, err = client.Describe(cmd.Context(), cfg.Name, cfg.Namespace, fn.Function{}) + if err != nil { + return err } - if !f.Initialized() { - return fn.NewErrNotInitialized(f.Root) + } else { + f, err := fn.NewFunction(cfg.Path) + if err != nil { + return err } - // Use Function's Namespace with precedence - // - // Unless the namespace flag was explicitly provided (not the default), - // use the function's current namespace. - // - // TODO(lkingland): this stanza can be removed when Global Config: Function - // Context is merged. - if !cmd.Flags().Changed("namespace") { - cfg.Namespace = f.Deploy.Namespace + details, err = client.Describe(cmd.Context(), "", "", f) + if err != nil { + return err } } - client, done := newClient(ClientConfig{Namespace: cfg.Namespace, Verbose: cfg.Verbose}) - defer done() - - // TODO(lkingland): update API to use the above function instance rather than path - d, err := client.Describe(cmd.Context(), cfg.Name, f) - if err != nil { - return - } - - write(os.Stdout, info(d), cfg.Output) + write(os.Stdout, info(details), cfg.Output) return } @@ -112,22 +102,29 @@ type describeConfig struct { Verbose bool } -func newDescribeConfig(args []string) describeConfig { - c := describeConfig{ +func newDescribeConfig(cmd *cobra.Command, args []string) (cfg describeConfig, err error) { + var name string + if len(args) > 0 { + name = args[0] + } + cfg = describeConfig{ + Name: name, Namespace: viper.GetString("namespace"), Output: viper.GetString("output"), Path: viper.GetString("path"), Verbose: viper.GetBool("verbose"), } - if len(args) > 0 { - c.Name = args[0] + if cfg.Name == "" && cmd.Flags().Changed("namespace") { + // logicially inconsistent to supply only a namespace. + // Either use the function's local state in its entirety, or specify + // both a name and a namespace to ignore any local function source. + err = fmt.Errorf("must also specify a name when specifying namespace.") } - return c -} - -func (c describeConfig) Validate(cmd *cobra.Command) (err error) { - if c.Name != "" && c.Path != "" && cmd.Flags().Changed("path") { - return fmt.Errorf("Only one of --path or [NAME] should be provided") + if cfg.Name != "" && cmd.Flags().Changed("path") { + // logically inconsistent to provide both a name and a path to source. + // Either use the function's local state on disk (--path), or specify + // a name and a namespace to ignore any local function source. + err = fmt.Errorf("only one of --path and [NAME] should be provided") } return } diff --git a/cmd/describe_test.go b/cmd/describe_test.go index 954772c814..f6b4a5fdc7 100644 --- a/cmd/describe_test.go +++ b/cmd/describe_test.go @@ -1,10 +1,12 @@ package cmd import ( + "context" "testing" fn "knative.dev/func/pkg/functions" "knative.dev/func/pkg/mock" + . "knative.dev/func/pkg/testing" ) // TestDescribe_ByName ensures that describing a function by name invokes @@ -15,9 +17,9 @@ func TestDescribe_ByName(t *testing.T) { describer = mock.NewDescriber() ) - describer.DescribeFn = func(n string) (fn.Instance, error) { - if n != testname { - t.Fatalf("expected describe name '%v', got '%v'", testname, n) + describer.DescribeFn = func(_ context.Context, name, namespace string) (fn.Instance, error) { + if name != testname { + t.Fatalf("expected describe name '%v', got '%v'", testname, name) } return fn.Instance{}, nil } @@ -37,10 +39,11 @@ func TestDescribe_ByName(t *testing.T) { // (func created in the current working directory) invokes the describer with // its name correctly. func TestDescribe_ByProject(t *testing.T) { - root := fromTempDirectory(t) + root := FromTempDirectory(t) + expected := "testname" _, err := fn.New().Init(fn.Function{ - Name: "testname", + Name: expected, Runtime: "go", Registry: TestRegistry, Root: root, @@ -50,9 +53,9 @@ func TestDescribe_ByProject(t *testing.T) { } describer := mock.NewDescriber() - describer.DescribeFn = func(n string) (i fn.Instance, err error) { - if n != "testname" { - t.Fatalf("expected describer to receive name 'testname', got '%v'", n) + describer.DescribeFn = func(_ context.Context, name, namespace string) (i fn.Instance, err error) { + if name != expected { + t.Fatalf("expected describer to receive name %q, got %q", expected, name) } return } @@ -77,63 +80,3 @@ func TestDescribe_NameAndPathExclusivity(t *testing.T) { t.Fatal("describer was invoked when conflicting flags were provided") } } - -// TestDescribe_Namespace ensures that the namespace provided to the client -// for use when describing a function is set -// 1. Blank when not provided nor available (delegate to the describer impl to -// choose current kube context) -// 2. The namespace of the contextually active function -// 3. The flag /env variable if provided -func TestDescribe_Namespace(t *testing.T) { - root := fromTempDirectory(t) - - client := fn.New(fn.WithDescriber(mock.NewDescriber())) - - // Ensure that the default is "", indicating the describer should use - // config.DefaultNamespace - cmd := NewDescribeCmd(func(cc ClientConfig, _ ...fn.Option) (*fn.Client, func()) { - if cc.Namespace != "" { - t.Fatalf("expected '', got '%v'", cc.Namespace) - } - return client, func() {} - }) - cmd.SetArgs([]string{"somefunc"}) // by name such that no f need be created - if err := cmd.Execute(); err != nil { - t.Fatal(err) - } - - // Ensure the extant function's namespace is used - f := fn.Function{ - Root: root, - Runtime: "go", - Deploy: fn.DeploySpec{ - Namespace: "deployed", - }, - } - if _, err := client.Init(f); err != nil { - t.Fatal(err) - } - cmd = NewDescribeCmd(func(cc ClientConfig, _ ...fn.Option) (*fn.Client, func()) { - if cc.Namespace != "deployed" { - t.Fatalf("expected 'deployed', got '%v'", cc.Namespace) - } - return client, func() {} - }) - cmd.SetArgs([]string{}) - if err := cmd.Execute(); err != nil { - t.Fatal(err) - } - - // Ensure an explicit namespace is plumbed through - cmd = NewDescribeCmd(func(cc ClientConfig, _ ...fn.Option) (*fn.Client, func()) { - if cc.Namespace != "ns" { - t.Fatalf("expected 'ns', got '%v'", cc.Namespace) - } - return client, func() {} - }) - cmd.SetArgs([]string{"--namespace", "ns"}) - if err := cmd.Execute(); err != nil { - t.Fatal(err) - } - -} diff --git a/cmd/environment.go b/cmd/environment.go index dd9363f7a5..f2cf846316 100644 --- a/cmd/environment.go +++ b/cmd/environment.go @@ -195,10 +195,10 @@ func describeFuncInformation(context context.Context, newClient ClientFactory, c return nil, nil } - client, done := newClient(ClientConfig{Namespace: function.Deploy.Namespace, Verbose: cfg.Verbose}) + client, done := newClient(ClientConfig{Verbose: cfg.Verbose}) defer done() - instance, err := client.Describe(context, function.Name, function) + instance, err := client.Describe(context, function.Name, function.Deploy.Namespace, function) if err != nil { return &function, nil } diff --git a/cmd/invoke.go b/cmd/invoke.go index c03fc1fa94..5195de8b59 100644 --- a/cmd/invoke.go +++ b/cmd/invoke.go @@ -153,7 +153,7 @@ func runInvoke(cmd *cobra.Command, _ []string, newClient ClientFactory) (err err } // Client instance from env vars, flags, args and user prompts (if --confirm) - client, done := newClient(ClientConfig{Namespace: f.Deploy.Namespace, Verbose: cfg.Verbose, InsecureSkipVerify: cfg.Insecure}) + client, done := newClient(ClientConfig{Verbose: cfg.Verbose, InsecureSkipVerify: cfg.Insecure}) defer done() // Message to send the running function built from parameters gathered diff --git a/cmd/invoke_test.go b/cmd/invoke_test.go index 8995041fe2..02f19a92eb 100644 --- a/cmd/invoke_test.go +++ b/cmd/invoke_test.go @@ -13,11 +13,12 @@ import ( fn "knative.dev/func/pkg/functions" "knative.dev/func/pkg/mock" + . "knative.dev/func/pkg/testing" ) // TestInvoke command executes the invocation path. func TestInvoke(t *testing.T) { - root := fromTempDirectory(t) + root := FromTempDirectory(t) var invoked int32 @@ -77,31 +78,3 @@ func TestInvoke(t *testing.T) { t.Fatal("function was not invoked") } } - -// TestInvoke_Namespace ensures that invocation uses the Function's namespace -// despite the currently active. -func TestInvoke_Namespace(t *testing.T) { - root := fromTempDirectory(t) - - // Create a Function in a non-active namespace - f := fn.Function{Runtime: "go", Root: root, Deploy: fn.DeploySpec{Namespace: "ns"}} - _, err := fn.New().Init(f) - if err != nil { - t.Fatal(err) - } - - // The shared Client constructor should receive the current function's - // namespace when constructing its describer (used when finding the - // function's route), not the currently active namespace. - namespace := "" - newClient := func(conf ClientConfig, opts ...fn.Option) (*fn.Client, func()) { - namespace = conf.Namespace // should be set to that of the function - return NewClient(conf, opts...) - } - cmd := NewInvokeCmd(newClient) - _ = cmd.Execute() // invocation error expected - - if namespace != "ns" { - t.Fatalf("expected client to receive function's current namespace 'ns', got '%v'", namespace) - } -} diff --git a/cmd/languages_test.go b/cmd/languages_test.go index a91ecff08d..4874b6b2ad 100644 --- a/cmd/languages_test.go +++ b/cmd/languages_test.go @@ -2,13 +2,15 @@ package cmd import ( "testing" + + . "knative.dev/func/pkg/testing" ) // TestLanguages_Default ensures that the default behavior of listing // all supported languages is to print a plain text list of all the builtin // language runtimes. func TestLanguages_Default(t *testing.T) { - _ = fromTempDirectory(t) + _ = FromTempDirectory(t) buf := piped(t) // gather output cmd := NewLanguagesCmd(NewClient) @@ -33,7 +35,7 @@ typescript` // TestLanguages_JSON ensures that listing languages in --json format returns // builtin languages as a JSON array. func TestLanguages_JSON(t *testing.T) { - _ = fromTempDirectory(t) + _ = FromTempDirectory(t) buf := piped(t) // gather output cmd := NewLanguagesCmd(NewClient) diff --git a/cmd/list.go b/cmd/list.go index f0d2ed5a02..cdf14c90db 100644 --- a/cmd/list.go +++ b/cmd/list.go @@ -23,7 +23,7 @@ func NewListCmd(newClient ClientFactory) *cobra.Command { Short: "List deployed functions", Long: `List deployed functions -Lists all deployed functions in a given namespace. +Lists deployed functions. `, Example: ` # List all functions in the current namespace with human readable output @@ -50,17 +50,25 @@ Lists all deployed functions in a given namespace. // Namespace Config // Differing from other commands, the default namespace for the list - // command is always the currently active namespace as returned by - // config.DefaultNamespace(). The -A flag clears this value indicating - // the lister implementation should not filter by namespace and instead - // list from all namespaces. This logic is sligtly inverse to the other - // namespace-sensitive commands which default to the currently active - // function if available, and delegate to the implementation to use - // the config default otherwise. + // command is set to the currently active namespace as returned by + // calling k8s.DefaultNamespace(). This way a call to `func list` will + // show functions in the currently active namespace. If the value can + // not be determined due to error, a warning is printed to log and + // no namespace is passed to the lister, which should result in the + // lister showing functions for all namespaces. + // + // This also extends to the treatment of the global setting for + // namespace. This is likewise intended for command which require a + // namespace no matter what. Therefore the global namespace setting is + // not applicable to this command because "default" really means "all". + // + // This is slightly different than other commands wherein their + // default is often to presume namespace "default" if none was either + // supplied nor available. // Flags cmd.Flags().BoolP("all-namespaces", "A", false, "List functions in all namespaces. If set, the --namespace flag is ignored.") - cmd.Flags().StringP("namespace", "n", config.DefaultNamespace(), "The namespace for which to list functions. ($FUNC_NAMESPACE)") + cmd.Flags().StringP("namespace", "n", defaultNamespace(fn.Function{}, false), "The namespace for which to list functions. ($FUNC_NAMESPACE)") cmd.Flags().StringP("output", "o", "human", "Output format (human|plain|json|xml|yaml) ($FUNC_OUTPUT)") addVerboseFlag(cmd, cfg.Verbose) @@ -72,16 +80,15 @@ Lists all deployed functions in a given namespace. } func runList(cmd *cobra.Command, _ []string, newClient ClientFactory) (err error) { - cfg := newListConfig() - - if err := cfg.Validate(cmd); err != nil { + cfg, err := newListConfig(cmd) + if err != nil { return err } - client, done := newClient(ClientConfig{Namespace: cfg.Namespace, Verbose: cfg.Verbose}) + client, done := newClient(ClientConfig{Verbose: cfg.Verbose}) defer done() - items, err := client.List(cmd.Context()) + items, err := client.List(cmd.Context(), cfg.Namespace) if err != nil { return } @@ -109,26 +116,24 @@ type listConfig struct { Verbose bool } -func newListConfig() listConfig { - c := listConfig{ +func newListConfig(cmd *cobra.Command) (cfg listConfig, err error) { + cfg = listConfig{ Namespace: viper.GetString("namespace"), Output: viper.GetString("output"), Verbose: viper.GetBool("verbose"), } - // Lister instantiated by newClient explicitly expects "" namespace to - // inidicate it should list from all namespaces, so remove default "default" - // when -A. + // If --all-namespaces, zero out any value for namespace (such as) + // "all" to the lister. if viper.GetBool("all-namespaces") { - c.Namespace = "" + cfg.Namespace = "" } - return c -} -func (c listConfig) Validate(cmd *cobra.Command) error { + // specifying both -A and --namespace is logically inconsistent if cmd.Flags().Changed("namespace") && viper.GetBool("all-namespaces") { - return errors.New("Both --namespace and --all-namespaces specified.") + err = errors.New("Both --namespace and --all-namespaces specified.") } - return nil + + return } // Output Formatting (serializers) diff --git a/cmd/list_test.go b/cmd/list_test.go index b0dae81141..c90e5b48f4 100644 --- a/cmd/list_test.go +++ b/cmd/list_test.go @@ -1,37 +1,53 @@ package cmd import ( + "context" "testing" fn "knative.dev/func/pkg/functions" "knative.dev/func/pkg/mock" + . "knative.dev/func/pkg/testing" ) -// TestList_Namespace ensures that list command options for specifying a -// namespace (--namespace) or all namespaces (--all-namespaces) are properly -// evaluated. +// TestList_Namespace ensures that list command handles namespace options +// namespace (--namespace) and all namespaces (--all-namespaces) correctly +// and that the current kube context is used by default. func TestList_Namespace(t *testing.T) { - _ = fromTempDirectory(t) + _ = FromTempDirectory(t) tests := []struct { name string - all bool // --all-namespaces - namespace string // use specific namespace - expected string // expected + namespace string // --namespace flag (use specific namespace) + all bool // --all-namespaces (no namespace filter) + allShort bool // -A (no namespace filter) + expected string // expected value passed to lister err bool // expected error }{ { - name: "default", - expected: "func", // see ./testdata/default_kubeconfig + name: "default (none specififed)", + namespace: "", + all: false, + allShort: false, + expected: "func", // see testdata kubeconfig }, { name: "namespace provided", namespace: "ns", + all: false, + allShort: false, expected: "ns", }, { - name: "all namespaces", - all: true, + name: "all namespaces", + namespace: "", + all: true, + allShort: false, + expected: "", // --all-namespaces | -A explicitly mean none specified + }, + { + name: "all namespaces - short flag", + all: false, + allShort: true, expected: "", // blank is implemented by lister as meaning all }, { @@ -43,33 +59,51 @@ func TestList_Namespace(t *testing.T) { } for _, test := range tests { t.Run(test.name, func(t *testing.T) { - var ( - lister = mock.NewLister() - client = fn.New(fn.WithLister(lister)) - ) - cmd := NewListCmd(func(cc ClientConfig, options ...fn.Option) (*fn.Client, func()) { - if cc.Namespace != test.expected { - t.Fatalf("expected '%v', got '%v'", test.expected, cc.Namespace) + // create a mock lister implementation which validates the expected + // value has been passed. + lister := mock.NewLister() + lister.ListFn = func(_ context.Context, namespace string) ([]fn.ListItem, error) { + if namespace != test.expected { + t.Fatalf("expected list namespace %q, got %q", test.expected, namespace) } - return client, func() {} - }) + return []fn.ListItem{}, nil + } + + // Create an instance of the command which sets the flags + // according to the test case + cmd := NewListCmd(NewTestClient(fn.WithLister(lister))) args := []string{} if test.namespace != "" { args = append(args, "--namespace", test.namespace) } if test.all { + args = append(args, "--all-namespaces") + } + if test.allShort { args = append(args, "-A") } cmd.SetArgs(args) + // Execute err := cmd.Execute() - if err != nil && !test.err { - // TODO: typed error for --namespace with -A. Perhaps ErrFlagConflict? - t.Fatalf("unexpected error: %v", err) - } - if err == nil && test.err { + + // Check for expected error + if err != nil { + if !test.err { + t.Fatalf("unexpected error: %v", err) + } + // expected error received + return + } else if test.err { t.Fatalf("did not receive expected error ") } + + // For tests which did not expect an error, ensure the lister + // was invoked + if !lister.ListInvoked { + t.Fatalf("%v: the lister was not invoked", test.name) + } + }) } } diff --git a/cmd/repository_test.go b/cmd/repository_test.go index 6cb24c12ff..267924bc32 100644 --- a/cmd/repository_test.go +++ b/cmd/repository_test.go @@ -10,7 +10,7 @@ import ( // set of repositories by name for builtin repositories, by explicitly // setting the repositories' path to a new path which includes no others. func TestRepository_List(t *testing.T) { - _ = fromTempDirectory(t) + _ = FromTempDirectory(t) cmd := NewRepositoryListCmd(NewClient) cmd.SetArgs([]string{}) // Do not use test command args @@ -34,7 +34,7 @@ func TestRepository_List(t *testing.T) { // upon subsequent 'list'. func TestRepository_Add(t *testing.T) { url := ServeRepo("repository.git#main", t) - _ = fromTempDirectory(t) + _ = FromTempDirectory(t) t.Log(url) var ( @@ -75,7 +75,7 @@ func TestRepository_Add(t *testing.T) { // reflected as having been renamed upon subsequent 'list'. func TestRepository_Rename(t *testing.T) { url := ServeRepo("repository.git", t) - _ = fromTempDirectory(t) + _ = FromTempDirectory(t) var ( add = NewRepositoryAddCmd(NewClient) @@ -123,7 +123,7 @@ func TestRepository_Rename(t *testing.T) { // subsequent 'list'. func TestRepository_Remove(t *testing.T) { url := ServeRepo("repository.git", t) - _ = fromTempDirectory(t) + _ = FromTempDirectory(t) var ( add = NewRepositoryAddCmd(NewClient) diff --git a/cmd/root.go b/cmd/root.go index 25cb001a00..d813f92208 100644 --- a/cmd/root.go +++ b/cmd/root.go @@ -24,6 +24,10 @@ import ( // DefaultVersion when building source directly (bypassing the Makefile) const DefaultVersion = "v0.0.0+source" +// DefaultNamespace is the global static default namespace, and is equivalent +// to the Kubernetes default namespace. +const DefaultNamespace = "default" + type RootCommandConfig struct { Name string // usually `func` or `kn func` Version @@ -151,6 +155,45 @@ func effectivePath() (path string) { return path } +// defaultNamespace to use when none is provided explicitly. +// This requires a bit more logic than normal flag defaults, which rely +// on the order of precidence Static Config -> Global Config -> Current Func -> +// -> Environment Variables -> Flags. This defautl caluclation adds the +// step of using the active kubernetes namespace after Static Config and before +// the optional Global Config setting. The staic default is "default" +func defaultNamespace(f fn.Function, verbose bool) string { + // Specifically-requested + if f.Namespace != "" { + return f.Namespace + } + + // Last deployed + if f.Deploy.Namespace != "" { + return f.Deploy.Namespace + } + + // Active K8S namespace + namespace, err := k8s.GetDefaultNamespace() + if err != nil { + if verbose { + fmt.Fprintf(os.Stderr, "Unable to get current active kubernetes namespace. Defaults will be used. %v", err) + } + } else if namespace != "" { + return namespace + } + + // Globally-defined default in ~/.config/func/config.yaml is next + cfg, err := config.NewDefault() + if err != nil { + fmt.Fprintf(os.Stderr, "error loading global config at '%v'. %v\n", config.File(), err) + } else if cfg.Namespace != "" { + return cfg.Namespace + } + + // Static Default is the standard Kubernetes default "default" + return DefaultNamespace +} + // interactiveTerminal returns whether or not the currently attached process // terminal is interactive. Used for determining whether or not to // interactively prompt the user to confirm default choices, etc. diff --git a/cmd/root_test.go b/cmd/root_test.go index 138501711c..a97aa7807d 100644 --- a/cmd/root_test.go +++ b/cmd/root_test.go @@ -271,6 +271,75 @@ func TestRoot_effectivePath(t *testing.T) { } +// Test_defaultNamespace ensures that the order of precedence for +// determining the effective namespace is followed. +// to use for the next deployment. +func Test_defaultNamespace(t *testing.T) { + // Clear non-test envs and set the test KUBECONFIG to nonexistent, but + // save the current working directory for setting kube context in some + // test cases. + cwd := Cwd() + _ = FromTempDirectory(t) // clears non-test envs and enters a temp dir. + t.Setenv("KUBECONFIG", filepath.Join(t.TempDir(), "nonexistent")) + + // also clear the test KUBECONFIG env + tests := []struct { + name string + context bool + global bool + expected string + }{ + // TODO cases for function state f.Namespace and f.Deploy.Namespace + { + name: "static default", + context: false, // no active kube context + global: false, // no global + expected: DefaultNamespace, // expect static default + }, { + name: "global config", + context: false, + global: true, // see the global defined in FUNC_HOME testdata + expected: "globaldefault", // expect global to override static + }, { + name: "active context", + context: true, // see the config in KUBECONFIG testdata + global: true, + expected: "mynamespace", // active context overrides global default + }, + } + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + + if test.global { // enable a global config setting + t.Setenv("XDG_CONFIG_HOME", filepath.Join(cwd, "testdata", "Test_defaultNamespace")) + } + if test.context { // enable an active kube context + t.Setenv("KUBECONFIG", filepath.Join(cwd, "testdata", "Test_defaultNamespace", "kubeconfig")) + } + + namespace := defaultNamespace(fn.Function{}, false) + if namespace != test.expected { + t.Fatalf("%v: expected namespace %q, got %q", test.name, test.expected, namespace) + } + + }) + } + + // t.Setenv("KUBECONFIG", filepath.Join(t.TempDir(), "nonexistent")) + // t.Setenv("KUBERNETES_SERVICE_HOST", "") + // t.Setenv("XDG_CONFIG_HOME", home) + // if config.DefaultNamespace() != "default" { + // t.Fatalf("did not receive expected default namespace 'default', got '%v'", config.DefaultNamespace()) + // } + // + // // should be "func" when active k8s namespace is "func" + // kubeconfig := filepath.Join(cwd, "testdata", "TestDefaultNamespace", "kubeconfig") + // t.Setenv("KUBECONFIG", kubeconfig) + // if config.DefaultNamespace() != "func" { + // t.Fatalf("expected default namespace of 'func' when that is the active k8s namespace. Got '%v'", config.DefaultNamespace()) + // } +} + // Helpers // ------- @@ -309,30 +378,3 @@ func piped(t *testing.T) func() string { return strings.TrimSpace(b.String()) } } - -// fromTempDirectory is a test helper which endeavors to create -// 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' - // NOTE: the below settings affect unit tests only, and we do explicitly - // want all unit tests to start in an empty environment with tests "opting in" - // to config, not opting out. - t.Setenv("KUBECONFIG", filepath.Join(cwd(), "testdata", "default_kubeconfig")) - - // By default unit tests presum no config exists unless provided in testdata. - t.Setenv("XDG_CONFIG_HOME", t.TempDir()) - - t.Setenv("KUBERNETES_SERVICE_HOST", "") - - // creates and CDs to a temp directory - d, done := Mktemp(t) - - // Return to original directory and resets viper. - t.Cleanup(func() { done(); viper.Reset() }) - return d -} diff --git a/cmd/run_test.go b/cmd/run_test.go index b7cd6b2d5e..b655186870 100644 --- a/cmd/run_test.go +++ b/cmd/run_test.go @@ -8,6 +8,7 @@ import ( fn "knative.dev/func/pkg/functions" "knative.dev/func/pkg/mock" + . "knative.dev/func/pkg/testing" ) func TestRun_Run(t *testing.T) { @@ -102,7 +103,7 @@ func TestRun_Run(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - root := fromTempDirectory(t) + root := FromTempDirectory(t) runner := mock.NewRunner() if tt.runError != nil { diff --git a/cmd/subscribe_test.go b/cmd/subscribe_test.go index daf6651884..b013cff914 100644 --- a/cmd/subscribe_test.go +++ b/cmd/subscribe_test.go @@ -4,10 +4,11 @@ import ( "testing" fn "knative.dev/func/pkg/functions" + . "knative.dev/func/pkg/testing" ) func TestSubscribeWithAll(t *testing.T) { - root := fromTempDirectory(t) + root := FromTempDirectory(t) _, err := fn.New().Init(fn.Function{Runtime: "go", Root: root}) if err != nil { @@ -40,7 +41,7 @@ func TestSubscribeWithAll(t *testing.T) { } func TestSubscribeWithMultiple(t *testing.T) { - root := fromTempDirectory(t) + root := FromTempDirectory(t) _, err := fn.New().Init(fn.Function{Runtime: "go", Root: root}) if err != nil { @@ -101,7 +102,7 @@ func TestSubscribeWithMultiple(t *testing.T) { } func TestSubscribeWithMultipleBrokersAndOverride(t *testing.T) { - root := fromTempDirectory(t) + root := FromTempDirectory(t) _, err := fn.New().Init(fn.Function{Runtime: "go", Root: root}) if err != nil { @@ -182,7 +183,7 @@ func TestSubscribeWithMultipleBrokersAndOverride(t *testing.T) { } func TestSubscribeWithNoExplicitSourceAll(t *testing.T) { - root := fromTempDirectory(t) + root := FromTempDirectory(t) _, err := fn.New().Init(fn.Function{Runtime: "go", Root: root}) if err != nil { @@ -215,7 +216,7 @@ func TestSubscribeWithNoExplicitSourceAll(t *testing.T) { } func TestSubscribeWithDuplicated(t *testing.T) { - root := fromTempDirectory(t) + root := FromTempDirectory(t) _, err := fn.New().Init(fn.Function{Runtime: "go", Root: root}) if err != nil { diff --git a/cmd/templates_test.go b/cmd/templates_test.go index 455f81e4b6..e57dbebed8 100644 --- a/cmd/templates_test.go +++ b/cmd/templates_test.go @@ -6,12 +6,13 @@ import ( "github.com/google/go-cmp/cmp" "gotest.tools/v3/assert" + . "knative.dev/func/pkg/testing" ) // TestTemplates_Default ensures that the default behavior is listing all // templates for all language runtimes. func TestTemplates_Default(t *testing.T) { - _ = fromTempDirectory(t) + _ = FromTempDirectory(t) buf := piped(t) // gather output cmd := NewTemplatesCmd(NewClient) @@ -46,7 +47,7 @@ typescript http` // TestTemplates_JSON ensures that listing templates respects the --json // output format. func TestTemplates_JSON(t *testing.T) { - _ = fromTempDirectory(t) + _ = FromTempDirectory(t) buf := piped(t) // gather output cmd := NewTemplatesCmd(NewClient) @@ -96,7 +97,7 @@ func TestTemplates_JSON(t *testing.T) { // TestTemplates_ByLanguage ensures that the output is correctly filtered // by language runtime when provided. func TestTemplates_ByLanguage(t *testing.T) { - _ = fromTempDirectory(t) + _ = FromTempDirectory(t) cmd := NewTemplatesCmd(NewClient) cmd.SetArgs([]string{"go"}) @@ -135,7 +136,7 @@ http` } func TestTemplates_ErrTemplateRepoDoesNotExist(t *testing.T) { - _ = fromTempDirectory(t) + _ = FromTempDirectory(t) cmd := NewTemplatesCmd(NewClient) cmd.SetArgs([]string{"--repository", "https://github.com/boson-project/repo-does-not-exist"}) @@ -145,7 +146,7 @@ func TestTemplates_ErrTemplateRepoDoesNotExist(t *testing.T) { } func TestTemplates_WrongRepositoryUrl(t *testing.T) { - _ = fromTempDirectory(t) + _ = FromTempDirectory(t) cmd := NewTemplatesCmd(NewClient) cmd.SetArgs([]string{"--repository", "wrong://github.com/boson-project/repo-does-not-exist"}) diff --git a/cmd/testdata/Test_defaultNamespace/func/config.yaml b/cmd/testdata/Test_defaultNamespace/func/config.yaml new file mode 100644 index 0000000000..fea5a81e6b --- /dev/null +++ b/cmd/testdata/Test_defaultNamespace/func/config.yaml @@ -0,0 +1 @@ +namespace: "globaldefault" diff --git a/cmd/testdata/TestDeploy_NamespaceDefaults/TestDeploy_NamespaceRedeployWarning/kubeconfig b/cmd/testdata/Test_defaultNamespace/kubeconfig similarity index 100% rename from cmd/testdata/TestDeploy_NamespaceDefaults/TestDeploy_NamespaceRedeployWarning/kubeconfig rename to cmd/testdata/Test_defaultNamespace/kubeconfig diff --git a/cmd/testdata/Test_namespace/kubeconfig b/cmd/testdata/Test_namespace/kubeconfig deleted file mode 100644 index cadebe7409..0000000000 --- a/cmd/testdata/Test_namespace/kubeconfig +++ /dev/null @@ -1,25 +0,0 @@ -apiVersion: v1 -clusters: -- cluster: - insecure-skip-tls-verify: true - server: https://cluster.example.com:6443 - name: cluster-example-com:6443 -contexts: -- context: - cluster: cluster-example-com:6443 - namespace: default - user: kube:admin/cluster-example-com:6443 - name: default/cluster-example-com:6443/kube:admin -- context: - cluster: cluster-example-com:6443 - namespace: mynamespace - user: kube:admin/cluster-example-com:6443 - name: mynamespace/cluster-example-com:6443/kube:admin -current-context: mynamespace/cluster-example-com:6443/kube:admin -kind: Config -preferences: {} -users: -- name: kubeadmin - user: - token: sha256~XXXXexample-test-hash - diff --git a/pkg/config/config.go b/pkg/config/config.go index 0f5059a3ab..e357b733d4 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -29,19 +29,6 @@ const ( DefaultBuilder = builders.Default ) -// DefaultNamespace for remote operations is the currently active -// context namespace (if available) or the fallback "default". -// Subsequently the value will be populated, indicating the namespace in which the -// function is currently deployed. Changes to this value will issue warnings -// to the user. -func DefaultNamespace() (namespace string) { - var err error - if namespace, err = k8s.GetDefaultNamespace(); err != nil { - return "default" - } - return -} - // Global configuration settings. type Global struct { Builder string `yaml:"builder,omitempty"` @@ -139,9 +126,12 @@ func (c Global) Apply(f fn.Function) Global { if f.Runtime != "" { c.Language = f.Runtime } - if f.Deploy.Namespace != "" { - c.Namespace = f.Deploy.Namespace - } + // Namespace resolution is handled manually in the CLI due to needing + // to consider current kubernetes context. This may be merged back + // in here once the logic is refined. + // if f.Deploy.Namespace != "" { + // c.Namespace = f.Deploy.Namespace + // } if f.Registry != "" { c.Registry = f.Registry } diff --git a/pkg/config/config_test.go b/pkg/config/config_test.go index d8c123f6a8..eac6a3e731 100644 --- a/pkg/config/config_test.go +++ b/pkg/config/config_test.go @@ -173,32 +173,6 @@ func TestRepositoriesPath(t *testing.T) { } } -// TestDefaultNamespace ensures that, when there is a problem determining the -// active namespace, the static DefaultNamespace ("default") is used and that -// the currently active k8s namespace is used as the default if available. -func TestDefaultNamespace(t *testing.T) { - cwd := Cwd() // store for use after Mktemp which changes working directory - - // Namespace "default" when empty home - // Note that KUBECONFIG must be defined, or the current user's ~/.kube/config - // will be used (and thus whichever namespace they have currently active) - home, cleanup := Mktemp(t) - t.Cleanup(cleanup) - t.Setenv("KUBECONFIG", filepath.Join(t.TempDir(), "nonexistent")) - t.Setenv("KUBERNETES_SERVICE_HOST", "") - t.Setenv("XDG_CONFIG_HOME", home) - if config.DefaultNamespace() != "default" { - t.Fatalf("did not receive expected default namespace 'default', got '%v'", config.DefaultNamespace()) - } - - // should be "func" when active k8s namespace is "func" - kubeconfig := filepath.Join(cwd, "testdata", "TestDefaultNamespace", "kubeconfig") - t.Setenv("KUBECONFIG", kubeconfig) - if config.DefaultNamespace() != "func" { - 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. @@ -226,9 +200,14 @@ func TestApply(t *testing.T) { if cfg.Language != "runtime" { t.Error("apply missing map of f.Runtime ") } - if cfg.Namespace != "namespace" { - t.Error("apply missing map of f.Namespace") - } + // Note that namespace is handled manually in the clients because + // active k8s context must be taken into account. This may + // be merged back into this config package in the future, but for now + // "applying" a function's state onto a config will not alter + // the namespace value, because it's not a simple mapping. + // if cfg.Namespace != "namespace" { + // t.Error("apply missing map of f.Namespace") + // } if cfg.Registry != "registry" { t.Error("apply missing map of f.Registry") } @@ -242,9 +221,9 @@ func TestApply(t *testing.T) { 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.Namespace == "" { + // t.Error("empty f.Namespace should not be mapped") + // } if cfg.Registry == "" { t.Error("empty f.Registry should not be mapped") } diff --git a/pkg/functions/client.go b/pkg/functions/client.go index 622b2055fe..bcfda8edef 100644 --- a/pkg/functions/client.go +++ b/pkg/functions/client.go @@ -145,7 +145,7 @@ type Remover interface { // Lister of deployed functions. type Lister interface { // List the functions currently deployed. - List(ctx context.Context) ([]ListItem, error) + List(ctx context.Context, namespace string) ([]ListItem, error) } type ListItem struct { @@ -159,7 +159,7 @@ type ListItem struct { // Describer of function instances type Describer interface { // Describe the named function in the remote environment. - Describe(ctx context.Context, name string) (Instance, error) + Describe(ctx context.Context, name, namespace string) (Instance, error) } // Instance data about the runtime state of a function in a given environment. @@ -480,6 +480,7 @@ func (c *Client) Update(ctx context.Context, f Function) (string, Function, erro // // Use Apply for higher level control. Use Init, Build, Push, Deploy // independently for lower level control. +// // Returns the primary route to the function or error. func (c *Client) New(ctx context.Context, cfg Function) (string, Function, error) { // Always start a concurrent routine listening for context cancellation. @@ -723,13 +724,13 @@ func (c *Client) printBuildActivity(ctx context.Context) { }() } -type DeployParams struct { +type DeployOptions struct { skipBuiltCheck bool } -type DeployOption func(f *DeployParams) +type DeployOption func(f *DeployOptions) func WithDeploySkipBuildCheck(skipBuiltCheck bool) DeployOption { - return func(f *DeployParams) { + return func(f *DeployOptions) { f.skipBuiltCheck = skipBuiltCheck } } @@ -737,10 +738,10 @@ func WithDeploySkipBuildCheck(skipBuiltCheck bool) DeployOption { // Deploy the function at path. // Errors if the function has not been built unless explicitly instructed // to ignore this build check. -func (c *Client) Deploy(ctx context.Context, f Function, opts ...DeployOption) (Function, error) { - deployParams := &DeployParams{skipBuiltCheck: false} - for _, opt := range opts { - opt(deployParams) +func (c *Client) Deploy(ctx context.Context, f Function, oo ...DeployOption) (Function, error) { + options := &DeployOptions{} + for _, o := range oo { + o(options) } go func() { @@ -749,7 +750,7 @@ func (c *Client) Deploy(ctx context.Context, f Function, opts ...DeployOption) ( // Functions must be built (have an associated image) before being deployed. // Note that externally built images may be specified in the func.yaml - if !deployParams.skipBuiltCheck && !f.Built() { + if !f.Built() && !options.skipBuiltCheck { return f, ErrNotBuilt } @@ -759,44 +760,45 @@ func (c *Client) Deploy(ctx context.Context, f Function, opts ...DeployOption) ( return f, ErrNameRequired } - // TODO: gauron99 -- ideally namespace would be determined here to keep consistancy - // with the Remover but it either creates a cyclic dependency or deployer.namespace - // is not defined here for it to be complete. Maybe it would be worth to try to - // do it this way. - - // Deploy a new or Update the previously-deployed function - fmt.Fprintf(os.Stderr, "⬆️ Deploying function to the cluster\n") - result, err := c.deployer.Deploy(ctx, f) - if err != nil { - fmt.Printf("deploy error: %v\n", err) - return f, err + // Warn if moving + changingNamespace := func(f Function) bool { + // We're changing namespace if: + return f.Deploy.Namespace != "" && // it's already deployed + f.Namespace != "" && // a specific (new) namespace is requested + (f.Namespace != f.Deploy.Namespace) // and it's different } // If Redeployment to NEW namespace was successful -- undeploy dangling Function in old namespace. // On forced namespace change (using --namespace flag) - if f.Namespace != "" && f.Namespace != f.Deploy.Namespace && f.Deploy.Namespace != "" { + if changingNamespace(f) { if c.verbose { - fmt.Fprintf(os.Stderr, "Info: Deleting old func in '%s' because the namespace has changed to '%s'\n", f.Deploy.Namespace, f.Namespace) + fmt.Fprintf(os.Stderr, "Moving Function from %q to %q \n", f.Deploy.Namespace, f.Namespace) } // c.Remove removes a Function in f.Deploy.Namespace which removes the OLD Function // because its not updated yet (see few lines below) - err = c.Remove(ctx, f, true) - - // Warn when service is not found and set err to nil to continue. Function's - // service mightve been manually deleted prior to the subsequent deploy or the - // namespace is already deleted therefore there is nothing to delete - if ErrFunctionNotFound != err { - fmt.Fprintf(os.Stderr, "Warning: Cant undeploy Function in namespace '%s' - service not found. Namespace/Service might be deleted already\n", f.Deploy.Namespace) - err = nil - } + err := c.Remove(ctx, "", "", f, true) if err != nil { + // Warn when service is not found and set err to nil to continue. Function's + // service mightve been manually deleted prior to the subsequent deploy or the + // namespace is already deleted therefore there is nothing to delete + if errors.Is(err, ErrFunctionNotFound) { + fmt.Fprintf(os.Stderr, "Warning: Can't undeploy Function from namespace '%s'. The Function's service was not found. The namespace or service may have already been removed\n", f.Deploy.Namespace) + err = nil + } return f, err } } - // Update the function with the namespace into which the function was - // deployed + // Deploy a new or Update the previously-deployed function + if c.verbose { + fmt.Fprintf(os.Stderr, "⬆️ Deploying \n") + } + result, err := c.deployer.Deploy(ctx, f) + if err != nil { + return f, fmt.Errorf("deploy error. %w", err) + } + // Update the function to reflect the new deployed state of the Function f.Deploy.Namespace = result.Namespace if result.Status == Deployed { @@ -960,28 +962,40 @@ func (c *Client) Run(ctx context.Context, f Function, options ...RunOption) (job return job, nil } -// Describe a function. Name takes precedence. If no name is provided, -// the function defined at root is used. -func (c *Client) Describe(ctx context.Context, name string, f Function) (d Instance, err error) { +// Describe a function. Name/Namespace takes precedence if provided. If no +// name/namespace is provided, the function passed is described based off of +// its name and currently deployed namespace. +func (c *Client) Describe(ctx context.Context, name, namespace string, f Function) (d Instance, err error) { // If name is provided, it takes precedence. // Otherwise load the function defined at root. + // It is up to the concrete implementation whether or not namespace is + // also required. if name != "" { - return c.describer.Describe(ctx, name) + return c.describer.Describe(ctx, name, namespace) } + // If the function's not initialized, then we can save some time and + // fail fast. if !f.Initialized() { return d, fmt.Errorf("function not initialized: %v", f.Root) } + + // If the function is undeployed, we can't describe it either. if f.Name == "" { return d, fmt.Errorf("unable to describe without a name. %v", ErrNameRequired) } - return c.describer.Describe(ctx, f.Name) + + return c.describer.Describe(ctx, f.Name, f.Deploy.Namespace) } // List currently deployed functions. -func (c *Client) List(ctx context.Context) ([]ListItem, error) { +// If namespace is empty, the static implementation of the current +// "Lister" is used, which for example with the knative lister defaults to +// using the current kubernetes context namespace, falling back to the static +// default "namespace". +func (c *Client) List(ctx context.Context, namespace string) ([]ListItem, error) { // delegate to concrete implementation of lister entirely. - return c.lister.List(ctx) + return c.lister.List(ctx, namespace) } // Remove a function. Name takes precedence. If no name is provided, the @@ -989,66 +1003,57 @@ func (c *Client) List(ctx context.Context) ([]ListItem, error) { // namespace must be provided in .Deploy.Namespace field except when using mocks // in which case empty namespace is accepted because its existence is checked // in the sub functions remover.Remove and pipilines.Remove -func (c *Client) Remove(ctx context.Context, cfg Function, deleteAll bool) error { - functionName := cfg.Name - functionNamespace := cfg.Deploy.Namespace - - // If name is provided, it takes precedence. - // Otherwise load the function defined at root. - if cfg.Name == "" { - f, err := NewFunction(cfg.Root) - if err != nil { - return err - } - if !f.Initialized() { - return fmt.Errorf("function at %v can not be removed unless initialized. Try removing by name", f.Root) - } - // take the functions name and namespace and load it as current function - functionName = f.Name - functionNamespace = f.Deploy.Namespace - cfg = f - } - - // if still empty, get current function's yaml deployed namespace - if functionNamespace == "" { - var f Function - f, err := NewFunction(cfg.Root) - if err != nil { - return err - } - functionNamespace = f.Deploy.Namespace +func (c *Client) Remove(ctx context.Context, name, namespace string, f Function, all bool) error { + // Default to name/namespace, fallback to passed Function + if name == "" { + name = f.Name + namespace = f.Deploy.Namespace } - if functionName == "" { + // Preconditions + if name == "" { return ErrNameRequired } - if functionNamespace == "" { + if namespace == "" { return ErrNamespaceRequired } - // Delete Knative Service and dependent resources in parallel - fmt.Fprintf(os.Stderr, "Removing Knative Service: %v in namespace '%v'\n", functionName, functionNamespace) - errChan := make(chan error) + // Logging + if c.verbose { + if all { + fmt.Fprintf(os.Stderr, "Removing %v (namespace %q) and all dependent resources\n", name, namespace) + } else { + fmt.Fprintf(os.Stderr, "Removing %v (namespace %q)\n", name, namespace) + } + } + + // Perform the Removal + var ( + serviceRemovalErrCh = make(chan error) + resourceRemovalError error + ) go func() { - errChan <- c.remover.Remove(ctx, functionName, functionNamespace) + serviceRemovalErrCh <- c.remover.Remove(ctx, name, namespace) }() - - var errResources error - if deleteAll { - fmt.Fprintf(os.Stderr, "Removing Knative Service '%v' and all dependent resources\n", functionName) - // TODO: might not be necessary - cfg.Deploy.Namespace = functionNamespace - errResources = c.pipelinesProvider.Remove(ctx, cfg) + if all { + resourceRemovalError = c.pipelinesProvider.Remove(ctx, + Function{Name: name, Deploy: DeploySpec{Namespace: namespace}}) } + serviceRemovalError := <-serviceRemovalErrCh - errService := <-errChan - - if errService != nil && errResources != nil { - return fmt.Errorf("%s\n%s", errService, errResources) - } else if errResources != nil { - return errResources - } - return errService + // Return a combined error + return func(e1, e2 error) error { + if e1 == nil && e2 == nil { + return nil + } + if e1 != nil && e2 != nil { + return errors.New(e1.Error() + "\n" + e2.Error()) + } + if e1 != nil { + return e1 + } + return e2 + }(serviceRemovalError, resourceRemovalError) } // Invoke is a convenience method for triggering the execution of a function @@ -1346,12 +1351,12 @@ func (n *noopRemover) Remove(context.Context, string, string) error { return nil // Lister type noopLister struct{ output io.Writer } -func (n *noopLister) List(context.Context) ([]ListItem, error) { return []ListItem{}, nil } +func (n *noopLister) List(context.Context, string) ([]ListItem, error) { return []ListItem{}, nil } // Describer type noopDescriber struct{ output io.Writer } -func (n *noopDescriber) Describe(context.Context, string) (Instance, error) { +func (n *noopDescriber) Describe(context.Context, string, string) (Instance, error) { return Instance{}, nil } diff --git a/pkg/functions/client_int_test.go b/pkg/functions/client_int_test.go index 5bbd3acd1e..966f821d2a 100644 --- a/pkg/functions/client_int_test.go +++ b/pkg/functions/client_int_test.go @@ -61,14 +61,14 @@ func TestList(t *testing.T) { verbose := true // Assemble - lister := knative.NewLister(DefaultNamespace, verbose) + lister := knative.NewLister(verbose) client := fn.New( fn.WithLister(lister), fn.WithVerbose(verbose)) // Act - names, err := client.List(context.Background()) + names, err := client.List(context.Background(), DefaultNamespace) if err != nil { t.Fatal(err) } @@ -89,13 +89,13 @@ func TestNew(t *testing.T) { client := newClient(verbose) // Act - if _, _, err := client.New(context.Background(), fn.Function{Name: name, Root: root, Runtime: "go"}); err != nil { + if _, _, err := client.New(context.Background(), fn.Function{Name: name, Namespace: DefaultNamespace, Root: root, Runtime: "go"}); err != nil { t.Fatal(err) } - defer del(t, client, name) + defer del(t, client, name, DefaultNamespace) // Assert - items, err := client.List(context.Background()) + items, err := client.List(context.Background(), DefaultNamespace) names := []string{} for _, item := range items { names = append(names, item.Name) @@ -114,7 +114,7 @@ func TestDeploy(t *testing.T) { verbose := true client := newClient(verbose) - f := fn.Function{Name: "deploy", Root: ".", Runtime: "go"} + f := fn.Function{Name: "deploy", Namespace: DefaultNamespace, Root: ".", Runtime: "go"} var err error if f, err = client.Init(f); err != nil { @@ -127,7 +127,7 @@ func TestDeploy(t *testing.T) { t.Fatal(err) } - defer del(t, client, "deploy") + defer del(t, client, "deploy", DefaultNamespace) // TODO: gauron99 -- remove this when you set full image name after build instead // of push -- this has to be here because of a workaround f.Deploy.Image = f.Build.Image @@ -171,7 +171,7 @@ func TestDeployWithOptions(t *testing.T) { if _, _, err := client.New(context.Background(), f); err != nil { t.Fatal(err) } - defer del(t, client, "test-deploy-with-options") + defer del(t, client, "test-deploy-with-options", DefaultNamespace) } func TestDeployWithTriggers(t *testing.T) { @@ -179,7 +179,7 @@ func TestDeployWithTriggers(t *testing.T) { defer cleanup() verbose := true - f := fn.Function{Runtime: "go", Name: "test-deploy-with-triggers", Root: root} + f := fn.Function{Runtime: "go", Name: "test-deploy-with-triggers", Root: root, Namespace: DefaultNamespace} f.Deploy = fn.DeploySpec{ Subscriptions: []fn.KnativeSubscription{ { @@ -196,7 +196,7 @@ func TestDeployWithTriggers(t *testing.T) { if _, _, err := client.New(context.Background(), f); err != nil { t.Fatal(err) } - defer del(t, client, "test-deploy-with-triggers") + defer del(t, client, "test-deploy-with-triggers", DefaultNamespace) } func TestUpdateWithAnnotationsAndLabels(t *testing.T) { @@ -208,12 +208,12 @@ func TestUpdateWithAnnotationsAndLabels(t *testing.T) { // Deploy a function without any annotations or labels client := newClient(verbose) - f := fn.Function{Name: functionName, Root: ".", Runtime: "go"} + f := fn.Function{Name: functionName, Root: ".", Runtime: "go", Namespace: DefaultNamespace} if _, f, err = client.New(context.Background(), f); err != nil { t.Fatal(err) } - defer del(t, client, functionName) + defer del(t, client, functionName, DefaultNamespace) // Updated function with a new set of annotations and labels // deploy and check that deployed kcsv contains correct annotations and labels @@ -291,24 +291,24 @@ func TestUpdateWithAnnotationsAndLabels(t *testing.T) { } } -// TestRemove deletes +// TestRemove ensures removal of a function instance. func TestRemove(t *testing.T) { defer Within(t, "testdata/example.com/remove")() verbose := true client := newClient(verbose) - f := fn.Function{Name: "remove", Root: ".", Runtime: "go"} + f := fn.Function{Name: "remove", Namespace: DefaultNamespace, Root: ".", Runtime: "go"} var err error if _, f, err = client.New(context.Background(), f); err != nil { t.Fatal(err) } - waitFor(t, client, "remove") + waitFor(t, client, "remove", DefaultNamespace) - if err = client.Remove(context.Background(), f, false); err != nil { + if err = client.Remove(context.Background(), "", "", f, false); err != nil { t.Fatal(err) } - names, err := client.List(context.Background()) + names, err := client.List(context.Background(), DefaultNamespace) if err != nil { t.Fatal(err) } @@ -382,7 +382,7 @@ func TestInvoke_ClientToService(t *testing.T) { defer done() // Create a function - f := fn.Function{Name: "f", Runtime: "go"} + f := fn.Function{Name: "f", Runtime: "go", Namespace: DefaultNamespace} f, err = client.Init(f) if err != nil { t.Fatal(err) @@ -404,7 +404,7 @@ func Handle(ctx context.Context, res http.ResponseWriter, req *http.Request) { if route, f, err = client.Apply(ctx, f); err != nil { t.Fatal(err) } - defer client.Remove(ctx, f, true) + defer client.Remove(ctx, "", "", f, true) // Invoke via the route resp, err := http.Get(route) @@ -448,7 +448,7 @@ func TestInvoke_ServiceToService(t *testing.T) { // A function which responds to GET requests with a static value. root, done := Mktemp(t) defer done() - f := fn.Function{Name: "a", Runtime: "go"} + f := fn.Function{Name: "a", Runtime: "go", Namespace: DefaultNamespace} f, err = client.Init(f) if err != nil { t.Fatal(err) @@ -469,7 +469,7 @@ func Handle(ctx context.Context, res http.ResponseWriter, req *http.Request) { if _, f, err = client.Apply(ctx, f); err != nil { t.Fatal(err) } - defer client.Remove(ctx, f, true) + defer client.Remove(ctx, "", "", f, true) // Create Function B // which responds with the response from an invocation of 'a' via the @@ -525,7 +525,7 @@ func Handle(ctx context.Context, w http.ResponseWriter, req *http.Request) { if route, f, err = client.Apply(ctx, f); err != nil { t.Fatal(err) } - defer client.Remove(ctx, f, true) + defer client.Remove(ctx, "", "", f, true) resp, err := http.Get(route) if err != nil { @@ -552,10 +552,10 @@ func Handle(ctx context.Context, w http.ResponseWriter, req *http.Request) { func newClient(verbose bool) *fn.Client { builder := buildpacks.NewBuilder(buildpacks.WithVerbose(verbose)) pusher := docker.NewPusher(docker.WithVerbose(verbose)) - deployer := knative.NewDeployer(knative.WithDeployerNamespace(DefaultNamespace), knative.WithDeployerVerbose(verbose)) - describer := knative.NewDescriber(DefaultNamespace, verbose) + deployer := knative.NewDeployer(knative.WithDeployerVerbose(verbose)) + describer := knative.NewDescriber(verbose) remover := knative.NewRemover(verbose) - lister := knative.NewLister(DefaultNamespace, verbose) + lister := knative.NewLister(verbose) return fn.New( fn.WithRegistry(DefaultRegistry), @@ -579,10 +579,11 @@ func newClient(verbose bool) *fn.Client { // Of course, ideally this would be replaced by the use of a synchronous // method, or at a minimum a way to register a callback/listener for the // creation event. This is what we have for now, and the show must go on. -func del(t *testing.T, c *fn.Client, name string) { +func del(t *testing.T, c *fn.Client, name, namespace string) { t.Helper() - waitFor(t, c, name) - if err := c.Remove(context.Background(), fn.Function{Name: name, Deploy: fn.DeploySpec{Namespace: DefaultNamespace}}, false); err != nil { + waitFor(t, c, name, namespace) + f := fn.Function{Name: name, Deploy: fn.DeploySpec{Namespace: DefaultNamespace}} + if err := c.Remove(context.Background(), "", "", f, false); err != nil { t.Fatal(err) } cli, _, err := docker.NewClient(client.DefaultDockerHost) @@ -612,12 +613,12 @@ func del(t *testing.T, c *fn.Client, name string) { // TODO: the API should be synchronous, but that depends first on // Create returning the derived name such that we can bake polling in. // Ideally the provider's Create would be made syncrhonous. -func waitFor(t *testing.T, c *fn.Client, name string) { +func waitFor(t *testing.T, c *fn.Client, name, namespace string) { t.Helper() var pollInterval = 2 * time.Second for { // ever (i.e. defer to global test timeout) - nn, err := c.List(context.Background()) + nn, err := c.List(context.Background(), namespace) if err != nil { t.Fatal(err) } diff --git a/pkg/functions/client_test.go b/pkg/functions/client_test.go index fa3f3778ef..f579069ba8 100644 --- a/pkg/functions/client_test.go +++ b/pkg/functions/client_test.go @@ -984,8 +984,8 @@ func TestClient_Deploy_NamespaceUpdate(t *testing.T) { ) // New runs build and deploy, thus the initial instantiation should result in - // the namespace member being populated into the most default namespace - if _, f, err = client.New(ctx, fn.Function{Runtime: "go", Name: "f", Root: root}); err != nil { + // the namespace member being populated into the given namespace + if _, f, err = client.New(ctx, fn.Function{Runtime: "go", Name: "f", Namespace: "initialnamespace", Root: root}); err != nil { t.Fatal(err) } if f.Deploy.Namespace == "" { @@ -994,23 +994,16 @@ func TestClient_Deploy_NamespaceUpdate(t *testing.T) { // change deployed namespace to simulate already deployed function -- should // take precedence - f.Deploy.Namespace = "alreadydeployed" + f.Namespace = "secondnamespace" f, err = client.Deploy(ctx, f) if err != nil { t.Fatal(err) } - if f.Deploy.Namespace != "alreadydeployed" { + if f.Deploy.Namespace != "secondnamespace" { err = fmt.Errorf("namespace should match the already deployed function ns") t.Fatal(err) } - - // desired namespace takes precedence - f.Namespace = "desiredns" - f, err = client.Deploy(ctx, f) - if err != nil { - t.Fatal(err) - } } // TestClient_Remove_ByPath ensures that the remover is invoked to remove @@ -1042,7 +1035,7 @@ func TestClient_Remove_ByPath(t *testing.T) { return nil } - if err := client.Remove(context.Background(), f, false); err != nil { + if err := client.Remove(context.Background(), "", "", f, false); err != nil { t.Fatal(err) } @@ -1085,7 +1078,7 @@ func TestClient_Remove_DeleteAll(t *testing.T) { return nil } - if err := client.Remove(context.Background(), f, deleteAll); err != nil { + if err := client.Remove(context.Background(), "", "", f, deleteAll); err != nil { t.Fatal(err) } @@ -1132,7 +1125,7 @@ func TestClient_Remove_Dont_DeleteAll(t *testing.T) { return nil } - if err := client.Remove(context.Background(), f, deleteAll); err != nil { + if err := client.Remove(context.Background(), "", "", f, deleteAll); err != nil { t.Fatal(err) } @@ -1174,12 +1167,12 @@ func TestClient_Remove_ByName(t *testing.T) { } // Run remove with name (and namespace in .Deploy to simulate deployed function) - if err := client.Remove(context.Background(), fn.Function{Name: expectedName, Deploy: fn.DeploySpec{Namespace: namespace}}, false); err != nil { + if err := client.Remove(context.Background(), "", "", fn.Function{Name: expectedName, Deploy: fn.DeploySpec{Namespace: namespace}}, false); err != nil { t.Fatal(err) } // Run remove with a name and a root, which should be ignored in favor of the name. - if err := client.Remove(context.Background(), fn.Function{Name: expectedName, Root: root, Deploy: fn.DeploySpec{Namespace: namespace}}, false); err != nil { + if err := client.Remove(context.Background(), "", "", fn.Function{Name: expectedName, Root: root, Deploy: fn.DeploySpec{Namespace: namespace}}, false); err != nil { t.Fatal(err) } @@ -1210,7 +1203,7 @@ func TestClient_Remove_UninitializedFails(t *testing.T) { fn.WithRemover(remover)) // Attempt to remove by path (uninitialized), expecting an error. - if err := client.Remove(context.Background(), fn.Function{Root: root}, false); err == nil { + if err := client.Remove(context.Background(), "", "", fn.Function{Root: root}, false); err == nil { t.Fatalf("did not received expeced error removing an uninitialized func") } } @@ -1221,7 +1214,7 @@ func TestClient_List(t *testing.T) { client := fn.New(fn.WithLister(lister)) // lists deployed functions. - if _, err := client.List(context.Background()); err != nil { + if _, err := client.List(context.Background(), ""); err != nil { t.Fatal(err) } @@ -1240,7 +1233,7 @@ func TestClient_List_OutsideRoot(t *testing.T) { // Instantiate in the current working directory, with no name. client := fn.New(fn.WithLister(lister)) - if _, err := client.List(context.Background()); err != nil { + if _, err := client.List(context.Background(), ""); err != nil { t.Fatal(err) } @@ -1261,9 +1254,15 @@ func TestClient_Deploy_Image(t *testing.T) { client := fn.New( fn.WithBuilder(mock.NewBuilder()), fn.WithDeployer(mock.NewDeployer()), - fn.WithRegistry("example.com/alice")) + ) - f, err := client.Init(fn.Function{Name: "myfunc", Runtime: "go", Root: root}) + f, err := client.Init(fn.Function{ + Name: "myfunc", + Namespace: "initialnamespace", + Runtime: "go", + Root: root, + Registry: TestRegistry, + }) if err != nil { t.Fatal(err) } @@ -1331,9 +1330,10 @@ func TestClient_Pipelines_Deploy_Image(t *testing.T) { fn.WithRegistry("example.com/alice")) f := fn.Function{ - Name: "myfunc", - Runtime: "node", - Root: root, + Name: "myfunc", + Namespace: "initialnamespace", + Runtime: "node", + Root: root, Build: fn.BuildSpec{ Git: fn.Git{URL: "http://example-git.com/alice/myfunc.git"}, }, diff --git a/pkg/functions/function.go b/pkg/functions/function.go index 66436edb93..f5cbd998c0 100644 --- a/pkg/functions/function.go +++ b/pkg/functions/function.go @@ -681,7 +681,9 @@ func (f Function) Built() bool { // If f.Image is specified, registry is overridden -- meaning its not taken into // consideration and can be different from actually built image. - if !strings.Contains(f.Build.Image, f.Registry) && f.Image == "" { + buildImage := f.Build.Image + fRegistry := f.Registry + if !strings.Contains(buildImage, fRegistry) && f.Image == "" { fmt.Fprintf(os.Stderr, "Warning: registry '%s' does not match currently built image '%s' and no direct image override was provided via --image\n", f.Registry, f.Build.Image) return false } diff --git a/pkg/functions/instances.go b/pkg/functions/instances.go index 529bb9602d..b90ea35a0b 100644 --- a/pkg/functions/instances.go +++ b/pkg/functions/instances.go @@ -112,5 +112,5 @@ func (s *InstanceRefs) Remote(ctx context.Context, name, root string) (Instance, return Instance{}, nil } - return s.client.describer.Describe(ctx, f.Name) + return s.client.describer.Describe(ctx, f.Name, f.Deploy.Namespace) } diff --git a/pkg/knative/deployer.go b/pkg/knative/deployer.go index 836f3cfded..f441c048f6 100644 --- a/pkg/knative/deployer.go +++ b/pkg/knative/deployer.go @@ -33,9 +33,6 @@ import ( const LIVENESS_ENDPOINT = "/health/liveness" const READINESS_ENDPOINT = "/health/readiness" -// static default namespace for deployer -const StaticDefaultNamespace = "func" - type DeployDecorator interface { UpdateAnnotations(fn.Function, map[string]string) map[string]string UpdateLabels(fn.Function, map[string]string) map[string]string @@ -44,17 +41,14 @@ type DeployDecorator interface { type DeployerOpt func(*Deployer) type Deployer struct { - // Namespace with which to override that set on the default configuration (such as the ~/.kube/config). - // If left blank, deployment will commence to the configured namespace. - Namespace string // verbose logging enablement flag. verbose bool decorator DeployDecorator } -// ActiveNamespace attempts to read the kubernetes active namepsace. -// Missing configs or not having an active kuberentes configuration are +// ActiveNamespace attempts to read the Kubernetes active namespace. +// Missing configs or not having an active Kubernetes configuration are // equivalent to having no default namespace (empty string). func ActiveNamespace() string { // Get client config, if it exists, and from that the namespace @@ -75,12 +69,6 @@ func NewDeployer(opts ...DeployerOpt) *Deployer { return d } -func WithDeployerNamespace(namespace string) DeployerOpt { - return func(d *Deployer) { - d.Namespace = namespace - } -} - func WithDeployerVerbose(verbose bool) DeployerOpt { return func(d *Deployer) { d.verbose = verbose @@ -104,7 +92,7 @@ func (d *Deployer) isImageInPrivateRegistry(ctx context.Context, client clientse if err != nil { return false } - list, err := k8sClient.CoreV1().Pods(namespace(d.Namespace, f)).List(ctx, metav1.ListOptions{ + list, err := k8sClient.CoreV1().Pods(f.Deploy.Namespace).List(ctx, metav1.ListOptions{ LabelSelector: "serving.knative.dev/revision=" + ksvc.Status.LatestCreatedRevisionName + ",serving.knative.dev/service=" + f.Name, FieldSelector: "status.phase=Pending", }) @@ -123,40 +111,25 @@ func (d *Deployer) isImageInPrivateRegistry(ctx context.Context, client clientse return false } -// returns correct namespace to deploy to, ordered in a descending order by -// priority: User specified via cli -> client WithDeployer -> already deployed -> -// -> k8s default; if fails, use static default -func namespace(dflt string, f fn.Function) string { - // namespace ordered by highest priority decending +func (d *Deployer) Deploy(ctx context.Context, f fn.Function) (fn.DeploymentResult, error) { + // Choosing f.Namespace vs f.Deploy.Namespace: + // This is minimal logic currently required of all deployer impls. + // If f.Namespace is defined, this is the (possibly new) target + // namespace. Otherwise use the last deployed namespace. Error if + // neither are set. The logic which arbitrates between curret k8s context, + // flags, environment variables and global defaults to determine the + // effective namespace is not logic for the deployer implementation, which + // should have a minimum of logic. In this case limited to "new ns or + // existing namespace? namespace := f.Namespace - - // if deployed before: use already deployed namespace if namespace == "" { namespace = f.Deploy.Namespace } - - // deployer WithDeployerNamespace provided if namespace == "" { - namespace = dflt + return fn.DeploymentResult{}, fmt.Errorf("deployer requires either a target namespace or that the function be already deployed.") } - if namespace == "" { - var err error - // still not set, just use the defaultest default - namespace, err = k8s.GetDefaultNamespace() - if err != nil { - fmt.Fprintf(os.Stderr, "trying to get default namespace returns an error: '%s'\nSetting static default namespace '%s'", err, StaticDefaultNamespace) - namespace = StaticDefaultNamespace - } - } - return namespace -} - -func (d *Deployer) Deploy(ctx context.Context, f fn.Function) (fn.DeploymentResult, error) { - - // returns correct namespace by priority - namespace := namespace(d.Namespace, f) - + // Clients client, err := NewServingClient(namespace) if err != nil { return fn.DeploymentResult{}, err diff --git a/pkg/knative/deployer_test.go b/pkg/knative/deployer_test.go index 88e6694625..90204062c7 100644 --- a/pkg/knative/deployer_test.go +++ b/pkg/knative/deployer_test.go @@ -4,36 +4,14 @@ package knative import ( - "fmt" "os" "testing" corev1 "k8s.io/api/core/v1" + fn "knative.dev/func/pkg/functions" ) -// Test_DefaultNamespace ensures that if there is an active kubeconfig, -// that namespace will be returned as the default from the public -// DefaultNamespace accessor, empty string otherwise. -func Test_DefaultNamespace(t *testing.T) { - // Update Kubeconfig to indicate the currently active namespace is: - // "test-ns-deploy" - t.Setenv("KUBECONFIG", fmt.Sprintf("%s/testdata/test_default_namespace", cwd())) - - if ActiveNamespace() != "test-ns-deploy" { - t.Fatalf("expected 'test-ns-deploy', got '%v'", ActiveNamespace()) - } -} - -func cwd() (cwd string) { - cwd, err := os.Getwd() - if err != nil { - fmt.Fprintf(os.Stderr, "Unable to determine current working directory: %v", err) - os.Exit(1) - } - return cwd -} - func Test_setHealthEndpoints(t *testing.T) { f := fn.Function{ Name: "testing", @@ -102,59 +80,16 @@ func Test_processValue(t *testing.T) { {name: "bad context", arg: "{{secret:S}}", want: "", wantErr: true}, {name: "unset envvar", arg: "{{env:SOME_UNSET_VAR}}", want: "", wantErr: true}, } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - got, err := processLocalEnvValue(tt.arg) - if (err != nil) != tt.wantErr { - t.Errorf("processValue() error = %v, wantErr %v", err, tt.wantErr) + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + got, err := processLocalEnvValue(test.arg) + if (err != nil) != test.wantErr { + t.Errorf("processValue() error = %v, wantErr %v", err, test.wantErr) return } - if got != tt.want { - t.Errorf("processValue() got = %v, want %v", got, tt.want) + if got != test.want { + t.Errorf("processValue() got = %v, want %v", got, test.want) } }) } } - -// Test_deployerNamespace tests that namespace function returns what it should -// via preferences. namespace() is used in knative deployer to determine what -// namespace to deploy the function to. -func Test_deployerNamespace(t *testing.T) { - // these are the namespaces being used descending in preference (top = highest pref) - var ( - desiredNs = "desiredNs" - deployedNs = "deployedNs" - deployerNs = "deployerNs" - defaultNs = "test-ns-deploy" - // StaticDefaultNamespace -- is exported - ) - f := fn.Function{Name: "myfunc"} - - //set static default - if ns := namespace("", f); ns != StaticDefaultNamespace { - t.Fatal("expected static default namespace") - } - t.Setenv("KUBECONFIG", fmt.Sprintf("%s/testdata/test_default_namespace", cwd())) - - // active kubernetes default - if ns := namespace("", f); ns != defaultNs { - t.Fatal("expected default k8s namespace") - } - - // knative deployer namespace - if ns := namespace(deployerNs, f); ns != deployerNs { - t.Fatal("expected knative deployer namespace") - } - - // already deployed namespace - f.Deploy.Namespace = deployedNs - if ns := namespace(deployerNs, f); ns != deployedNs { - t.Fatal("expected namespace where function is already deployed") - } - - // desired namespace - f.Namespace = desiredNs - if ns := namespace(deployerNs, f); ns != desiredNs { - t.Fatal("expected desired namespace defined via f.Namespace") - } -} diff --git a/pkg/knative/describer.go b/pkg/knative/describer.go index 0c8ca056bd..46a003f16a 100644 --- a/pkg/knative/describer.go +++ b/pkg/knative/describer.go @@ -2,45 +2,42 @@ package knative import ( "context" + "fmt" "k8s.io/apimachinery/pkg/api/errors" clientservingv1 "knative.dev/client-pkg/pkg/serving/v1" eventingv1 "knative.dev/eventing/pkg/apis/eventing/v1" fn "knative.dev/func/pkg/functions" - "knative.dev/func/pkg/k8s" ) type Describer struct { - namespace string - verbose bool + verbose bool } -func NewDescriber(namespaceOverride string, verbose bool) *Describer { +func NewDescriber(verbose bool) *Describer { return &Describer{ - namespace: namespaceOverride, - verbose: verbose, + verbose: verbose, } } -// Describe by name. Note that the consuming API uses domain style notation, whereas Kubernetes -// restricts to label-syntax, which is thus escaped. Therefore as a knative (kube) implementation -// detal proper full names have to be escaped on the way in and unescaped on the way out. ex: +// Describe a function by name. Note that the consuming API uses domain style +// notation, whereas Kubernetes restricts to label-syntax, which is thus +// escaped. Therefore as a knative (kube) implementation detal proper full +// names have to be escaped on the way in and unescaped on the way out. ex: // www.example-site.com -> www-example--site-com -func (d *Describer) Describe(ctx context.Context, name string) (description fn.Instance, err error) { - if d.namespace == "" { - d.namespace, err = k8s.GetDefaultNamespace() - if err != nil { - return fn.Instance{}, err - } +func (d *Describer) Describe(ctx context.Context, name, namespace string) (description fn.Instance, err error) { + if namespace == "" { + err = fmt.Errorf("function namespace is required when describing %q", name) + return } - servingClient, err := NewServingClient(d.namespace) + servingClient, err := NewServingClient(namespace) if err != nil { return } - eventingClient, err := NewEventingClient(d.namespace) + eventingClient, err := NewEventingClient(namespace) if err != nil { return } @@ -66,7 +63,7 @@ func (d *Describer) Describe(ctx context.Context, name string) (description fn.I } description.Name = name - description.Namespace = d.namespace + description.Namespace = namespace description.Route = primaryRouteURL description.Routes = routeURLs diff --git a/pkg/knative/integration_test.go b/pkg/knative/integration_test.go index 5df5a18ff1..c05de554c6 100644 --- a/pkg/knative/integration_test.go +++ b/pkg/knative/integration_test.go @@ -159,7 +159,7 @@ func TestIntegration(t *testing.T) { _ = knative.GetKServiceLogs(ctx, namespace, functionName, function.Deploy.Image, &now, buff) }() - deployer := knative.NewDeployer(knative.WithDeployerNamespace(namespace), knative.WithDeployerVerbose(false)) + deployer := knative.NewDeployer(knative.WithDeployerVerbose(false)) depRes, err := deployer.Deploy(ctx, function) if err != nil { @@ -194,8 +194,8 @@ func TestIntegration(t *testing.T) { t.Error("config-map was not mounted") } - describer := knative.NewDescriber(namespace, false) - instance, err := describer.Describe(ctx, functionName) + describer := knative.NewDescriber(false) + instance, err := describer.Describe(ctx, functionName, namespace) if err != nil { t.Fatal(err) } @@ -228,8 +228,8 @@ func TestIntegration(t *testing.T) { } } - lister := knative.NewLister(namespace, false) - list, err := lister.List(ctx) + lister := knative.NewLister(false) + list, err := lister.List(ctx, namespace) if err != nil { t.Fatal(err) } @@ -277,7 +277,7 @@ func TestIntegration(t *testing.T) { t.Fatal(err) } - list, err = lister.List(ctx) + list, err = lister.List(ctx, namespace) if err != nil { t.Fatal(err) } diff --git a/pkg/knative/lister.go b/pkg/knative/lister.go index 0328648a80..124e5e9cd0 100644 --- a/pkg/knative/lister.go +++ b/pkg/knative/lister.go @@ -8,31 +8,20 @@ import ( "knative.dev/pkg/apis" fn "knative.dev/func/pkg/functions" - "knative.dev/func/pkg/k8s" "knative.dev/func/pkg/k8s/labels" ) type Lister struct { - Namespace string - verbose bool + verbose bool } -func NewLister(namespaceOverride string, verbose bool) *Lister { - return &Lister{ - Namespace: namespaceOverride, - verbose: verbose, - } +func NewLister(verbose bool) *Lister { + return &Lister{verbose: verbose} } -func (l *Lister) List(ctx context.Context) (items []fn.ListItem, err error) { - if l.Namespace == "" { - l.Namespace, err = k8s.GetDefaultNamespace() - if err != nil { - return nil, err - } - } - - client, err := NewServingClient(l.Namespace) +// List functions, optionally specifying a namespace. +func (l *Lister) List(ctx context.Context, namespace string) (items []fn.ListItem, err error) { + client, err := NewServingClient(namespace) if err != nil { return } @@ -48,7 +37,7 @@ func (l *Lister) List(ctx context.Context) (items []fn.ListItem, err error) { return } - listOfFunctions := lst.Items[:] + services := lst.Items[:] for i, depLabelF := range lstDeprecated.Items { found := false for _, f := range lst.Items { @@ -58,16 +47,16 @@ func (l *Lister) List(ctx context.Context) (items []fn.ListItem, err error) { } } if !found { - listOfFunctions = append(listOfFunctions, lstDeprecated.Items[i]) + services = append(services, lstDeprecated.Items[i]) } } // --- end of handling usage of deprecated function labels - for _, f := range listOfFunctions { + for _, service := range services { // get status ready := corev1.ConditionUnknown - for _, con := range f.Status.Conditions { + for _, con := range service.Status.Conditions { if con.Type == apis.ConditionReady { ready = con.Status break @@ -76,18 +65,18 @@ func (l *Lister) List(ctx context.Context) (items []fn.ListItem, err error) { // --- handle usage of deprecated runtime labels (`boson.dev/runtime`) runtimeLabel := "" - if val, ok := f.Labels[labels.FunctionRuntimeKey]; ok { + if val, ok := service.Labels[labels.FunctionRuntimeKey]; ok { runtimeLabel = val } else { - runtimeLabel = f.Labels[labels.DeprecatedFunctionRuntimeKey] + runtimeLabel = service.Labels[labels.DeprecatedFunctionRuntimeKey] } // --- end of handling usage of deprecated runtime labels listItem := fn.ListItem{ - Name: f.Name, - Namespace: f.Namespace, + Name: service.Name, + Namespace: service.Namespace, Runtime: runtimeLabel, - URL: f.Status.URL.String(), + URL: service.Status.URL.String(), Ready: string(ready), } diff --git a/pkg/mock/deployer.go b/pkg/mock/deployer.go index 7a4ecb44e2..456b8494e8 100644 --- a/pkg/mock/deployer.go +++ b/pkg/mock/deployer.go @@ -2,6 +2,7 @@ package mock import ( "context" + "errors" fn "knative.dev/func/pkg/functions" ) @@ -20,16 +21,17 @@ type Deployer struct { func NewDeployer() *Deployer { return &Deployer{ - DeployFn: func(_ context.Context, f fn.Function) (fn.DeploymentResult, error) { - result := fn.DeploymentResult{} - result.Namespace = f.Namespace - if result.Namespace == "" { - result.Namespace = f.Deploy.Namespace + DeployFn: func(_ context.Context, f fn.Function) (result fn.DeploymentResult, err error) { + // the minimum necessary logic for a deployer, which should be + // confirmed by tests in the respective implementations. + if f.Namespace != "" { + result.Namespace = f.Namespace // deployed to that requested + } else if f.Deploy.Namespace != "" { + result.Namespace = f.Deploy.Namespace // redeploy to current + } else { + err = errors.New("namespace required for initial deployment") } - if result.Namespace == "" { - result.Namespace = DefaultNamespace - } - return result, nil + return }, } } diff --git a/pkg/mock/describer.go b/pkg/mock/describer.go index 710482192d..c9a6f7a760 100644 --- a/pkg/mock/describer.go +++ b/pkg/mock/describer.go @@ -8,16 +8,16 @@ import ( type Describer struct { DescribeInvoked bool - DescribeFn func(string) (fn.Instance, error) + DescribeFn func(context.Context, string, string) (fn.Instance, error) } func NewDescriber() *Describer { return &Describer{ - DescribeFn: func(string) (fn.Instance, error) { return fn.Instance{}, nil }, + DescribeFn: func(context.Context, string, string) (fn.Instance, error) { return fn.Instance{}, nil }, } } -func (l *Describer) Describe(_ context.Context, name string) (fn.Instance, error) { +func (l *Describer) Describe(ctx context.Context, name, namespace string) (fn.Instance, error) { l.DescribeInvoked = true - return l.DescribeFn(name) + return l.DescribeFn(ctx, name, namespace) } diff --git a/pkg/mock/lister.go b/pkg/mock/lister.go index 2993d52dc5..32a1dfe72c 100644 --- a/pkg/mock/lister.go +++ b/pkg/mock/lister.go @@ -8,16 +8,16 @@ import ( type Lister struct { ListInvoked bool - ListFn func() ([]fn.ListItem, error) + ListFn func(context.Context, string) ([]fn.ListItem, error) } func NewLister() *Lister { return &Lister{ - ListFn: func() ([]fn.ListItem, error) { return []fn.ListItem{}, nil }, + ListFn: func(context.Context, string) ([]fn.ListItem, error) { return []fn.ListItem{}, nil }, } } -func (l *Lister) List(context.Context) ([]fn.ListItem, error) { +func (l *Lister) List(ctx context.Context, ns string) ([]fn.ListItem, error) { l.ListInvoked = true - return l.ListFn() + return l.ListFn(ctx, ns) } diff --git a/pkg/mock/pipelines_provider.go b/pkg/mock/pipelines_provider.go index 3abd438227..9c96bfa469 100644 --- a/pkg/mock/pipelines_provider.go +++ b/pkg/mock/pipelines_provider.go @@ -2,6 +2,7 @@ package mock import ( "context" + "errors" fn "knative.dev/func/pkg/functions" ) @@ -19,17 +20,17 @@ type PipelinesProvider struct { func NewPipelinesProvider() *PipelinesProvider { return &PipelinesProvider{ - RunFn: func(f fn.Function) (string, string, error) { - // simplified namespace resolution, doesnt take current k8s context into - // account and returns DefaultNamespace if nothing else instead - ns := f.Namespace - if ns == "" { - ns = f.Deploy.Namespace + RunFn: func(f fn.Function) (x string, namespace string, err error) { + // the minimum necessary logic for a deployer, which should be + // confirmed by tests in the respective implementations. + if f.Namespace != "" { + namespace = f.Namespace + } else if f.Deploy.Namespace != "" { + namespace = f.Deploy.Namespace + } else { + err = errors.New("namespace required for initial deployment") } - if ns == "" { - ns = DefaultNamespace - } - return "", ns, nil + return }, RemoveFn: func(fn.Function) error { return nil }, ConfigurePACFn: func(fn.Function) error { return nil }, diff --git a/pkg/pipelines/tekton/pipelines_provider.go b/pkg/pipelines/tekton/pipelines_provider.go index 7398037a7e..22d381a48a 100644 --- a/pkg/pipelines/tekton/pipelines_provider.go +++ b/pkg/pipelines/tekton/pipelines_provider.go @@ -37,8 +37,8 @@ import ( "knative.dev/pkg/apis" ) -// static const namespace for deployement when everything else fails -const StaticDefaultNamespace = "func" +// DefaultNamespace is the kubernetes default namespace +const DefaultNamespace = "default" // DefaultPersistentVolumeClaimSize to allocate for the function. var DefaultPersistentVolumeClaimSize = resource.MustParse("256Mi") @@ -578,8 +578,8 @@ func namespace(dflt string, f fn.Function) string { // still not set, just use the defaultest default namespace, err = k8s.GetDefaultNamespace() if err != nil { - fmt.Fprintf(os.Stderr, "trying to get default namespace returns an error: '%s'\nSetting static default namespace '%s'", err, StaticDefaultNamespace) - namespace = StaticDefaultNamespace + fmt.Fprintf(os.Stderr, "trying to get default namespace returns an error: '%s'\nSetting static default namespace '%s'", err, DefaultNamespace) + namespace = DefaultNamespace } } return namespace diff --git a/pkg/testing/testing.go b/pkg/testing/testing.go index 4b82d3aa46..9853dac9cb 100644 --- a/pkg/testing/testing.go +++ b/pkg/testing/testing.go @@ -214,6 +214,45 @@ func RunGitServer(root string, t *testing.T) (url string) { return "http://" + url } +// FromTempDirectory moves the test into a new temporary directory and +// clears all known interfering environment variables. Returned is the +// path to the somewhat isolated test environment. +// Note that KUBECONFIG is also set to testdata/default_kubeconfig which can +// be used for tests which are explicitly checking logic which depends on +// kube context. +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' + // NOTE: the below settings affect unit tests only, and we do explicitly + // want all unit tests to start in an empty environment with tests "opting in" + // to config, not opting out. + t.Setenv("KUBECONFIG", filepath.Join(Cwd(), "testdata", "default_kubeconfig")) + + // By default unit tests presum no config exists unless provided in testdata. + t.Setenv("XDG_CONFIG_HOME", t.TempDir()) + + t.Setenv("KUBERNETES_SERVICE_HOST", "") + + // creates and CDs to a temp directory + d, done := Mktemp(t) + + // Done and Reset + // NOTE: + // NO CLI command should require resetting viper. If a CLI test + // is failing, and the following fixes the problem, it's probably because + // an instance of a command is being reused multiple times in the same + // test when a new instance of the command struct should instead be + // created for each test case: + // t.Cleanup(func() { done(); viper.Reset() }) + t.Cleanup(done) + + return d +} + // Cwd returns the current working directory or panic if unable to determine. func Cwd() (cwd string) { cwd, err := os.Getwd()