From b787b33c54a6e562f13e5cb00b317178e2cd0d0c Mon Sep 17 00:00:00 2001 From: lvjiawei Date: Wed, 25 Dec 2019 19:00:03 +0800 Subject: [PATCH] Handling map like options in a same way Fixes #577 --- .../service/configuration_edit_flags.go | 27 +++---------- pkg/kn/commands/trigger/update_flags.go | 39 +++++-------------- pkg/kn/commands/trigger/update_flags_test.go | 14 +++++-- pkg/util/parsing_helper.go | 17 ++++++++ pkg/util/parsing_helper_test.go | 24 +++++++++++- 5 files changed, 65 insertions(+), 56 deletions(-) diff --git a/pkg/kn/commands/service/configuration_edit_flags.go b/pkg/kn/commands/service/configuration_edit_flags.go index d74100081f..6c1505c9c9 100644 --- a/pkg/kn/commands/service/configuration_edit_flags.go +++ b/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 diff --git a/pkg/kn/commands/trigger/update_flags.go b/pkg/kn/commands/trigger/update_flags.go index e18317b6c3..e7fee79c6b 100644 --- a/pkg/kn/commands/trigger/update_flags.go +++ b/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 } diff --git a/pkg/kn/commands/trigger/update_flags_test.go b/pkg/kn/commands/trigger/update_flags_test.go index b52d932203..9c3682b069 100644 --- a/pkg/kn/commands/trigger/update_flags_test.go +++ b/pkg/kn/commands/trigger/update_flags_test.go @@ -15,6 +15,7 @@ package trigger import ( + "sort" "testing" "gotest.tools/assert" @@ -44,8 +45,9 @@ func TestGetFilters(t *testing.T) { 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"}, @@ -65,7 +67,7 @@ func TestGetFilters(t *testing.T) { 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") }) } diff --git a/pkg/util/parsing_helper.go b/pkg/util/parsing_helper.go index 74a68f4b4a..8bd77f5ade 100644 --- a/pkg/util/parsing_helper.go +++ b/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] } } diff --git a/pkg/util/parsing_helper_test.go b/pkg/util/parsing_helper_test.go index 0afc4f068b..b04757d6b7 100644 --- a/pkg/util/parsing_helper_test.go +++ b/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) +}