From 3f146b14db4e027cc49bade56f1f6ef43f395c2a Mon Sep 17 00:00:00 2001 From: Daniel Helfand Date: Thu, 11 Jun 2020 02:22:23 -0400 Subject: [PATCH] Return Error Message from Using --untag with Nonexistent Tag (#880) * return error message from using --untag with nonexistent tag * improve err message for --untag and add e2e test * change untagRevision to return bool and add traffic e2e tests * update changelog --- CHANGELOG.adoc | 9 +++ lib/test/service.go | 9 +++ pkg/kn/commands/service/update.go | 2 +- pkg/kn/commands/service/update_test.go | 9 +++ pkg/kn/traffic/compute.go | 20 ++++-- pkg/kn/traffic/compute_test.go | 16 ++++- test/e2e/service_test.go | 13 ++++ test/e2e/traffic_split_test.go | 87 +++++++++++++++++++++----- 8 files changed, 143 insertions(+), 22 deletions(-) diff --git a/CHANGELOG.adoc b/CHANGELOG.adoc index d726ecd8a0..2565c816fb 100644 --- a/CHANGELOG.adoc +++ b/CHANGELOG.adoc @@ -12,6 +12,15 @@ | https://github.com/knative/client/pull/[#] //// +[cols="1,10,3", options="header", width="100%"] +|=== +| | Description | PR + +| 🐛 +| Return Error Message from Using --untag with Nonexistent Tag +| https://github.com/knative/client/pull/880[#880] +|=== + ## v0.15.1 (2020-06-03) [cols="1,10,3", options="header", width="100%"] diff --git a/lib/test/service.go b/lib/test/service.go index 508e106e83..69f8613b68 100644 --- a/lib/test/service.go +++ b/lib/test/service.go @@ -72,6 +72,15 @@ func ServiceUpdate(r *KnRunResultCollector, serviceName string, args ...string) assert.Check(r.T(), util.ContainsAllIgnoreCase(out.Stdout, "updating", "service", serviceName, "ready")) } +// ServiceUpdateWithError verifies service update operation with given arguments in sync mode +// when expecting an error +func ServiceUpdateWithError(r *KnRunResultCollector, serviceName string, args ...string) { + fullArgs := append([]string{}, "service", "update", serviceName) + fullArgs = append(fullArgs, args...) + out := r.KnTest().Kn().Run(fullArgs...) + r.AssertError(out) +} + // ServiceDelete verifies service deletion in sync mode func ServiceDelete(r *KnRunResultCollector, serviceName string) { out := r.KnTest().Kn().Run("service", "delete", "--wait", serviceName) diff --git a/pkg/kn/commands/service/update.go b/pkg/kn/commands/service/update.go index 3499472b3a..c8b64410f8 100644 --- a/pkg/kn/commands/service/update.go +++ b/pkg/kn/commands/service/update.go @@ -92,7 +92,7 @@ func NewServiceUpdateCommand(p *commands.KnParams) *cobra.Command { } if trafficFlags.Changed(cmd) { - traffic, err := traffic.Compute(cmd, service.Spec.Traffic, &trafficFlags) + traffic, err := traffic.Compute(cmd, service.Spec.Traffic, &trafficFlags, service.Name) if err != nil { return nil, err } diff --git a/pkg/kn/commands/service/update_test.go b/pkg/kn/commands/service/update_test.go index 0549bbf897..6b7ee0271c 100644 --- a/pkg/kn/commands/service/update_test.go +++ b/pkg/kn/commands/service/update_test.go @@ -737,6 +737,15 @@ func TestServiceUpdateDeletionTimestampNotNil(t *testing.T) { assert.ErrorContains(t, err, "service") } +func TestServiceUpdateTagDoesNotExist(t *testing.T) { + orig := newEmptyService() + + _, _, _, err := fakeServiceUpdate(orig, []string{ + "service", "update", "foo", "--untag", "foo", "--no-wait"}) + + assert.Assert(t, util.ContainsAll(err.Error(), "tag(s)", "foo", "not present", "service", "foo")) +} + func newEmptyService() *servingv1.Service { ret := &servingv1.Service{ TypeMeta: metav1.TypeMeta{ diff --git a/pkg/kn/traffic/compute.go b/pkg/kn/traffic/compute.go index ab0d6bb654..ef027bc22a 100644 --- a/pkg/kn/traffic/compute.go +++ b/pkg/kn/traffic/compute.go @@ -83,13 +83,15 @@ func (e ServiceTraffic) isTagPresent(tag string) bool { return false } -func (e ServiceTraffic) untagRevision(tag string) { +func (e ServiceTraffic) untagRevision(tag string, serviceName string) bool { for i, target := range e { if target.Tag == tag { e[i].Tag = "" - break + return true } } + + return false } func (e ServiceTraffic) isRevisionPresent(revision string) bool { @@ -267,7 +269,8 @@ func verifyInputSanity(trafficFlags *flags.Traffic) error { } // Compute takes service traffic targets and updates per given traffic flags -func Compute(cmd *cobra.Command, targets []servingv1.TrafficTarget, trafficFlags *flags.Traffic) ([]servingv1.TrafficTarget, error) { +func Compute(cmd *cobra.Command, targets []servingv1.TrafficTarget, + trafficFlags *flags.Traffic, serviceName string) ([]servingv1.TrafficTarget, error) { err := verifyInputSanity(trafficFlags) if err != nil { return nil, err @@ -276,8 +279,17 @@ func Compute(cmd *cobra.Command, targets []servingv1.TrafficTarget, trafficFlags traffic := newServiceTraffic(targets) // First precedence: Untag revisions + var errTagNames []string for _, tag := range trafficFlags.UntagRevisions { - traffic.untagRevision(tag) + tagExists := traffic.untagRevision(tag, serviceName) + if !tagExists { + errTagNames = append(errTagNames, tag) + } + } + + // Return all errors from untagging revisions + if len(errTagNames) > 0 { + return nil, fmt.Errorf("tag(s) %s not present for any revisions of service %s", strings.Join(errTagNames, ", "), serviceName) } for _, each := range trafficFlags.RevisionsTags { diff --git a/pkg/kn/traffic/compute_test.go b/pkg/kn/traffic/compute_test.go index 470e5a590f..71262b300c 100644 --- a/pkg/kn/traffic/compute_test.go +++ b/pkg/kn/traffic/compute_test.go @@ -193,7 +193,7 @@ func TestCompute(t *testing.T) { testCmd, tFlags := newTestTrafficCommand() testCmd.SetArgs(testCase.inputFlags) testCmd.Execute() - targets, err := Compute(testCmd, testCase.existingTraffic, tFlags) + targets, err := Compute(testCmd, testCase.existingTraffic, tFlags, "serviceName") if err != nil { t.Fatal(err) } @@ -278,12 +278,24 @@ func TestComputeErrMsg(t *testing.T) { []string{"--traffic", "echo-v1=40", "--traffic", "echo-v1=60"}, "repetition of revision reference echo-v1 is not allowed, use only once with --traffic flag", }, + { + "untag single tag that does not exist", + append(newServiceTraffic([]servingv1.TrafficTarget{}), newTarget("latest", "echo-v1", 100, false)), + []string{"--untag", "foo"}, + "tag(s) foo not present for any revisions of service serviceName", + }, + { + "untag multiple tags that do not exist", + append(newServiceTraffic([]servingv1.TrafficTarget{}), newTarget("latest", "echo-v1", 100, false)), + []string{"--untag", "foo", "--untag", "bar"}, + "tag(s) foo, bar not present for any revisions of service serviceName", + }, } { t.Run(testCase.name, func(t *testing.T) { testCmd, tFlags := newTestTrafficCommand() testCmd.SetArgs(testCase.inputFlags) testCmd.Execute() - _, err := Compute(testCmd, testCase.existingTraffic, tFlags) + _, err := Compute(testCmd, testCase.existingTraffic, tFlags, "serviceName") assert.Error(t, err, testCase.errMsg) }) } diff --git a/test/e2e/service_test.go b/test/e2e/service_test.go index de49e6455a..900c5d3aeb 100644 --- a/test/e2e/service_test.go +++ b/test/e2e/service_test.go @@ -59,6 +59,10 @@ func TestService(t *testing.T) { t.Log("create service private and make public") serviceCreatePrivateUpdatePublic(r, "hello-private-public") + t.Log("error message from --untag with tag that doesn't exist") + test.ServiceCreate(r, "untag") + serviceUntagTagThatDoesNotExist(r, "untag") + t.Log("delete all services in a namespace") test.ServiceCreate(r, "svc1") test.ServiceCreate(r, "service2") @@ -140,6 +144,15 @@ func serviceMultipleDelete(r *test.KnRunResultCollector, existService, nonexistS assert.Check(r.T(), strings.Contains(out.Stdout, expectedErr), "Failed to get 'not found' error") } +func serviceUntagTagThatDoesNotExist(r *test.KnRunResultCollector, serviceName string) { + out := r.KnTest().Kn().Run("service", "list", serviceName) + r.AssertNoError(out) + assert.Check(r.T(), strings.Contains(out.Stdout, serviceName), "Service "+serviceName+" does not exist for test (but should exist)") + + out = r.KnTest().Kn().Run("service", "update", serviceName, "--untag", "foo", "--no-wait") + assert.Check(r.T(), util.ContainsAll(out.Stderr, "tag(s)", "foo", "not present", "service", "untag"), "Expected error message for using --untag with nonexistent tag") +} + func serviceDeleteAll(r *test.KnRunResultCollector) { out := r.KnTest().Kn().Run("service", "list") r.AssertNoError(out) diff --git a/test/e2e/traffic_split_test.go b/test/e2e/traffic_split_test.go index 53bcc0ecc5..ac1ee49bc6 100644 --- a/test/e2e/traffic_split_test.go +++ b/test/e2e/traffic_split_test.go @@ -105,7 +105,7 @@ func TestTrafficSplit(t *testing.T) { // make ordered fields per tflags (tag, revision, percent, latest) expectedTargets := []TargetFields{newTargetFields("v1", rev1, 50, false), newTargetFields("v2", rev2, 50, false)} - verifyTargets(r, serviceName, expectedTargets) + verifyTargets(r, serviceName, expectedTargets, false) test.ServiceDelete(r, serviceName) }, ) @@ -127,7 +127,7 @@ func TestTrafficSplit(t *testing.T) { test.ServiceUpdate(r, serviceName, "--traffic", fmt.Sprintf("%s=20,%s=80", rev1, rev2)) expectedTargets := []TargetFields{newTargetFields("", rev1, 20, false), newTargetFields("", rev2, 80, false)} - verifyTargets(r, serviceName, expectedTargets) + verifyTargets(r, serviceName, expectedTargets, false) test.ServiceDelete(r, serviceName) }, ) @@ -148,7 +148,7 @@ func TestTrafficSplit(t *testing.T) { test.ServiceUpdate(r, serviceName, "--tag", fmt.Sprintf("%s=%s", rev1, "candidate")) expectedTargets := []TargetFields{newTargetFields("", rev2, 100, true), newTargetFields("candidate", rev1, 0, false)} - verifyTargets(r, serviceName, expectedTargets) + verifyTargets(r, serviceName, expectedTargets, false) test.ServiceDelete(r, serviceName) }, ) @@ -171,7 +171,7 @@ func TestTrafficSplit(t *testing.T) { "--traffic", "candidate=2%,@latest=98%") expectedTargets := []TargetFields{newTargetFields("", rev2, 98, true), newTargetFields("candidate", rev1, 2, false)} - verifyTargets(r, serviceName, expectedTargets) + verifyTargets(r, serviceName, expectedTargets, false) test.ServiceDelete(r, serviceName) }, ) @@ -204,7 +204,7 @@ func TestTrafficSplit(t *testing.T) { // there will be 2 targets in existing block 1. @latest, 2.for revision $rev2 // target for rev1 is removed as it had no traffic and we untagged it's tag current expectedTargets := []TargetFields{newTargetFields("", rev3, 100, true), newTargetFields("current", rev2, 0, false)} - verifyTargets(r, serviceName, expectedTargets) + verifyTargets(r, serviceName, expectedTargets, false) test.ServiceDelete(r, serviceName) }, ) @@ -225,7 +225,7 @@ func TestTrafficSplit(t *testing.T) { test.ServiceUpdate(r, serviceName, "--untag", "testing", "--tag", "@latest=staging") expectedTargets := []TargetFields{newTargetFields("staging", rev1, 100, true)} - verifyTargets(r, serviceName, expectedTargets) + verifyTargets(r, serviceName, expectedTargets, false) test.ServiceDelete(r, serviceName) }, ) @@ -250,7 +250,7 @@ func TestTrafficSplit(t *testing.T) { expectedTargets := []TargetFields{newTargetFields("", rev2, 100, true), newTargetFields("staging", rev1, 0, false)} - verifyTargets(r, serviceName, expectedTargets) + verifyTargets(r, serviceName, expectedTargets, false) test.ServiceDelete(r, serviceName) }, ) @@ -277,7 +277,7 @@ func TestTrafficSplit(t *testing.T) { test.ServiceUpdate(r, serviceName, "--untag", "old", "--traffic", "@latest=100") expectedTargets := []TargetFields{newTargetFields("", rev2, 100, true)} - verifyTargets(r, serviceName, expectedTargets) + verifyTargets(r, serviceName, expectedTargets, false) test.ServiceDelete(r, serviceName) }, ) @@ -300,7 +300,7 @@ func TestTrafficSplit(t *testing.T) { "--traffic", "stable=50%,current=50%") expectedTargets := []TargetFields{newTargetFields("stable", rev1, 50, false), newTargetFields("current", rev1, 50, false)} - verifyTargets(r, serviceName, expectedTargets) + verifyTargets(r, serviceName, expectedTargets, false) test.ServiceDelete(r, serviceName) }, ) @@ -324,7 +324,7 @@ func TestTrafficSplit(t *testing.T) { test.ServiceUpdate(r, serviceName, "--traffic", "@latest=100") expectedTargets := []TargetFields{newTargetFields("", rev2, 100, true)} - verifyTargets(r, serviceName, expectedTargets) + verifyTargets(r, serviceName, expectedTargets, false) test.ServiceDelete(r, serviceName) }, ) @@ -343,7 +343,7 @@ func TestTrafficSplit(t *testing.T) { test.ServiceUpdate(r, serviceName, "--tag", "@latest=current") expectedTargets := []TargetFields{newTargetFields("current", rev1, 100, true)} - verifyTargets(r, serviceName, expectedTargets) + verifyTargets(r, serviceName, expectedTargets, false) test.ServiceDelete(r, serviceName) }, ) @@ -373,7 +373,7 @@ func TestTrafficSplit(t *testing.T) { expectedTargets := []TargetFields{newTargetFields("current", rev2, 0, true), newTargetFields("testing", rev1, 100, false)} - verifyTargets(r, serviceName, expectedTargets) + verifyTargets(r, serviceName, expectedTargets, false) test.ServiceDelete(r, serviceName) }, ) @@ -405,17 +405,74 @@ func TestTrafficSplit(t *testing.T) { // In spec of traffic block (not status) either latestReadyRevision:true or revisionName can be given per target newTargetFields("latest", rev2, 0, false)} - verifyTargets(r, serviceName, expectedTargets) + verifyTargets(r, serviceName, expectedTargets, false) + test.ServiceDelete(r, serviceName) + }, + ) + t.Run("UntagNonExistentTag", + func(t *testing.T) { + t.Log("use --untag on a tag that does not exist") + r := test.NewKnRunResultCollector(t, it) + defer r.DumpIfFailed() + + serviceName := test.GetNextServiceName(serviceBase) + rev1 := fmt.Sprintf("%s-rev-1", serviceName) + serviceCreateWithOptions(r, serviceName, "--revision-name", rev1) + + // existing state: a revision exist with latest tag + test.ServiceUpdate(r, serviceName, "--tag", fmt.Sprintf("%s=latest", rev1)) + + // desired state of revision tags: rev1=latest + // attempt to untag a tag that does not exist for the service's revisions + test.ServiceUpdateWithError(r, serviceName, "--untag", "foo") + + // state should remain the same as error from --untag will stop service update + expectedTargets := []TargetFields{ + newTargetFields("", rev1, 100, true), + newTargetFields("latest", rev1, 0, false), + } + + verifyTargets(r, serviceName, expectedTargets, true) + test.ServiceDelete(r, serviceName) + }, + ) + t.Run("UntagNonExistentTagAndValidTag", + func(t *testing.T) { + t.Log("use --untag on a tag that does not exist in addition to a tag that exists") + r := test.NewKnRunResultCollector(t, it) + defer r.DumpIfFailed() + + serviceName := test.GetNextServiceName(serviceBase) + rev1 := fmt.Sprintf("%s-rev-1", serviceName) + serviceCreateWithOptions(r, serviceName, "--revision-name", rev1) + + // existing state: a revision exist with latest tag + test.ServiceUpdate(r, serviceName, "--tag", fmt.Sprintf("%s=latest", rev1)) + + // desired state of revision tags: rev1=latest + // attempt to untag a tag that does not exist for the service's revisions (foo) + // and also untag a tag that exists (latest) + test.ServiceUpdateWithError(r, serviceName, "--untag", "latest", "--untag", "foo") + + // state should remain the same as error from --untag will stop service update + expectedTargets := []TargetFields{ + newTargetFields("", rev1, 100, true), + newTargetFields("latest", rev1, 0, false), + } + + verifyTargets(r, serviceName, expectedTargets, true) test.ServiceDelete(r, serviceName) }, ) } -func verifyTargets(r *test.KnRunResultCollector, serviceName string, expectedTargets []TargetFields) { +func verifyTargets(r *test.KnRunResultCollector, serviceName string, expectedTargets []TargetFields, expectErr bool) { out := test.ServiceDescribeWithJSONPath(r, serviceName, targetsJsonPath) assert.Check(r.T(), out != "") actualTargets, err := splitTargets(out, targetsSeparator, len(expectedTargets)) - assert.NilError(r.T(), err) + if !expectErr { + assert.NilError(r.T(), err) + } formattedActualTargets := formatActualTargets(r.T(), r.KnTest(), actualTargets) assert.DeepEqual(r.T(), expectedTargets, formattedActualTargets) if r.T().Failed() {