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

@@ -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%"]

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)

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
 					}

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{

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 {

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)
 		})
 	}

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)

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() {