From 99d8b2269b1fc9648cc784ca0d48070ffa4cbca8 Mon Sep 17 00:00:00 2001 From: Lv Jiawei Date: Thu, 2 Jan 2020 14:57:41 +0800 Subject: [PATCH] Handling map like options in a same way (#592) Fixes #577 --- .../service/configuration_edit_flags.go | 27 +++---------- pkg/kn/commands/trigger/update_flags.go | 39 +++++-------------- pkg/kn/commands/trigger/update_flags_test.go | 20 ++++++---- pkg/util/parsing_helper.go | 17 ++++++++ pkg/util/parsing_helper_test.go | 24 +++++++++++- 5 files changed, 68 insertions(+), 59 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..9cabb07029 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..3a9b3c0c63 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" @@ -39,25 +40,26 @@ 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) { @@ -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) +}