improve err message for --untag and add e2e test

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

@@ -743,7 +743,7 @@ func TestServiceUpdateTagDoesNotExist(t *testing.T) {
 	_, _, _, err := fakeServiceUpdate(orig, []string{
 		"service", "update", "foo", "--untag", "foo", "--no-wait"})
 
-	assert.Error(t, err, "Error: tag foo does not exist")
+	assert.Assert(t, util.ContainsAll(err.Error(), "tag(s)", "foo", "not present", "service", "foo"))
 }
 
 func newEmptyService() *servingv1.Service {

pkg/kn/traffic/compute.go

@@ -83,15 +83,15 @@ func (e ServiceTraffic) isTagPresent(tag string) bool {
 	return false
 }
 
-func (e ServiceTraffic) untagRevision(tag string) error {
+func (e ServiceTraffic) untagRevision(tag string, serviceName string) string {
 	for i, target := range e {
 		if target.Tag == tag {
 			e[i].Tag = ""
-			return nil
+			return ""
 		}
 	}
 
-	return fmt.Errorf("Error: tag %s does not exist", tag)
+	return tag
 }
 
 func (e ServiceTraffic) isRevisionPresent(revision string) bool {
@@ -269,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
@@ -278,17 +279,17 @@ func Compute(cmd *cobra.Command, targets []servingv1.TrafficTarget, trafficFlags
 	traffic := newServiceTraffic(targets)
 
 	// First precedence: Untag revisions
-	var errStrings []string
+	var errTagNames []string
 	for _, tag := range trafficFlags.UntagRevisions {
-		err = traffic.untagRevision(tag)
-		if err != nil {
-			errStrings = append(errStrings, err.Error())
+		tagName := traffic.untagRevision(tag, serviceName)
+		if tagName != "" {
+			errTagNames = append(errTagNames, tagName)
 		}
 	}
 
 	// Return all errors from untagging revisions
-	if len(errStrings) > 0 {
-		return nil, fmt.Errorf(strings.Join(errStrings, "\n"))
+	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)
 			}
@@ -282,20 +282,20 @@ func TestComputeErrMsg(t *testing.T) {
 			"untag single tag that does not exist",
 			append(newServiceTraffic([]servingv1.TrafficTarget{}), newTarget("latest", "echo-v1", 100, false)),
 			[]string{"--untag", "foo"},
-			"Error: tag foo does not exist",
+			"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"},
-			"Error: tag foo does not exist\nError: tag bar does not exist",
+			"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.Stdout, "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)