From d51a3a8d9126c251906affc56c9499051e69bc72 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Roland=20Hu=C3=9F?= Date: Tue, 2 Jul 2019 21:19:32 +0200 Subject: [PATCH] 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 | 9 +++- .../commands/service/service_create_test.go | 48 ++++++++++++++----- 2 files changed, 44 insertions(+), 13 deletions(-) diff --git a/pkg/kn/commands/service/service_create.go b/pkg/kn/commands/service/service_create.go index 1c5685248f..ba0f885031 100644 --- a/pkg/kn/commands/service/service_create.go +++ b/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()) diff --git a/pkg/kn/commands/service/service_create_test.go b/pkg/kn/commands/service/service_create_test.go index 898feb125b..f2bed74bcc 100644 --- a/pkg/kn/commands/service/service_create_test.go +++ b/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)