change untagRevision to return bool and add traffic e2e tests

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/traffic/compute.go

@@ -83,15 +83,15 @@ func (e ServiceTraffic) isTagPresent(tag string) bool {
 	return false
 }
 
-func (e ServiceTraffic) untagRevision(tag string, serviceName string) string {
+func (e ServiceTraffic) untagRevision(tag string, serviceName string) bool {
 	for i, target := range e {
 		if target.Tag == tag {
 			e[i].Tag = ""
-			return ""
+			return true
 		}
 	}
 
-	return tag
+	return false
 }
 
 func (e ServiceTraffic) isRevisionPresent(revision string) bool {
@@ -281,9 +281,9 @@ func Compute(cmd *cobra.Command, targets []servingv1.TrafficTarget,
 	// First precedence: Untag revisions
 	var errTagNames []string
 	for _, tag := range trafficFlags.UntagRevisions {
-		tagName := traffic.untagRevision(tag, serviceName)
-		if tagName != "" {
-			errTagNames = append(errTagNames, tagName)
+		tagExists := traffic.untagRevision(tag, serviceName)
+		if !tagExists {
+			errTagNames = append(errTagNames, tag)
 		}
 	}
 

test/e2e/traffic_split_test.go

@@ -405,6 +405,59 @@ 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)
+			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("latest", rev1, 0, false),
+			}
+
+			verifyTargets(r, serviceName, expectedTargets)
+			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("latest", rev1, 0, false),
+			}
+
 			verifyTargets(r, serviceName, expectedTargets)
 			test.ServiceDelete(r, serviceName)
 		},