fix(serving): Fix logic 'service create --force' and some bogus tests (…

…#216)

* fix(servicing): Fix logic 'service create --force' and some bogus tests

Now a more meaningful error is returned when the user tries to create
a service that already exists. Until know the service is tried
to be created even if the client already knows that it exists.

Also some broken (broken in the sense that the test was nonsense) tests has been fixed.

* chore(serving): Use constant instead of string lateral in test

pkg/kn/commands/service/service_create.go

@@ -84,12 +84,17 @@ func NewServiceCreateCommand(p *commands.KnParams) *cobra.Command {
 				return err
 			}
 
-			serviceExists, err := serviceExists(client, service.Name, namespace)
+			serviceExists, err := serviceExists(client, name, namespace)
 			if err != nil {
 				return err
 			}
 
-			if editFlags.ForceCreate && serviceExists {
+			if serviceExists {
+				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)
+				}
 				err = replaceService(client, service, namespace, cmd.OutOrStdout())
 			} else {
 				err = createService(client, service, namespace, cmd.OutOrStdout())

pkg/kn/commands/service/service_create_test.go

@@ -38,14 +38,16 @@ import (
 
 func fakeServiceCreate(args []string, withExistingService bool, sync bool) (
 	action client_testing.Action,
-	created *v1alpha1.Service,
+	service *v1alpha1.Service,
 	output string,
 	err error) {
 	knParams := &commands.KnParams{}
+	nrGetCalled := 0
 	cmd, fakeServing, buf := commands.CreateTestKnCommand(NewServiceCommand(knParams), knParams)
 	fakeServing.AddReactor("get", "services",
 		func(a client_testing.Action) (bool, runtime.Object, error) {
-			if withExistingService {
+			nrGetCalled++
+			if withExistingService || (sync && nrGetCalled > 1) {
 				return true, &v1alpha1.Service{}, nil
 			}
 			return true, nil, api_errors.NewNotFound(schema.GroupResource{}, "")
@@ -57,11 +59,24 @@ func fakeServiceCreate(args []string, withExistingService bool, sync bool) (
 			if !ok {
 				return true, nil, fmt.Errorf("wrong kind of action %v", a)
 			}
-			created, ok = createAction.GetObject().(*v1alpha1.Service)
+			service, ok = createAction.GetObject().(*v1alpha1.Service)
 			if !ok {
 				return true, nil, errors.New("was passed the wrong object")
 			}
-			return true, created, nil
+			return true, service, nil
+		})
+	fakeServing.AddReactor("update", "services",
+		func(a client_testing.Action) (bool, runtime.Object, error) {
+			updateAction, ok := a.(client_testing.UpdateAction)
+			action = updateAction
+			if !ok {
+				return true, nil, fmt.Errorf("wrong kind of action %v", a)
+			}
+			service, ok = updateAction.GetObject().(*v1alpha1.Service)
+			if !ok {
+				return true, nil, errors.New("was passed the wrong object")
+			}
+			return true, service, nil
 		})
 	if sync {
 		fakeServing.AddWatchReactor("services",
@@ -83,6 +98,7 @@ func fakeServiceCreate(args []string, withExistingService bool, sync bool) (
 	cmd.SetArgs(args)
 	err = cmd.Execute()
 	if err != nil {
+		output = err.Error()
 		return
 	}
 	output = buf.String()
@@ -118,7 +134,7 @@ func TestServiceCreateImage(t *testing.T) {
 
 func TestServiceCreateImageSync(t *testing.T) {
 	action, created, output, err := fakeServiceCreate([]string{
-		"service", "create", "foo", "--image", "gcr.io/foo/bar:baz"}, true, true)
+		"service", "create", "foo", "--image", "gcr.io/foo/bar:baz"}, false, true)
 	if err != nil {
 		t.Fatal(err)
 	} else if !action.Matches("create", "services") {
@@ -382,17 +398,27 @@ func parseQuantity(t *testing.T, quantityString string) resource.Quantity {
 	return quantity
 }
 
-func TestServiceCreateImageForce(t *testing.T) {
-	_, _, _, err := fakeServiceCreate([]string{
-		"service", "create", "foo", "--image", "gcr.io/foo/bar:v1", "--async"}, false, false)
-	if err != nil {
+func TestServiceCreateImageExistsAndNoForce(t *testing.T) {
+	_, _, output, err := fakeServiceCreate([]string{
+		"service", "create", "foo", "--image", "gcr.io/foo/bar:v2", "--async"}, true, false)
+	if err == nil {
 		t.Fatal(err)
 	}
+	if !strings.Contains(output, "foo") ||
+		!strings.Contains(output, commands.FakeNamespace) ||
+		!strings.Contains(output, "create") ||
+		!strings.Contains(output, "--force") {
+		t.Errorf("Invalid error output: '%s'", output)
+	}
+
+}
+
+func TestServiceCreateImageForce(t *testing.T) {
 	action, created, output, err := fakeServiceCreate([]string{
-		"service", "create", "foo", "--force", "--image", "gcr.io/foo/bar:v2", "--async"}, false, false)
+		"service", "create", "foo", "--force", "--image", "gcr.io/foo/bar:v2", "--async"}, true, false)
 	if err != nil {
 		t.Fatal(err)
-	} else if !action.Matches("create", "services") {
+	} else if !action.Matches("update", "services") {
 		t.Fatalf("Bad action %v", action)
 	}
 	template, err := servinglib.GetRevisionTemplate(created)