Repository: knative-sandbox/kn-plugin-func
Pull request #647 (2021-11-10 12:01:19)

making language runtime a required flag
<!-- Thanks for sending a pull request! -->

# Changes

<!-- 
Describe your changes here- ideally you can get that description straight from
your descriptive commit message(s)! 

- :gift: Add new feature
- :bug: Fix bug
- :broom: Update or clean up current behavior
- :wastebasket: Remove feature or internal logic
-->

- :broom: add check to make language runtime a mandatory flag 

<!--
In addition, categorize the changes you're making using the "/kind" Prow command, example:

/kind 

Supported kinds are: api-change, bug, cleanup, deprecation, removal, documentation, enhancement, performance

-->
/kind enhancement

<!--
*Automatically closes linked issue when PR is merged.
Usage: `Fixes #<issue number>`, or `Fixes (paste link of issue)`.
_If PR is about `failing-tests or flakes`, please post the related issues/tests in a comment and do not use `Fixes`_*
-->
Fixes #
fixing AC1 of https://github.com/knative-sandbox/kn-plugin-func/issues/553



knative-prow-robot: [APPROVALNOTIFIER] This PR is **NOT APPROVED**

This pull-request has been approved by: *<a href="https://github.com/knative-sandbox/kn-plugin-func/pull/647#" title="Author self-approved">Shashankft9</a>*
To complete the [pull request process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process), please assign **zroubalik** after the PR has been reviewed.
You can assign the PR to them by writing `/assign @zroubalik` in a comment when ready.

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=knative-sandbox%2Fkn-plugin-func).

<details open>
Needs approval from an approver in each of these files:

- **[OWNERS](https://github.com/knative-sandbox/kn-plugin-func/blob/main/OWNERS)**

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":["zroubalik"]} -->
codecov[bot]: # [Codecov](https://codecov.io/gh/knative-sandbox/kn-plugin-func/pull/647?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative-sandbox) Report
> Merging [#647](https://codecov.io/gh/knative-sandbox/kn-plugin-func/pull/647?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative-sandbox) (6f8dde0) into [main](https://codecov.io/gh/knative-sandbox/kn-plugin-func/commit/1bbef7a0e660b2ed0bddc535cd70c80bc487082d?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative-sandbox) (1bbef7a) will **decrease** coverage by `0.12%`.
> The diff coverage is `9.09%`.

[![Impacted file tree graph](https://codecov.io/gh/knative-sandbox/kn-plugin-func/pull/647/graphs/tree.svg?width=650&height=150&src=pr&token=da1B9bMsQ9&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative-sandbox)](https://codecov.io/gh/knative-sandbox/kn-plugin-func/pull/647?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative-sandbox)

```diff
@@            Coverage Diff             @@
##             main     #647      +/-   ##
==========================================
- Coverage   36.26%   36.13%   -0.13%     
==========================================
  Files          38       38              
  Lines        3847     3855       +8     
==========================================
- Hits         1395     1393       -2     
- Misses       2268     2277       +9     
- Partials      184      185       +1     
```


| [Impacted Files](https://codecov.io/gh/knative-sandbox/kn-plugin-func/pull/647?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative-sandbox) | Coverage Δ | |
|---|---|---|
| [client.go](https://codecov.io/gh/knative-sandbox/kn-plugin-func/pull/647/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative-sandbox#diff-Y2xpZW50Lmdv) | `60.00% <ø> (-0.37%)` | :arrow_down: |
| [cmd/create.go](https://codecov.io/gh/knative-sandbox/kn-plugin-func/pull/647/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative-sandbox#diff-Y21kL2NyZWF0ZS5nbw==) | `35.56% <9.09%> (-1.56%)` | :arrow_down: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/knative-sandbox/kn-plugin-func/pull/647?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative-sandbox).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative-sandbox)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/knative-sandbox/kn-plugin-func/pull/647?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative-sandbox). Last update [1bbef7a...6f8dde0](https://codecov.io/gh/knative-sandbox/kn-plugin-func/pull/647?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative-sandbox). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative-sandbox).

Shashankft9: cc @lkingland 
Shashankft9: its ready for reviews now - thanks
knative-prow-robot: @Shashankft9: PR needs rebase.

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
</details>
knative-prow-robot: [APPROVALNOTIFIER] This PR is **NOT APPROVED**

This pull-request has been approved by: *<a href="https://github.com/knative-sandbox/kn-plugin-func/pull/647#" title="Author self-approved">Shashankft9</a>*
To complete the [pull request process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process), please ask for approval from **lkingland** after the PR has been reviewed.

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=knative-sandbox%2Fkn-plugin-func).

<details open>
Needs approval from an approver in each of these files:

- **[OWNERS](https://github.com/knative-sandbox/kn-plugin-func/blob/main/OWNERS)**

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":["lkingland"]} -->
knative-prow-robot: @Shashankft9: PR needs rebase.

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
</details>
lkingland: > I have two concerns here. I think we should leave the defaults in place at the library level. As a user of the client library, defaulting as much as possible is preferable as it eases basic usage, tests etc. and the default is clearly visible in code/docs. I would therefore consider leaving in the defaults, and tests, in the core client library and instead focus this change in the CLI.
> 
> Since this is a CLI, user-facing request, based on UX feedback, I would recommend we keep this as a CLI change: simply remove the default from the flag. If in "confirm" mode they get a prompt, and if in non-confirm-mode they get an error stating language is required.
> 
> Secondly, I am a bit hesitant to create "required flags", because they are by definition optional. If it's required, it should probably be an argument.

Just to follow up on this a bit.  Since we agree the config option is a good final destination, I am completely good with this being our path forward right now, if we can simply error on a missing required flag by default, but add it to the prompts when in "confirm" mode like the other commands.
knative-prow-robot: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/knative-sandbox/kn-plugin-func/pull/647#pullrequestreview-814355295" title="LGTM">lkingland</a>*, *<a href="https://github.com/knative-sandbox/kn-plugin-func/pull/647#" title="Author self-approved">Shashankft9</a>*

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=knative-sandbox%2Fkn-plugin-func).

The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)

<details >
Needs approval from an approver in each of these files:

- ~~[OWNERS](https://github.com/knative-sandbox/kn-plugin-func/blob/main/OWNERS)~~ [lkingland]

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":[]} -->
knative-prow-robot: New changes are detected. LGTM label has been removed.
lkingland: /lgtm

Patch:
diff --git a/cmd/create.go b/cmd/create.go
index 9e6ea70b..a667e334 100644
--- a/cmd/create.go
+++ b/cmd/create.go
@@ -16,6 +16,15 @@ import (
 	"knative.dev/kn-plugin-func/utils"
 )
 
+// ErrNoRuntime indicates that the language runtime flag was not passed.
+type ErrNoRuntime error
+
+// ErrInvalidRuntime indicates that the passed language runtime was invalid.
+type ErrInvalidRuntime error
+
+// ErrInvalidTemplate indicates that the passed template was invalid.
+type ErrInvalidTemplate error
+
 func init() {
 	// Add to the root a new "Create" command which obtains an appropriate
 	// instance of fn.Client from the given client creator function.
@@ -86,7 +95,7 @@ EXAMPLES
 	}
 
 	// Flags
-	cmd.Flags().StringP("language", "l", fn.DefaultRuntime, "Language Runtime (see help text for list) (Env: $FUNC_LANGUAGE)")
+	cmd.Flags().StringP("language", "l", "", "Language Runtime (see help text for list) (Env: $FUNC_LANGUAGE)")
 	cmd.Flags().StringP("template", "t", fn.DefaultTemplate, "Function template. (see help text for list) (Env: $FUNC_TEMPLATE)")
 	cmd.Flags().StringP("repository", "r", "", "URI to a Git repository containing the specified template (Env: $FUNC_REPOSITORY)")
 	cmd.Flags().BoolP("confirm", "c", false, "Prompt to confirm all options interactively (Env: $FUNC_CONFIRM)")
@@ -315,6 +324,22 @@ func isValidTemplate(client *fn.Client, runtime, template string) bool {
 	return false
 }
 
+// noRuntimeError creates an error stating that the language flag
+// is required, and a verbose list of valid options.
+func noRuntimeError(client *fn.Client) error {
+	b := strings.Builder{}
+	fmt.Fprintf(&b, "Required flag \"language\" not set.\n")
+	fmt.Fprintln(&b, "Available language runtimes are:")
+	runtimes, err := client.Runtimes()
+	if err != nil {
+		return err
+	}
+	for _, v := range runtimes {
+		fmt.Fprintf(&b, "  %v\n", v)
+	}
+	return ErrNoRuntime(errors.New(b.String()))
+}
+
 // newInvalidRuntimeError creates an error stating that the given language
 // is not valid, and a verbose list of valid options.
 func newInvalidRuntimeError(client *fn.Client, runtime string) error {
@@ -328,7 +353,7 @@ func newInvalidRuntimeError(client *fn.Client, runtime string) error {
 	for _, v := range runtimes {
 		fmt.Fprintf(&b, "  %v\n", v)
 	}
-	return errors.New(b.String())
+	return ErrInvalidRuntime(errors.New(b.String()))
 }
 
 // newInvalidTemplateError creates an error stating that the given template
@@ -345,7 +370,7 @@ func newInvalidTemplateError(client *fn.Client, runtime, template string) error
 	for _, v := range templates {
 		fmt.Fprintf(&b, "  %v\n", v)
 	}
-	return errors.New(b.String())
+	return ErrInvalidTemplate(errors.New(b.String()))
 }
 
 // Validate the current state of the config, returning any errors.
@@ -355,6 +380,7 @@ func newInvalidTemplateError(client *fn.Client, runtime, template string) error
 // pre-client validation should not be required, as the Client does its own
 // validation.
 func (c createConfig) Validate(client *fn.Client) (err error) {
+
 	// Confirm Name is valid
 	// Note that this is highly constricted, as it must currently adhere to the
 	// naming of a Knative Service, which itself is constrained to a Kubernetes
@@ -371,13 +397,16 @@ func (c createConfig) Validate(client *fn.Client) (err error) {
 	// Perhaps additional validation would be of use here in the CLI, but
 	// the client libray itself is ultimately responsible for validating all input
 	// prior to exeuting any requests.
-	// Client validates both language runtime and template exist, defaulting
-	// them if not (to 'node' and 'http', respectively).  However, if either of
-	// them are invalid, or the chosen combination does not exist, the error
-	// message is a rather terse one-liner.  This is suitable for libraries, but
+	// Client validates both language runtime and template exist, with language runtime
+	// being a mandatory flag while defaulting template if not present to 'http'.
+	// However, if either of them are invalid, or the chosen combination does not exist,
+	// the error message is a rather terse one-liner. This is suitable for libraries, but
 	// for a CLI it behooves us to be more verbose, including valid options for
 	// each.  So here, we check that the values entered (if any) are both valid
 	// and valid together.
+	if c.Runtime == "" {
+		return noRuntimeError(client)
+	}
 	if c.Runtime != "" && c.Repository == "" &&
 		!isValidRuntime(client, c.Runtime) {
 		return newInvalidRuntimeError(client, c.Runtime)


Test patch:
diff --git a/cmd/create_test.go b/cmd/create_test.go
index feb96a08..1a7b25f6 100644
--- a/cmd/create_test.go
+++ b/cmd/create_test.go
@@ -2,6 +2,7 @@ package cmd
 
 import (
 	"errors"
+	"fmt"
 	"io/ioutil"
 	"os"
 	"path/filepath"
@@ -18,13 +19,68 @@ func TestCreate(t *testing.T) {
 
 	// command with a client factory which yields a fully default client.
 	cmd := NewCreateCmd(func(createConfig) *fn.Client { return fn.New() })
-	cmd.SetArgs([]string{})
+	cmd.SetArgs([]string{
+		fmt.Sprintf("--language=%s", "go"),
+	})
 
 	if err := cmd.Execute(); err != nil {
 		t.Fatal(err)
 	}
 }
 
+// TestCreateWithNoRuntime ensures that an invocation of create must be
+// done with a runtime.
+func TestCreateWithNoRuntime(t *testing.T) {
+	defer fromTempDir(t)()
+
+	// command with a client factory which yields a fully default client.
+	cmd := NewCreateCmd(func(createConfig) *fn.Client { return fn.New() })
+	cmd.SetArgs([]string{})
+
+	err := cmd.Execute()
+	var e ErrNoRuntime
+	if !errors.As(err, &e) {
+		t.Fatalf("Did not receive ErrNoRuntime. Got %v", err)
+	}
+}
+
+// TestCreateWithNoRuntime ensures that an invocation of create must be
+// done with one of the valid runtimes only.
+func TestCreateWithInvalidRuntime(t *testing.T) {
+	defer fromTempDir(t)()
+
+	// command with a client factory which yields a fully default client.
+	cmd := NewCreateCmd(func(createConfig) *fn.Client { return fn.New() })
+	cmd.SetArgs([]string{
+		fmt.Sprintf("--language=%s", "test"),
+	})
+
+	err := cmd.Execute()
+	var e ErrInvalidRuntime
+	if !errors.As(err, &e) {
+		t.Fatalf("Did not receive ErrInvalidRuntime. Got %v", err)
+	}
+}
+
+// TestCreateWithInvalidTemplate ensures that an invocation of create must be
+// done with one of the valid templates only.
+func TestCreateWithInvalidTemplate(t *testing.T) {
+	defer fromTempDir(t)()
+
+	// command with a client factory which yields a fully default client.
+	cmd := NewCreateCmd(func(createConfig) *fn.Client { return fn.New() })
+	cmd.SetArgs([]string{
+		fmt.Sprintf("--language=%s", "go"),
+		fmt.Sprintf("--template=%s", "events"),
+	})
+
+	err := cmd.Execute()
+	var e ErrInvalidTemplate
+	if !errors.As(err, &e) {
+		t.Fatalf("Did not receive ErrInvalidTemplate. Got %v", err)
+	}
+}
+
 // TestCreateValidatesName ensures that the create command only accepts
 // DNS-1123 labels for Function name.
 func TestCreateValidatesName(t *testing.T) {
@@ -71,7 +127,9 @@ func TestCreateRepositoriesPath(t *testing.T) {
 		}
 		return fn.New()
 	})
-
+	cmd.SetArgs([]string{
+		fmt.Sprintf("--language=%s", "go"),
+	})
 	// Invoke the command, which is an airball, but does invoke the client
 	// constructor, which which evaluates the aceptance condition of ensuring the
 	// default repositories path was updated based on XDG_CONFIG_HOME.
diff --git a/function_test.go b/function_test.go
index dc7bfa03..391889cd 100644
--- a/function_test.go
+++ b/function_test.go
@@ -9,7 +9,7 @@ import (
 	fn "knative.dev/kn-plugin-func"
 	. "knative.dev/kn-plugin-func/testing"
 )
-
+    
 // TestFunctionNameDefault ensures that a Function's name is defaulted to that
 // which can be derived from the last part of its path.
 func TestFunctionNameDefault(t *testing.T) {
@@ -21,4 +21,4 @@ func TestFunctionNameDefault(t *testing.T) {
 	}
 	// TODO
 	// Test that the name was defaulted
-}
+}
\ No newline at end of file