From 3ae19db2b91a8a5346012ba611201ee761435103 Mon Sep 17 00:00:00 2001
From: Daniel Helfand <helfand.4@gmail.com>
Date: Wed, 10 Jun 2020 11:01:45 -0500
Subject: [PATCH] change untagRevision to return bool and add traffic e2e tests

---
 pkg/kn/traffic/compute.go      | 12 ++++----
 test/e2e/traffic_split_test.go | 53 ++++++++++++++++++++++++++++++++++
 2 files changed, 59 insertions(+), 6 deletions(-)

diff --git a/pkg/kn/traffic/compute.go b/pkg/kn/traffic/compute.go
index b5d81a0242..ef027bc22a 100644
--- a/pkg/kn/traffic/compute.go
+++ b/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)
 		}
 	}
 
diff --git a/test/e2e/traffic_split_test.go b/test/e2e/traffic_split_test.go
index 53bcc0ecc5..8f782eae7c 100644
--- a/test/e2e/traffic_split_test.go
+++ b/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.ServiceUpdate(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.ServiceUpdate(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)
 		},