diff --git a/cmd/client.go b/cmd/client.go index 36d65c96c4..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. @@ -63,15 +68,15 @@ func NewClient(cfg ClientConfig, options ...fn.Option) (*fn.Client, func()) { t = newTransport(cfg.InsecureSkipVerify) // may provide a custom impl which proxies c = newCredentialsProvider(config.Dir(), t) // for accessing registries d = newKnativeDeployer(cfg.Verbose) - pp = newTektonPipelinesProvider(cfg.Namespace, c, 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( diff --git a/cmd/client_test.go b/cmd/client_test.go index b7a63f1b5a..3c1ae8105a 100644 --- a/cmd/client_test.go +++ b/cmd/client_test.go @@ -24,30 +24,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) + // 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) } - f, err := fn.NewFunction("") - 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 d278bff5a9..15e9180d6c 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(cmd *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 b6e2cdab19..abfba8e283 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(cmd *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/delete.go b/cmd/delete.go index 2834fee839..a48523c4f5 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 { // Onterwise 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 307b2a9540..f836bfb681 100644 --- a/cmd/delete_test.go +++ b/cmd/delete_test.go @@ -14,40 +14,48 @@ import ( // with default options func TestDelete_Default(t *testing.T) { var ( + err error root = FromTempDirectory(t) - namespace = "myns" + name = "testdelete" + namespace = "testdeletedamespace" remover = mock.NewRemover() - err error + ctx = context.Background() ) - remover.RemoveFn = func(_, ns string) error { + remover.RemoveFn = func(n, ns string) error { + if name != name { + t.Fatalf("expected name '%v', got '%v'", name, n) + } if ns != namespace { - t.Fatalf("expected delete namespace '%v', got '%v'", namespace, ns) + t.Fatalf("expected namespace '%v', got '%v'", namespace, ns) } return nil } + client := fn.New( + fn.WithBuilder(mock.NewBuilder()), + fn.WithDeployer(mock.NewDeployer()), + fn.WithRemover(mock.NewRemover()), + ) + // Ensure the extant function's namespace is used 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 = client.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) } @@ -99,7 +107,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) } diff --git a/cmd/deploy.go b/cmd/deploy.go index b7ddfebe46..38d2ed37c7 100644 --- a/cmd/deploy.go +++ b/cmd/deploy.go @@ -151,8 +151,6 @@ EXAMPLES fmt.Sprintf("Builder to use when creating the function's container. Currently supported builders are %s.", KnownBuilders())) 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().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 @@ -192,6 +190,8 @@ EXAMPLES cmd.Flags().String("platform", "", "Optionally specify a specific platform to build for (e.g. linux/amd64). ($FUNC_PLATFORM)") 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)") // Oft-shared flags: addConfirmFlag(cmd, cfg.Confirm) @@ -234,30 +234,37 @@ func runDeploy(cmd *cobra.Command, newClient ClientFactory) (err error) { return } - // 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 will override a custom registry for OpenShift + // users who want to use something other than the internal registry. + // There is an open issue and associated PR which fixes this incoming. + 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 @@ -484,8 +491,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"), @@ -503,10 +510,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 ab54aec24a..53a9c23778 100644 --- a/cmd/deploy_test.go +++ b/cmd/deploy_test.go @@ -13,11 +13,7 @@ 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" @@ -902,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} @@ -915,64 +913,65 @@ 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 rever to using the default) + 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 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) } @@ -989,33 +988,41 @@ func TestDeploy_NamespaceDefaults(t *testing.T) { // 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 + // 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 + + /* TODO: check, because the below is probably not necessary: + // 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 { - result.Namespace = config.DefaultNamespace() + // 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) + // Execute a deploycommand with everything mocked cmd := NewDeployCmd(NewTestClient( fn.WithDeployer(deployer), fn.WithBuilder(mock.NewBuilder()), - fn.WithPipelinesProvider(mock.NewPipelinesProvider()), - fn.WithRegistry(TestRegistry), )) 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) @@ -1148,10 +1155,10 @@ func TestDeploy_NamespaceUpdateWarning(t *testing.T) { func TestDeploy_BasicRedeployInCorrectNamespace(t *testing.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 { @@ -1159,34 +1166,29 @@ 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=myns"}) if err := cmd.Execute(); err != nil { t.Fatal(err) } f, _ = fn.NewFunction(root) - if f.Deploy.Namespace == "" { + if f.Deploy.Namespace != "myns" { t.Fatal("expected deployed namespace to be specified after deploy") } // 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 != "" { + if f.Namespace != "myns" { t.Fatalf("no desired namespace should be specified but is %s", f.Namespace) } - if f.Deploy.Namespace == "" { + if f.Deploy.Namespace != "myns" { t.Fatal("expected deployed namespace to be specified after second deploy") } } @@ -1195,10 +1197,12 @@ func TestDeploy_BasicRedeployInCorrectNamespace(t *testing.T) { // and doesnt brake using pipelines func TestDeploy_BasicRedeployPipelinesCorrectNamespace(t *testing.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 { @@ -1209,7 +1213,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"}) @@ -1229,7 +1232,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) @@ -1775,8 +1778,9 @@ func TestReDeploy_OnRegistryChangeWithBuildFalse(t *testing.T) { } } -// 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) @@ -1785,15 +1789,32 @@ func TestDeploy_NoErrorOnOldFunctionNotFound(t *testing.T) { 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 forp + // 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 { @@ -1801,30 +1822,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 7b91bb5430..f6b4a5fdc7 100644 --- a/cmd/describe_test.go +++ b/cmd/describe_test.go @@ -1,6 +1,7 @@ package cmd import ( + "context" "testing" fn "knative.dev/func/pkg/functions" @@ -16,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 } @@ -39,9 +40,10 @@ func TestDescribe_ByName(t *testing.T) { // its name correctly. func TestDescribe_ByProject(t *testing.T) { root := FromTempDirectory(t) + expected := "testname" _, err := fn.New().Init(fn.Function{ - Name: "testname", + Name: expected, Runtime: "go", Registry: TestRegistry, Root: root, @@ -51,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 } @@ -78,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 b44da82b51..ea6851d5bc 100644 --- a/cmd/invoke.go +++ b/cmd/invoke.go @@ -153,7 +153,7 @@ func runInvoke(cmd *cobra.Command, args []string, newClient ClientFactory) (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 d99abdb740..02f19a92eb 100644 --- a/cmd/invoke_test.go +++ b/cmd/invoke_test.go @@ -78,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/list.go b/cmd/list.go index f0d2ed5a02..71f64bf8a1 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 likwise intended for command which require a + // namespace no matter what. Therefore the global namespace setting is + // not applicable to this command beacause "deafult" 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 ec04eb2c28..c90e5b48f4 100644 --- a/cmd/list_test.go +++ b/cmd/list_test.go @@ -1,6 +1,7 @@ package cmd import ( + "context" "testing" fn "knative.dev/func/pkg/functions" @@ -8,31 +9,45 @@ import ( . "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) 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 }, { @@ -44,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/root.go b/cmd/root.go index b9353a2ca4..7da3d1410c 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 @@ -150,6 +154,42 @@ func effectivePath() (path string) { return path } +// defaultNamespace to use when none is provided explicitly. +// This is first the active kubernetes namespace, followed by global config, +// with the static default "default" being the fallback. +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 99787fc67f..a97aa7807d 100644 --- a/cmd/root_test.go +++ b/cmd/root_test.go @@ -5,6 +5,7 @@ import ( "fmt" "io" "os" + "path/filepath" "reflect" "strings" "testing" @@ -13,6 +14,7 @@ import ( "knative.dev/client-pkg/pkg/util" fn "knative.dev/func/pkg/functions" + . "knative.dev/func/pkg/testing" ) const TestRegistry = "example.com/alice" @@ -269,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 // ------- 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/Test_namespace/kubeconfig b/cmd/testdata/Test_defaultNamespace/kubeconfig similarity index 100% rename from cmd/testdata/Test_namespace/kubeconfig rename to cmd/testdata/Test_defaultNamespace/kubeconfig diff --git a/pkg/config/config.go b/pkg/config/config.go index 4c8426b9c9..377b8517e8 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"` @@ -137,9 +124,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 60f54b70c6..0f0922479f 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 b4fc18c34a..f07b97df63 100644 --- a/pkg/functions/client.go +++ b/pkg/functions/client.go @@ -133,7 +133,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 { @@ -147,7 +147,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. @@ -468,6 +468,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. @@ -711,13 +712,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 } } @@ -725,10 +726,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() { @@ -737,7 +738,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 } @@ -747,44 +748,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") + if c.verbose { + fmt.Fprintf(os.Stderr, "⬆️ Deploying \n") + } result, err := c.deployer.Deploy(ctx, f) if err != nil { - fmt.Printf("deploy error: %v\n", err) - return f, err + return f, fmt.Errorf("deploy error. %w", err) + } + + 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 + // Update the function with its new namespace f.Deploy.Namespace = result.Namespace if result.Status == Deployed { @@ -948,28 +950,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 @@ -977,66 +991,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 @@ -1342,12 +1347,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 4ce5c2a4fa..1f6f59ea2a 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 31554a4e50..5b31ceea80 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/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 c04ebff94f..4a39767adb 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" -// DefaultNamespace for deployments when no other namespaces are provided. -const DefaultNamespace = "func" - type DeployDecorator interface { UpdateAnnotations(fn.Function, map[string]string) map[string]string UpdateLabels(fn.Function, map[string]string) map[string]string @@ -95,7 +92,7 @@ func (d *Deployer) isImageInPrivateRegistry(ctx context.Context, client clientse if err != nil { return false } - list, err := k8sClient.CoreV1().Pods(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", }) @@ -120,6 +117,9 @@ func (d *Deployer) isImageInPrivateRegistry(ctx context.Context, client clientse // - The last namespace to which the function was deployed via f.Deploy.Namespace // - The namespace of the curent kubernetes context if it exists // - The static default namespace DefaultNamespace +/* DEPRECATED: the deployer should expect the correct namespace to always be + passed in in order to keep implementations simple and complexity in the + core library. func namespace(f fn.Function) string { // namespace ordered by highest priority decending namespace := f.Namespace @@ -140,13 +140,25 @@ func namespace(f fn.Function) string { } return namespace } +*/ func (d *Deployer) Deploy(ctx context.Context, f fn.Function) (fn.DeploymentResult, error) { + // 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 namespace == "" { + namespace = f.Deploy.Namespace + } + if namespace == "" { + return fn.DeploymentResult{}, fmt.Errorf("deployer requires either a target namespace or that the funciton be already deployed.") + } - // derive the effective namespace by priority for the current - // deployment - namespace := 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 599d0a81bf..90204062c7 100644 --- a/pkg/knative/deployer_test.go +++ b/pkg/knative/deployer_test.go @@ -4,29 +4,14 @@ package knative import ( - "fmt" "os" "testing" corev1 "k8s.io/api/core/v1" fn "knative.dev/func/pkg/functions" - . "knative.dev/func/pkg/testing" ) -// 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 Test_setHealthEndpoints(t *testing.T) { f := fn.Function{ Name: "testing", @@ -108,60 +93,3 @@ func Test_processValue(t *testing.T) { }) } } - -// Test_namespace ensures the namespace function returns the correct namespace -// to use for the next deployment. -func Test_namespace(t *testing.T) { - // store path to test kubeconfig before changing working directory. - testKubeconfigPath := fmt.Sprintf("%s/testdata/test_default_namespace", Cwd()) - - tests := []struct { - testName, requested, current, expected string - active bool - }{ - { - testName: "Static default", - requested: "", // nothing requested (such as via the CLI) - current: "", // no current namespace (undeployed) - active: false, // no active k8s context to choose from - expected: DefaultNamespace, - }, { - testName: "Active k8s context", - requested: "", - current: "", - active: true, // use the test active k8s context - expected: "test-ns-deploy", // that is what is expected - }, { - testName: "Currently deployed", - requested: "", - current: "default", // currently deployed to "default" - active: true, // use the test active k8s context - expected: "default", // redeploy should take precidence - }, { - testName: "Move request", - requested: "func", // Requesting it be moved to "func" - current: "default", // currently deployed to "default" - active: true, // use the test active k8s context - expected: "func", // requested should take highest precidence - }, - } - for _, test := range tests { - t.Run(test.testName, func(t *testing.T) { - _ = FromTempDirectory(t) // clear existing envs and cd tmp - - // Populate a Function with the test settings - testname := test.testName - f := fn.Function{Name: "test"} - f.Namespace = test.requested - f.Deploy.Namespace = test.current - if test.active { - t.Setenv("KUBECONFIG", testKubeconfigPath) - } - - // Assert the correct namespace is evaluated as effective - if namespace(f) != test.expected { - t.Fatalf("%q expected namespace %q, got %q", testname, test.expected, namespace(f)) - } - }) - } -} 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..1644f49d2d 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 { 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 dbe60494df..9853dac9cb 100644 --- a/pkg/testing/testing.go +++ b/pkg/testing/testing.go @@ -215,9 +215,9 @@ func RunGitServer(root string, t *testing.T) (url string) { } // FromTempDirectory moves the test into a new temporary directory and -// clears all known interfereing environment variables. Returned is the +// 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 whcih can +// 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 {