Handle map-like options consistently

pkg/kn/commands/service/configuration_edit_flags.go

@@ -176,13 +176,8 @@ func (p *ConfigurationEditFlags) Apply(
 		if err != nil {
 			return errors.Wrap(err, "Invalid --env")
 		}
-		envToRemove := []string{}
-		for key := range envMap {
-			if strings.HasSuffix(key, "-") {
-				envToRemove = append(envToRemove, key[:len(key)-1])
-				delete(envMap, key)
-			}
-		}
+
+		envToRemove := util.ParseMinusSuffix(envMap)
 		err = servinglib.UpdateEnvVars(template, envMap, envToRemove)
 		if err != nil {
 			return err
@@ -314,13 +309,8 @@ func (p *ConfigurationEditFlags) Apply(
 		if err != nil {
 			return errors.Wrap(err, "Invalid --label")
 		}
-		labelsToRemove := []string{}
-		for key := range labelsMap {
-			if strings.HasSuffix(key, "-") {
-				labelsToRemove = append(labelsToRemove, key[:len(key)-1])
-				delete(labelsMap, key)
-			}
-		}
+
+		labelsToRemove := util.ParseMinusSuffix(labelsMap)
 		err = servinglib.UpdateLabels(service, template, labelsMap, labelsToRemove)
 		if err != nil {
 			return err
@@ -332,13 +322,8 @@ func (p *ConfigurationEditFlags) Apply(
 		if err != nil {
 			return errors.Wrap(err, "Invalid --annotation")
 		}
-		annotationsToRemove := []string{}
-		for key := range annotationsMap {
-			if strings.HasSuffix(key, "-") {
-				annotationsToRemove = append(annotationsToRemove, key[:len(key)-1])
-				delete(annotationsMap, key)
-			}
-		}
+
+		annotationsToRemove := util.ParseMinusSuffix(annotationsMap)
 		err = servinglib.UpdateAnnotations(service, template, annotationsMap, annotationsToRemove)
 		if err != nil {
 			return err

pkg/kn/commands/trigger/update_flags.go

@@ -15,10 +15,10 @@
 package trigger
 
 import (
-	"fmt"
-	"strings"
-
+	"github.com/pkg/errors"
 	"github.com/spf13/cobra"
+
+	"knative.dev/client/pkg/util"
 )
 
 type filterArray []string
@@ -48,39 +48,20 @@ type TriggerUpdateFlags struct {
 
 // GetFilter to return a map type of filters
 func (f *TriggerUpdateFlags) GetFilters() (map[string]string, error) {
-	filters := map[string]string{}
-	for _, item := range f.Filters {
-		parts := strings.Split(item, "=")
-		if len(parts) < 2 || parts[0] == "" || parts[1] == "" {
-			return nil, fmt.Errorf("invalid filter %s", f.Filters)
-		} else {
-			if _, ok := filters[parts[0]]; ok {
-				return nil, fmt.Errorf("duplicate key '%s' in filters %s", parts[0], f.Filters)
-			}
-			filters[parts[0]] = parts[1]
-		}
+	filters, err := util.MapFromArray(f.Filters, "=")
+	if err != nil {
+		return nil, errors.Wrap(err, "Invalid --filter")
 	}
 	return filters, nil
 }
 
 // GetFilter to return a map type of filters
 func (f *TriggerUpdateFlags) GetUpdateFilters() (map[string]string, []string, error) {
-	filters := map[string]string{}
-	var removes []string
-	for _, item := range f.Filters {
-		if strings.HasSuffix(item, "-") {
-			removes = append(removes, item[:len(item)-1])
-		} else {
-			parts := strings.Split(item, "=")
-			if len(parts) < 2 || parts[0] == "" || parts[1] == "" {
-				return nil, nil, fmt.Errorf("invalid filter %s", f.Filters)
-			}
-			if _, ok := filters[parts[0]]; ok {
-				return nil, nil, fmt.Errorf("duplicate key '%s' in filters %s", parts[0], f.Filters)
-			}
-			filters[parts[0]] = parts[1]
-		}
+	filters, err := util.MapFromArrayAllowingSingles(f.Filters, "=")
+	if err != nil {
+		return nil, nil, errors.Wrap(err, "Invalid --filter")
 	}
+	removes := util.ParseMinusSuffix(filters)
 	return filters, removes, nil
 }
 

pkg/kn/commands/trigger/update_flags_test.go

@@ -15,6 +15,7 @@
 package trigger
 
 import (
+	"sort"
 	"testing"
 
 	"gotest.tools/assert"
@@ -39,33 +40,34 @@ func TestGetFilters(t *testing.T) {
 			Filters: filterArray{"type"},
 		}
 		_, err := createFlag.GetFilters()
-		assert.ErrorContains(t, err, "invalid filter")
+		assert.ErrorContains(t, err, "Invalid --filter")
 
 		createFlag = TriggerUpdateFlags{
 			Filters: filterArray{"type="},
 		}
-		_, err = createFlag.GetFilters()
-		assert.ErrorContains(t, err, "invalid filter")
+		filters, err := createFlag.GetFilters()
+		wanted := map[string]string{"type": ""}
+		assert.DeepEqual(t, wanted, filters)
 
 		createFlag = TriggerUpdateFlags{
 			Filters: filterArray{"=value"},
 		}
 		_, err = createFlag.GetFilters()
-		assert.ErrorContains(t, err, "invalid filter")
+		assert.ErrorContains(t, err, "Invalid --filter")
 
 		createFlag = TriggerUpdateFlags{
 			Filters: filterArray{"="},
 		}
 		_, err = createFlag.GetFilters()
-		assert.ErrorContains(t, err, "invalid filter")
+		assert.ErrorContains(t, err, "Invalid --filter")
 	})
 
 	t.Run("get duplicate filters", func(t *testing.T) {
 		createFlag := TriggerUpdateFlags{
 			Filters: filterArray{"type=foo", "type=bar"},
 		}
 		_, err := createFlag.GetFilters()
-		assert.ErrorContains(t, err, "duplicate key")
+		assert.ErrorContains(t, err, "duplicate")
 	})
 }
 
@@ -90,6 +92,8 @@ func TestGetUpdateFilters(t *testing.T) {
 		}
 		updated, removed, err := createFlag.GetUpdateFilters()
 		wanted := []string{"type", "attr"}
+		sort.Strings(wanted)
+		sort.Strings(removed)
 		assert.NilError(t, err, "UpdateFilter should be created")
 		assert.DeepEqual(t, wanted, removed)
 		assert.Assert(t, len(updated) == 0)
@@ -105,6 +109,8 @@ func TestGetUpdateFilters(t *testing.T) {
 			"type":   "foo",
 			"source": "bar",
 		}
+		sort.Strings(wantedRemoved)
+		sort.Strings(removed)
 		assert.NilError(t, err, "UpdateFilter should be created")
 		assert.DeepEqual(t, wantedRemoved, removed)
 		assert.DeepEqual(t, wantedUpdated, updated)
@@ -115,6 +121,6 @@ func TestGetUpdateFilters(t *testing.T) {
 			Filters: filterArray{"type=foo", "type=bar"},
 		}
 		_, _, err := createFlag.GetUpdateFilters()
-		assert.ErrorContains(t, err, "duplicate key")
+		assert.ErrorContains(t, err, "duplicate")
 	})
 }

pkg/util/parsing_helper.go

@@ -50,6 +50,17 @@ func MapFromArray(arr []string, delimiter string) (map[string]string, error) {
 	return mapFromArray(arr, delimiter, false)
 }
 
+func ParseMinusSuffix(m map[string]string) []string {
+	stringToRemove := []string{}
+	for key := range m {
+		if strings.HasSuffix(key, "-") {
+			stringToRemove = append(stringToRemove, key[:len(key)-1])
+			delete(m, key)
+		}
+	}
+	return stringToRemove
+}
+
 // mapFromArray takes an array of strings where each item is a (key, value) pair
 // separated by a delimiter and returns a map where keys are mapped to their respective values.
 // If allowSingles is true, values without a delimiter will be added as keys pointing to empty strings
@@ -63,6 +74,12 @@ func mapFromArray(arr []string, delimiter string, allowSingles bool) (map[string
 			}
 			returnMap[pairSlice[0]] = ""
 		} else {
+			if pairSlice[0] == "" {
+				return nil, fmt.Errorf("The key is empty")
+			}
+			if _, ok := returnMap[pairSlice[0]]; ok {
+				return nil, fmt.Errorf("The key %q has been duplicate in %v", pairSlice[0], arr)
+			}
 			returnMap[pairSlice[0]] = pairSlice[1]
 		}
 	}

pkg/util/parsing_helper_test.go

@@ -23,9 +23,8 @@ import (
 func TestMapFromArray(t *testing.T) {
 	testMapFromArray(t, []string{"good=value"}, "=", map[string]string{"good": "value"})
 	testMapFromArray(t, []string{"multi=value", "other=value"}, "=", map[string]string{"multi": "value", "other": "value"})
-	testMapFromArray(t, []string{"over|write", "over|written"}, "|", map[string]string{"over": "written"})
 	testMapFromArray(t, []string{"only,split,once", "just,once,"}, ",", map[string]string{"only": "split,once", "just": "once,"})
-	testMapFromArray(t, []string{"empty=", "="}, "=", map[string]string{"empty": "", "": ""})
+	testMapFromArray(t, []string{"empty="}, "=", map[string]string{"empty": ""})
 }
 
 func testMapFromArray(t *testing.T, input []string, delimiter string, expected map[string]string) {
@@ -72,3 +71,24 @@ func TestMapFromArrayEmptyValueEmptyDelimiterAllowingSingles(t *testing.T) {
 	_, err := MapFromArrayAllowingSingles(input, "")
 	assert.ErrorContains(t, err, "Argument requires")
 }
+
+func TestMapFromArrayMapRepeat(t *testing.T) {
+	input := []string{"a1=b1", "a1=b2"}
+	_, err := MapFromArrayAllowingSingles(input, "=")
+	assert.ErrorContains(t, err, "duplicate")
+}
+
+func TestMapFromArrayMapKeyEmpty(t *testing.T) {
+	input := []string{"=a1"}
+	_, err := MapFromArrayAllowingSingles(input, "=")
+	assert.ErrorContains(t, err, "empty")
+}
+
+func TestParseMinusSuffix(t *testing.T) {
+	inputMap := map[string]string{"a1": "b1", "a2-": ""}
+	expectedMap := map[string]string{"a1": "b1"}
+	expectedStringToRemove := []string{"a2"}
+	stringToRemove := ParseMinusSuffix(inputMap)
+	assert.DeepEqual(t, expectedMap, inputMap)
+	assert.DeepEqual(t, expectedStringToRemove, stringToRemove)
+}