feat: Add --filename flag to service create command (#913)

* feat: Add --file flag to service create cmd

* fix: Fix linter warnings

* fix: Remove unecessary changes

* fix: Reflect review feedback

* fix: Change flag --file to -f,--filename

* fix: Reflect review feedback

* fix: Move test cleanup to correct place

* fix: Refactor name param restrictions

* fix: Fix flag name string in func

docs/cmd/kn_service_create.md

@@ -64,6 +64,7 @@ kn service create NAME --image IMAGE
       --concurrency-utilization int   Percentage of concurrent requests utilization before scaling up. (default 70)
   -e, --env stringArray               Environment variable to set. NAME=value; you may provide this flag any number of times to set multiple environment variables. To unset, specify the environment variable name followed by a "-" (e.g., NAME-).
       --env-from stringArray          Add environment variables from a ConfigMap (prefix cm: or config-map:) or a Secret (prefix secret:). Example: --env-from cm:myconfigmap or --env-from secret:mysecret. You can use this flag multiple times. To unset a ConfigMap/Secret reference, append "-" to the name, e.g. --env-from cm:myconfigmap-.
+  -f, --filename string               Create a service from file.
       --force                         Create service forcefully, replaces existing service if any.
   -h, --help                          help for create
       --image string                  Image to run.

pkg/kn/commands/service/configuration_edit_flags.go

@@ -68,6 +68,8 @@ type ConfigurationEditFlags struct {
 	GenerateRevisionName bool
 	ForceCreate          bool
 
+	Filename string
+
 	// Bookkeeping
 	flags []string
 }
@@ -272,7 +274,8 @@ func (p *ConfigurationEditFlags) AddCreateFlags(command *cobra.Command) {
 	p.addSharedFlags(command)
 	command.Flags().BoolVar(&p.ForceCreate, "force", false,
 		"Create service forcefully, replaces existing service if any.")
-	command.MarkFlagRequired("image")
+	command.Flags().StringVarP(&p.Filename, "filename", "f", "", "Create a service from file.")
+	command.MarkFlagFilename("filename")
 }
 
 // Apply mutates the given service according to the flags in the command.

pkg/kn/commands/service/create.go

@@ -18,6 +18,7 @@ import (
 	"errors"
 	"fmt"
 	"io"
+	"os"
 
 	"knative.dev/client/pkg/kn/commands"
 	servinglib "knative.dev/client/pkg/serving"
@@ -28,6 +29,8 @@ import (
 	corev1 "k8s.io/api/core/v1"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/util/yaml"
+
 	servingv1 "knative.dev/serving/pkg/apis/serving/v1"
 
 	clientservingv1 "knative.dev/client/pkg/serving/v1"
@@ -78,11 +81,14 @@ func NewServiceCreateCommand(p *commands.KnParams) *cobra.Command {
 		Short:   "Create a service",
 		Example: create_example,
 		RunE: func(cmd *cobra.Command, args []string) (err error) {
-			if len(args) != 1 {
+			if len(args) != 1 && editFlags.Filename == "" {
 				return errors.New("'service create' requires the service name given as single argument")
 			}
-			name := args[0]
-			if editFlags.Image == "" {
+			name := ""
+			if len(args) == 1 {
+				name = args[0]
+			}
+			if editFlags.Image == "" && editFlags.Filename == "" {
 				return errors.New("'service create' requires the image name to run provided with the --image option")
 			}
 
@@ -91,7 +97,12 @@ func NewServiceCreateCommand(p *commands.KnParams) *cobra.Command {
 				return err
 			}
 
-			service, err := constructService(cmd, editFlags, name, namespace)
+			var service *servingv1.Service
+			if editFlags.Filename == "" {
+				service, err = constructService(cmd, editFlags, name, namespace)
+			} else {
+				service, err = constructServiceFromFile(cmd, editFlags, name, namespace)
+			}
 			if err != nil {
 				return err
 			}
@@ -100,8 +111,7 @@ func NewServiceCreateCommand(p *commands.KnParams) *cobra.Command {
 			if err != nil {
 				return err
 			}
-
-			serviceExists, err := serviceExists(client, name)
+			serviceExists, err := serviceExists(client, service.Name)
 			if err != nil {
 				return err
 			}
@@ -111,7 +121,7 @@ func NewServiceCreateCommand(p *commands.KnParams) *cobra.Command {
 				if !editFlags.ForceCreate {
 					return fmt.Errorf(
 						"cannot create service '%s' in namespace '%s' "+
-							"because the service already exists and no --force option was given", name, namespace)
+							"because the service already exists and no --force option was given", service.Name, namespace)
 				}
 				err = replaceService(client, service, waitFlags, out)
 			} else {
@@ -263,3 +273,42 @@ func constructService(cmd *cobra.Command, editFlags ConfigurationEditFlags, name
 	}
 	return &service, nil
 }
+
+// constructServiceFromFile creates struct from provided file
+func constructServiceFromFile(cmd *cobra.Command, editFlags ConfigurationEditFlags, name, namespace string) (*servingv1.Service, error) {
+	var service servingv1.Service
+	file, err := os.Open(editFlags.Filename)
+	if err != nil {
+		return nil, err
+	}
+	decoder := yaml.NewYAMLOrJSONDecoder(file, 512)
+
+	err = decoder.Decode(&service)
+	if err != nil {
+		return nil, err
+	}
+	if name == "" && service.Name != "" {
+		// keep provided service.Name if name param is empty
+	} else if name != "" && service.Name == "" {
+		service.Name = name
+	} else if name != "" && service.Name != "" {
+		// throw error if names differ, otherwise use already set value
+		if name != service.Name {
+			return nil, fmt.Errorf("provided service name '%s' doesn't match name from file '%s'", name, service.Name)
+		}
+	} else {
+		return nil, fmt.Errorf("no service name provided in command parameter or file")
+	}
+
+	// Set namespace in case it's specified as --namespace
+	service.ObjectMeta.Namespace = namespace
+
+	// We need to generate revision to have --force replace working
+	revName, err := servinglib.GenerateRevisionName(editFlags.RevisionName, &service)
+	if err != nil {
+		return nil, err
+	}
+	service.Spec.Template.Name = revName
+
+	return &service, nil
+}

pkg/kn/commands/service/create_test.go

@@ -17,6 +17,9 @@ package service
 import (
 	"errors"
 	"fmt"
+	"io/ioutil"
+	"os"
+	"path/filepath"
 	"reflect"
 	"strings"
 	"testing"
@@ -707,3 +710,196 @@ func TestServiceCreateWithClusterLocal(t *testing.T) {
 		t.Fatalf("Incorrect VisibilityClusterLocal value '%s'", labelValue)
 	}
 }
+
+var serviceYAML = `
+apiVersion: serving.knative.dev/v1
+kind: Service
+metadata:
+  name: foo
+spec:
+  template:
+    spec:
+      containers:
+        - image: gcr.io/foo/bar:baz
+          env:
+            - name: TARGET
+              value: "Go Sample v1"
+`
+
+var serviceJSON = `
+{
+  "apiVersion": "serving.knative.dev/v1",
+  "kind": "Service",
+  "metadata": {
+    "name": "foo"
+  },
+  "spec": {
+    "template": {
+		"spec": {
+		"containers": [
+			{
+			"image": "gcr.io/foo/bar:baz",
+			"env": [
+				{
+				"name": "TARGET",
+				"value": "Go Sample v1"
+				}
+			]
+		  }
+		]
+	  }
+	}
+  }
+}`
+
+func TestServiceCreateFromYAML(t *testing.T) {
+	tempDir, err := ioutil.TempDir("", "kn-file")
+	defer os.RemoveAll(tempDir)
+	assert.NilError(t, err)
+
+	tempFile := filepath.Join(tempDir, "service.yaml")
+	err = ioutil.WriteFile(tempFile, []byte(serviceYAML), os.FileMode(0666))
+	assert.NilError(t, err)
+
+	action, created, _, err := fakeServiceCreate([]string{
+		"service", "create", "foo", "--filename", tempFile}, false)
+	assert.NilError(t, err)
+	assert.Assert(t, action.Matches("create", "services"))
+
+	assert.Equal(t, created.Name, "foo")
+	assert.Equal(t, created.Spec.Template.Spec.GetContainer().Image, "gcr.io/foo/bar:baz")
+}
+
+func TestServiceCreateFromJSON(t *testing.T) {
+	tempDir, err := ioutil.TempDir("", "kn-file")
+	defer os.RemoveAll(tempDir)
+	assert.NilError(t, err)
+
+	tempFile := filepath.Join(tempDir, "service.json")
+	err = ioutil.WriteFile(tempFile, []byte(serviceJSON), os.FileMode(0666))
+	assert.NilError(t, err)
+
+	action, created, _, err := fakeServiceCreate([]string{
+		"service", "create", "foo", "--filename", tempFile}, false)
+	assert.NilError(t, err)
+	assert.Assert(t, action.Matches("create", "services"))
+
+	assert.Equal(t, created.Name, "foo")
+	assert.Equal(t, created.Spec.Template.Spec.GetContainer().Image, "gcr.io/foo/bar:baz")
+}
+
+func TestServiceCreateFromFileWithName(t *testing.T) {
+	tempDir, err := ioutil.TempDir("", "kn-file")
+	defer os.RemoveAll(tempDir)
+	assert.NilError(t, err)
+
+	tempFile := filepath.Join(tempDir, "service.yaml")
+	err = ioutil.WriteFile(tempFile, []byte(serviceYAML), os.FileMode(0666))
+	assert.NilError(t, err)
+
+	t.Log("no NAME param provided")
+	action, created, _, err := fakeServiceCreate([]string{
+		"service", "create", "--filename", tempFile}, false)
+	assert.NilError(t, err)
+	assert.Assert(t, action.Matches("create", "services"))
+
+	assert.Equal(t, created.Name, "foo")
+	assert.Equal(t, created.Spec.Template.Spec.GetContainer().Image, "gcr.io/foo/bar:baz")
+
+	t.Log("no service.Name provided in file")
+	err = ioutil.WriteFile(tempFile, []byte(strings.ReplaceAll(serviceYAML, "name: foo", "")), os.FileMode(0666))
+	assert.NilError(t, err)
+	action, created, _, err = fakeServiceCreate([]string{
+		"service", "create", "cli-foo", "--filename", tempFile}, false)
+	assert.NilError(t, err)
+	assert.Assert(t, action.Matches("create", "services"))
+
+	assert.Equal(t, created.Name, "cli-foo")
+	assert.Equal(t, created.Spec.Template.Spec.GetContainer().Image, "gcr.io/foo/bar:baz")
+}
+
+func TestServiceCreateFileNameMismatch(t *testing.T) {
+	tempDir, err := ioutil.TempDir("", "kn-file")
+	assert.NilError(t, err)
+
+	tempFile := filepath.Join(tempDir, "service.json")
+	err = ioutil.WriteFile(tempFile, []byte(serviceJSON), os.FileMode(0666))
+	assert.NilError(t, err)
+
+	t.Log("NAME param nad service.Name differ")
+	_, _, _, err = fakeServiceCreate([]string{
+		"service", "create", "anotherFoo", "--filename", tempFile}, false)
+	assert.Assert(t, err != nil)
+	assert.Assert(t, util.ContainsAllIgnoreCase(err.Error(), "provided", "'anotherFoo'", "name", "match", "from", "file", "'foo'"))
+
+	t.Log("no NAME param & no service.Name provided in file")
+	err = ioutil.WriteFile(tempFile, []byte(strings.ReplaceAll(serviceYAML, "name: foo", "")), os.FileMode(0666))
+	assert.NilError(t, err)
+	_, _, _, err = fakeServiceCreate([]string{
+		"service", "create", "--filename", tempFile}, false)
+	assert.Assert(t, err != nil)
+	assert.Assert(t, util.ContainsAllIgnoreCase(err.Error(), "no", "service", "name", "provided", "parameter", "file"))
+}
+
+func TestServiceCreateFileError(t *testing.T) {
+	_, _, _, err := fakeServiceCreate([]string{
+		"service", "create", "foo", "--filename", "filepath"}, false)
+
+	assert.Assert(t, util.ContainsAll(err.Error(), "no", "such", "file", "directory", "filepath"))
+}
+
+func TestServiceCreateInvalidDataJSON(t *testing.T) {
+	tempDir, err := ioutil.TempDir("", "kn-file")
+	defer os.RemoveAll(tempDir)
+	assert.NilError(t, err)
+	tempFile := filepath.Join(tempDir, "invalid.json")
+
+	// Double curly bracket at the beginning of file
+	invalidData := strings.Replace(serviceJSON, "{\n", "{{\n", 1)
+	err = ioutil.WriteFile(tempFile, []byte(invalidData), os.FileMode(0666))
+	assert.NilError(t, err)
+	_, _, _, err = fakeServiceCreate([]string{"service", "create", "foo", "--filename", tempFile}, false)
+	assert.Assert(t, util.ContainsAll(err.Error(), "invalid", "character", "'{'", "beginning"))
+
+	// Remove closing quote on key
+	invalidData = strings.Replace(serviceJSON, "metadata\"", "metadata", 1)
+	err = ioutil.WriteFile(tempFile, []byte(invalidData), os.FileMode(0666))
+	assert.NilError(t, err)
+	_, _, _, err = fakeServiceCreate([]string{"service", "create", "foo", "--filename", tempFile}, false)
+	assert.Assert(t, util.ContainsAll(err.Error(), "invalid", "character", "'\\n'", "string", "literal"))
+
+	// Remove opening square bracket
+	invalidData = strings.Replace(serviceJSON, " [", "", 1)
+	err = ioutil.WriteFile(tempFile, []byte(invalidData), os.FileMode(0666))
+	assert.NilError(t, err)
+	_, _, _, err = fakeServiceCreate([]string{"service", "create", "foo", "--filename", tempFile}, false)
+	assert.Assert(t, util.ContainsAll(err.Error(), "invalid", "character", "']'", "after", "key:value"))
+}
+
+func TestServiceCreateInvalidDataYAML(t *testing.T) {
+	tempDir, err := ioutil.TempDir("", "kn-file")
+	defer os.RemoveAll(tempDir)
+	assert.NilError(t, err)
+	tempFile := filepath.Join(tempDir, "invalid.yaml")
+
+	// Remove dash
+	invalidData := strings.Replace(serviceYAML, "- image", "image", 1)
+	err = ioutil.WriteFile(tempFile, []byte(invalidData), os.FileMode(0666))
+	assert.NilError(t, err)
+	_, _, _, err = fakeServiceCreate([]string{"service", "create", "foo", "--filename", tempFile}, false)
+	assert.Assert(t, util.ContainsAll(err.Error(), "mapping", "values", "not", "allowed"))
+
+	// Remove name key
+	invalidData = strings.Replace(serviceYAML, "name:", "", 1)
+	err = ioutil.WriteFile(tempFile, []byte(invalidData), os.FileMode(0666))
+	assert.NilError(t, err)
+	_, _, _, err = fakeServiceCreate([]string{"service", "create", "foo", "--filename", tempFile}, false)
+	assert.Assert(t, util.ContainsAll(err.Error(), "cannot", "unmarshal", "Go", "struct", "Service.metadata"))
+
+	// Remove opening square bracket
+	invalidData = strings.Replace(serviceYAML, "env", "\tenv", 1)
+	err = ioutil.WriteFile(tempFile, []byte(invalidData), os.FileMode(0666))
+	assert.NilError(t, err)
+	_, _, _, err = fakeServiceCreate([]string{"service", "create", "foo", "--filename", tempFile}, false)
+	assert.Assert(t, util.ContainsAll(err.Error(), "found", "tab", "violates", "indentation"))
+}

test/e2e/service_file_test.go

@@ -0,0 +1,133 @@
+// Copyright 2020 The Knative Authors
+
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+
+//     http://www.apache.org/licenses/LICENSE-2.0
+
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+// +build e2e
+// +build !eventing
+
+package e2e
+
+import (
+	"fmt"
+	"io/ioutil"
+	"os"
+	"path/filepath"
+	"testing"
+
+	"gotest.tools/assert"
+
+	"knative.dev/client/lib/test"
+	"knative.dev/client/pkg/util"
+)
+
+const (
+	ServiceYAML string = `
+apiVersion: serving.knative.dev/v1
+kind: Service
+metadata:
+  name: foo-yaml
+spec:
+  template:
+    spec:
+      containers:
+        - image: %s
+          env:
+            - name: TARGET
+              value: "Go Sample v1"`
+
+	ServiceJSON string = `
+{
+  "apiVersion": "serving.knative.dev/v1",
+  "kind": "Service",
+  "metadata": {
+    "name": "foo-json"
+  },
+  "spec": {
+    "template": {
+		"spec": {
+		"containers": [
+			{
+			"image": "%s",
+			"env": [
+				{
+				"name": "TARGET",
+				"value": "Go Sample v1"
+				}
+			]
+		  }
+		]
+	  }
+	}
+  }
+}`
+)
+
+func TestServiceCreateFromFile(t *testing.T) {
+	t.Parallel()
+	it, err := test.NewKnTest()
+	assert.NilError(t, err)
+	defer func() {
+		assert.NilError(t, it.Teardown())
+	}()
+
+	r := test.NewKnRunResultCollector(t, it)
+	defer r.DumpIfFailed()
+
+	tempDir, err := ioutil.TempDir("", "kn-file")
+	defer os.RemoveAll(tempDir)
+	assert.NilError(t, err)
+
+	test.CreateFile("foo.json", fmt.Sprintf(ServiceJSON, test.KnDefaultTestImage), tempDir, test.FileModeReadWrite)
+	test.CreateFile("foo.yaml", fmt.Sprintf(ServiceYAML, test.KnDefaultTestImage), tempDir, test.FileModeReadWrite)
+
+	t.Log("create foo-json service from JSON file")
+	serviceCreateFromFile(r, "foo-json", filepath.Join(tempDir, "foo.json"), true)
+
+	t.Log("create foo-yaml service from YAML file")
+	serviceCreateFromFile(r, "foo-yaml", filepath.Join(tempDir, "foo.yaml"), false)
+
+	t.Log("error message for non-existing file")
+	serviceCreateFromFileError(r, "foo", filepath.Join(tempDir, "fake-foo.json"))
+	serviceCreateFromFileError(r, "foo", filepath.Join(tempDir, "fake-foo.yaml"))
+
+	t.Log("error message for mismatch names")
+	serviceCreateFromFileNameMismatch(r, "foo", filepath.Join(tempDir, "foo.json"))
+	serviceCreateFromFileNameMismatch(r, "foo", filepath.Join(tempDir, "foo.yaml"))
+}
+
+func serviceCreateFromFile(r *test.KnRunResultCollector, serviceName, filePath string, useName bool) {
+	var out test.KnRunResult
+	if useName {
+		out = r.KnTest().Kn().Run("service", "create", serviceName, "-f", filePath)
+	} else {
+		out = r.KnTest().Kn().Run("service", "create", "-f", filePath)
+	}
+	r.AssertNoError(out)
+	assert.Check(r.T(), util.ContainsAllIgnoreCase(out.Stdout, "service", serviceName, "creating", "namespace", r.KnTest().Kn().Namespace(), "ready"))
+
+	out = r.KnTest().Kn().Run("service", "describe", serviceName, "--verbose")
+	r.AssertNoError(out)
+	assert.Check(r.T(), util.ContainsAllIgnoreCase(out.Stdout, serviceName))
+}
+
+func serviceCreateFromFileError(r *test.KnRunResultCollector, serviceName, filePath string) {
+	out := r.KnTest().Kn().Run("service", "create", serviceName, "--filename", filePath)
+	r.AssertError(out)
+	assert.Check(r.T(), util.ContainsAllIgnoreCase(out.Stderr, "no", "such", "file", "directory", filePath))
+}
+
+func serviceCreateFromFileNameMismatch(r *test.KnRunResultCollector, serviceName, filePath string) {
+	out := r.KnTest().Kn().Run("service", "create", serviceName, "--filename", filePath)
+	r.AssertError(out)
+	assert.Check(r.T(), util.ContainsAllIgnoreCase(out.Stderr, "provided", "'"+serviceName+"'", "name", "match", "from", "file"))
+}