From b9c6e527e65b209a3b01fee94bb0177aed1c164f Mon Sep 17 00:00:00 2001 From: Aaron Lerner Date: Tue, 6 Aug 2019 15:27:43 -0700 Subject: [PATCH 01/18] Add support for service labels with --label or -l --- .../service/configuration_edit_flags.go | 44 ++++++++++++++----- pkg/serving/config_changes.go | 11 +++++ 2 files changed, 44 insertions(+), 11 deletions(-) diff --git a/pkg/kn/commands/service/configuration_edit_flags.go b/pkg/kn/commands/service/configuration_edit_flags.go index 01b5a7a37e..8e639f747f 100644 --- a/pkg/kn/commands/service/configuration_edit_flags.go +++ b/pkg/kn/commands/service/configuration_edit_flags.go @@ -35,6 +35,7 @@ type ConfigurationEditFlags struct { ConcurrencyTarget int ConcurrencyLimit int Port int32 + Label []string } type ResourceFlags struct { @@ -56,6 +57,7 @@ func (p *ConfigurationEditFlags) AddUpdateFlags(command *cobra.Command) { command.Flags().IntVar(&p.ConcurrencyTarget, "concurrency-target", 0, "Recommendation for when to scale up based on the concurrent number of incoming request. Defaults to --concurrency-limit when given.") command.Flags().IntVar(&p.ConcurrencyLimit, "concurrency-limit", 0, "Hard Limit of concurrent requests to be processed by a single replica.") command.Flags().Int32VarP(&p.Port, "port", "p", 0, "The port where application listens on.") + command.Flags().StringArrayVarP(&p.Label, "label", "l", []string{}, "Service label to set. NAME=value; you may provide this flag any number of times to set multiple labels.") } func (p *ConfigurationEditFlags) AddCreateFlags(command *cobra.Command) { @@ -71,18 +73,15 @@ func (p *ConfigurationEditFlags) Apply(service *servingv1alpha1.Service, cmd *co return err } - envMap := map[string]string{} - for _, pairStr := range p.Env { - pairSlice := strings.SplitN(pairStr, "=", 2) - if len(pairSlice) <= 1 { - return fmt.Errorf( - "--env argument requires a value that contains the '=' character; got %s", - pairStr) + if cmd.Flags().Changed("env") { + envMap, err := p.mapFromArray(p.Env, "=", "--env") + if err != nil { + return err + } + err = servinglib.UpdateEnvVars(template, envMap) + if err != nil { + return err } - envMap[pairSlice[0]] = pairSlice[1] - } - if err := servinglib.UpdateEnvVars(template, envMap); err != nil { - return err } if cmd.Flags().Changed("image") { @@ -139,6 +138,17 @@ func (p *ConfigurationEditFlags) Apply(service *servingv1alpha1.Service, cmd *co } } + if cmd.Flags().Changed("label") { + labelMap, err := p.mapFromArray(p.Label, "=", "--label") + if err != nil { + return err + } + err = servinglib.UpdateServiceLabels(service, labelMap) + if err != nil { + return err + } + } + return nil } @@ -165,3 +175,15 @@ func (p *ConfigurationEditFlags) computeResources(resourceFlags ResourceFlags) ( return resourceList, nil } + +func (p *ConfigurationEditFlags) mapFromArray(arr []string, delimiter string, flag string) (map[string]string, error) { + returnMap := map[string]string{} + for _, pairStr := range arr { + pairSlice := strings.SplitN(pairStr, delimiter, 2) + if len(pairSlice) <= 1 { + return nil, fmt.Errorf("%s argument requires a value that contains the '=' character; got %s", flag, pairStr) + } + returnMap[pairSlice[0]] = pairSlice[1] + } + return returnMap, nil +} diff --git a/pkg/serving/config_changes.go b/pkg/serving/config_changes.go index 7cc8bfbf1c..f51dc74415 100644 --- a/pkg/serving/config_changes.go +++ b/pkg/serving/config_changes.go @@ -152,6 +152,17 @@ func UpdateResources(template *servingv1alpha1.RevisionTemplateSpec, requestsRes return nil } +// UpdateServiceLables updates the labels on a service +func UpdateServiceLabels(service *servingv1alpha1.Service, vars map[string]string) error { + if service.ObjectMeta.Labels == nil { + service.ObjectMeta.Labels = make(map[string]string) + } + for key, value := range vars { + service.ObjectMeta.Labels[key] = value + } + return nil +} + // ======================================================================================= func updateEnvVarsFromMap(env []corev1.EnvVar, vars map[string]string) []corev1.EnvVar { From 158202f7bcc4f6e5e18d55eee1b3a08e97d34074 Mon Sep 17 00:00:00 2001 From: Aaron Lerner Date: Tue, 6 Aug 2019 15:28:05 -0700 Subject: [PATCH 02/18] Add tests --- .../commands/service/service_create_test.go | 27 ++++++++++ .../commands/service/service_update_test.go | 45 ++++++++++++++++ pkg/serving/config_changes_test.go | 52 +++++++++++++++++++ 3 files changed, 124 insertions(+) diff --git a/pkg/kn/commands/service/service_create_test.go b/pkg/kn/commands/service/service_create_test.go index 8bafc37906..004d7aac76 100644 --- a/pkg/kn/commands/service/service_create_test.go +++ b/pkg/kn/commands/service/service_create_test.go @@ -467,3 +467,30 @@ func TestServiceCreateEnvForce(t *testing.T) { t.Fatalf("wrong output: %s", output) } } + +func TestServiceCreateLabel(t *testing.T) { + action, created, _, err := fakeServiceCreate([]string{ + "service", "create", "foo", "--image", "gcr.io/foo/bar:baz", "-l", "a=mouse", "--label", "b=cookie", "--async"}, false, false) + + if err != nil { + t.Fatal(err) + } else if !action.Matches("create", "services") { + t.Fatalf("Bad action %v", action) + } + + expectedLabels := map[string]string{ + "a": "mouse", + "b": "cookie"} + actualLabels := created.ObjectMeta.Labels + + template, err := servinglib.RevisionTemplateOfService(created) + if err != nil { + t.Fatal(err) + } else if template.Spec.DeprecatedContainer.Image != "gcr.io/foo/bar:baz" { + t.Fatalf("wrong image set: %v", template.Spec.DeprecatedContainer.Image) + } else if !reflect.DeepEqual( + actualLabels, + expectedLabels) { + t.Fatalf("wrong labels %v", created.ObjectMeta.Labels) + } +} diff --git a/pkg/kn/commands/service/service_update_test.go b/pkg/kn/commands/service/service_update_test.go index 3a832447d6..3ecca70514 100644 --- a/pkg/kn/commands/service/service_update_test.go +++ b/pkg/kn/commands/service/service_update_test.go @@ -374,6 +374,51 @@ func TestServiceUpdateRequestsLimitsCPU_and_Memory(t *testing.T) { } } +func TestServiceUpdateLabelWhenEmpty(t *testing.T) { + original := newEmptyService() + + action, updated, _, err := fakeServiceUpdate(original, []string{ + "service", "update", "foo", "-l", "a=mouse", "--label", "b=cookie", "--async"}, false) + + if err != nil { + t.Fatal(err) + } else if !action.Matches("update", "services") { + t.Fatalf("Bad action %v", action) + } + + expectedLabels := map[string]string{ + "a": "mouse", + "b": "cookie"} + actualLabels := updated.ObjectMeta.Labels + + if !reflect.DeepEqual(actualLabels, expectedLabels) { + t.Fatalf("wrong labels %v", actualLabels) + } +} + +func TestServiceUpdateLabelExisting(t *testing.T) { + original := newEmptyService() + original.ObjectMeta.Labels = map[string]string{"already": "here"} + + action, updated, _, err := fakeServiceUpdate(original, []string{ + "service", "update", "foo", "-l", "already=gone", "--label", "b=cookie", "--async"}, false) + + if err != nil { + t.Fatal(err) + } else if !action.Matches("update", "services") { + t.Fatalf("Bad action %v", action) + } + + expectedLabels := map[string]string{ + "already": "gone", + "b": "cookie"} + actualLabels := updated.ObjectMeta.Labels + + if !reflect.DeepEqual(actualLabels, expectedLabels) { + t.Fatalf("wrong labels %v", actualLabels) + } +} + func newEmptyService() *v1alpha1.Service { return &v1alpha1.Service{ TypeMeta: metav1.TypeMeta{ diff --git a/pkg/serving/config_changes_test.go b/pkg/serving/config_changes_test.go index e07dc5e089..3855ecbd87 100644 --- a/pkg/serving/config_changes_test.go +++ b/pkg/serving/config_changes_test.go @@ -26,6 +26,7 @@ import ( servingv1alpha1 "github.com/knative/serving/pkg/apis/serving/v1alpha1" corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) func TestUpdateAutoscalingAnnotations(t *testing.T) { @@ -270,6 +271,35 @@ func testUpdateEnvVarsBoth(t *testing.T, template *servingv1alpha1.RevisionTempl } } +func TestUpdateLabelsNew(t *testing.T) { + service := getV1alpha1Service() + labels := map[string]string{ + "a": "foo", + "b": "bar", + } + err := UpdateServiceLabels(service, labels) + assert.NilError(t, err) + actual := service.ObjectMeta.Labels + if !reflect.DeepEqual(labels, actual) { + t.Fatalf("Labels did not match expected %v found %v", labels, actual) + } +} + +func TestUpdateLabelsExisting(t *testing.T) { + service := getV1alpha1Service() + service.ObjectMeta.Labels = map[string]string{"a": "foo"} + labels := map[string]string{ + "a": "notfood", + "b": "bar", + } + err := UpdateServiceLabels(service, labels) + assert.NilError(t, err) + actual := service.ObjectMeta.Labels + if !reflect.DeepEqual(labels, actual) { + t.Fatalf("Labels did not match expected %v found %v", labels, actual) + } +} + // ========================================================================================================= func getV1alpha1RevisionTemplateWithOldFields() (*servingv1alpha1.RevisionTemplateSpec, *corev1.Container) { @@ -296,6 +326,28 @@ func getV1alpha1Config() (*servingv1alpha1.RevisionTemplateSpec, *corev1.Contain return template, &containers[0] } +func getV1alpha1Service() *servingv1alpha1.Service { + template, _ := getV1alpha1RevisionTemplateWithOldFields() + service := &servingv1alpha1.Service{ + TypeMeta: metav1.TypeMeta{ + Kind: "Service", + APIVersion: "knative.dev/v1alph1", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: "foo", + Namespace: "default", + }, + Spec: servingv1alpha1.ServiceSpec{ + DeprecatedRunLatest: &servingv1alpha1.RunLatestType{ + Configuration: servingv1alpha1.ConfigurationSpec{ + DeprecatedRevisionTemplate: template, + }, + }, + }, + } + return service +} + func assertNoV1alpha1Old(t *testing.T, template *servingv1alpha1.RevisionTemplateSpec) { if template.Spec.DeprecatedContainer != nil { t.Error("Assuming only new v1alpha1 fields but found spec.container") From ac92237ac85d7d9b95e28e39405306c2932237fe Mon Sep 17 00:00:00 2001 From: Aaron Lerner Date: Tue, 6 Aug 2019 15:42:21 -0700 Subject: [PATCH 03/18] Fix typo --- pkg/serving/config_changes.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/serving/config_changes.go b/pkg/serving/config_changes.go index f51dc74415..5849d7a825 100644 --- a/pkg/serving/config_changes.go +++ b/pkg/serving/config_changes.go @@ -152,7 +152,7 @@ func UpdateResources(template *servingv1alpha1.RevisionTemplateSpec, requestsRes return nil } -// UpdateServiceLables updates the labels on a service +// UpdateServiceLabels updates the labels on a service func UpdateServiceLabels(service *servingv1alpha1.Service, vars map[string]string) error { if service.ObjectMeta.Labels == nil { service.ObjectMeta.Labels = make(map[string]string) From f1f01474078b4f2e6d6dc4b5f41d0b4e1fdf3a6a Mon Sep 17 00:00:00 2001 From: Aaron Lerner Date: Tue, 6 Aug 2019 18:27:50 -0700 Subject: [PATCH 04/18] Move MapToArray helper method to separate file --- pkg/kn/commands/parsing_helper.go | 34 +++++++++++++ pkg/kn/commands/parsing_helper_test.go | 49 +++++++++++++++++++ .../service/configuration_edit_flags.go | 24 ++------- 3 files changed, 88 insertions(+), 19 deletions(-) create mode 100644 pkg/kn/commands/parsing_helper.go create mode 100644 pkg/kn/commands/parsing_helper_test.go diff --git a/pkg/kn/commands/parsing_helper.go b/pkg/kn/commands/parsing_helper.go new file mode 100644 index 0000000000..f97e493ddf --- /dev/null +++ b/pkg/kn/commands/parsing_helper.go @@ -0,0 +1,34 @@ +// Copyright © 2019 The Knative Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package commands + +import ( + "fmt" + "strings" +) + +// 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 respsective values. +func MapFromArray(arr []string, delimiter string, flag string) (map[string]string, error) { + returnMap := map[string]string{} + for _, pairStr := range arr { + pairSlice := strings.SplitN(pairStr, delimiter, 2) + if len(pairSlice) <= 1 { + return nil, fmt.Errorf("%s argument requires a value that contains the %q character; got %q", flag, delimiter, pairStr) + } + returnMap[pairSlice[0]] = pairSlice[1] + } + return returnMap, nil +} diff --git a/pkg/kn/commands/parsing_helper_test.go b/pkg/kn/commands/parsing_helper_test.go new file mode 100644 index 0000000000..0dd38b5690 --- /dev/null +++ b/pkg/kn/commands/parsing_helper_test.go @@ -0,0 +1,49 @@ +// Copyright © 2018 The Knative Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package commands + +import ( + "reflect" + "testing" + + "gotest.tools/assert" +) + +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"}, ",", map[string]string{"only": "split,once"}) +} + +func testMapFromArray(t *testing.T, input []string, delimiter string, expected map[string]string) { + actual, err := MapFromArray(input, delimiter, "--flag") + assert.NilError(t, err) + if !reflect.DeepEqual(expected, actual) { + t.Fatalf("Map did not match expected: %s\nFound: %s", expected, actual) + } +} + +func TestMapFromArrayNoDelimiter(t *testing.T) { + input := []string{"good=value", "badvalue"} + _, err := MapFromArray(input, "=", "--flag") + assert.ErrorContains(t, err, "argument requires") +} + +func TestMapFromArrayEmptyValue(t *testing.T) { + input := []string{""} + _, err := MapFromArray(input, "=", "--flag") + assert.ErrorContains(t, err, "argument requires") +} diff --git a/pkg/kn/commands/service/configuration_edit_flags.go b/pkg/kn/commands/service/configuration_edit_flags.go index 8e639f747f..7ba9984752 100644 --- a/pkg/kn/commands/service/configuration_edit_flags.go +++ b/pkg/kn/commands/service/configuration_edit_flags.go @@ -15,9 +15,7 @@ package service import ( - "fmt" - "strings" - + commands "github.com/knative/client/pkg/kn/commands" servinglib "github.com/knative/client/pkg/serving" servingv1alpha1 "github.com/knative/serving/pkg/apis/serving/v1alpha1" "github.com/spf13/cobra" @@ -35,7 +33,7 @@ type ConfigurationEditFlags struct { ConcurrencyTarget int ConcurrencyLimit int Port int32 - Label []string + Labels []string } type ResourceFlags struct { @@ -57,7 +55,7 @@ func (p *ConfigurationEditFlags) AddUpdateFlags(command *cobra.Command) { command.Flags().IntVar(&p.ConcurrencyTarget, "concurrency-target", 0, "Recommendation for when to scale up based on the concurrent number of incoming request. Defaults to --concurrency-limit when given.") command.Flags().IntVar(&p.ConcurrencyLimit, "concurrency-limit", 0, "Hard Limit of concurrent requests to be processed by a single replica.") command.Flags().Int32VarP(&p.Port, "port", "p", 0, "The port where application listens on.") - command.Flags().StringArrayVarP(&p.Label, "label", "l", []string{}, "Service label to set. NAME=value; you may provide this flag any number of times to set multiple labels.") + command.Flags().StringArrayVarP(&p.Labels, "label", "l", []string{}, "Service label to set. NAME=value; you may provide this flag any number of times to set multiple labels.") } func (p *ConfigurationEditFlags) AddCreateFlags(command *cobra.Command) { @@ -74,7 +72,7 @@ func (p *ConfigurationEditFlags) Apply(service *servingv1alpha1.Service, cmd *co } if cmd.Flags().Changed("env") { - envMap, err := p.mapFromArray(p.Env, "=", "--env") + envMap, err := commands.MapFromArray(p.Env, "=", "--env") if err != nil { return err } @@ -139,7 +137,7 @@ func (p *ConfigurationEditFlags) Apply(service *servingv1alpha1.Service, cmd *co } if cmd.Flags().Changed("label") { - labelMap, err := p.mapFromArray(p.Label, "=", "--label") + labelMap, err := commands.MapFromArray(p.Labels, "=", "--label") if err != nil { return err } @@ -175,15 +173,3 @@ func (p *ConfigurationEditFlags) computeResources(resourceFlags ResourceFlags) ( return resourceList, nil } - -func (p *ConfigurationEditFlags) mapFromArray(arr []string, delimiter string, flag string) (map[string]string, error) { - returnMap := map[string]string{} - for _, pairStr := range arr { - pairSlice := strings.SplitN(pairStr, delimiter, 2) - if len(pairSlice) <= 1 { - return nil, fmt.Errorf("%s argument requires a value that contains the '=' character; got %s", flag, pairStr) - } - returnMap[pairSlice[0]] = pairSlice[1] - } - return returnMap, nil -} From b9992db3dd04e252036485bf72fc8328b6356425 Mon Sep 17 00:00:00 2001 From: Aaron Lerner Date: Tue, 6 Aug 2019 18:34:51 -0700 Subject: [PATCH 05/18] Allow unsetting labels by passing empty value --- .../service/configuration_edit_flags.go | 2 +- pkg/serving/config_changes.go | 7 ++++++- pkg/serving/config_changes_test.go | 18 ++++++++++++++++++ 3 files changed, 25 insertions(+), 2 deletions(-) diff --git a/pkg/kn/commands/service/configuration_edit_flags.go b/pkg/kn/commands/service/configuration_edit_flags.go index 7ba9984752..cbf64bb66d 100644 --- a/pkg/kn/commands/service/configuration_edit_flags.go +++ b/pkg/kn/commands/service/configuration_edit_flags.go @@ -55,7 +55,7 @@ func (p *ConfigurationEditFlags) AddUpdateFlags(command *cobra.Command) { command.Flags().IntVar(&p.ConcurrencyTarget, "concurrency-target", 0, "Recommendation for when to scale up based on the concurrent number of incoming request. Defaults to --concurrency-limit when given.") command.Flags().IntVar(&p.ConcurrencyLimit, "concurrency-limit", 0, "Hard Limit of concurrent requests to be processed by a single replica.") command.Flags().Int32VarP(&p.Port, "port", "p", 0, "The port where application listens on.") - command.Flags().StringArrayVarP(&p.Labels, "label", "l", []string{}, "Service label to set. NAME=value; you may provide this flag any number of times to set multiple labels.") + command.Flags().StringArrayVarP(&p.Labels, "label", "l", []string{}, "Service label to set. NAME=value; provide an empty string for the value to remove a label. You may provide this flag any number of times to set multiple labels.") } func (p *ConfigurationEditFlags) AddCreateFlags(command *cobra.Command) { diff --git a/pkg/serving/config_changes.go b/pkg/serving/config_changes.go index 5849d7a825..9150bd8e77 100644 --- a/pkg/serving/config_changes.go +++ b/pkg/serving/config_changes.go @@ -158,7 +158,12 @@ func UpdateServiceLabels(service *servingv1alpha1.Service, vars map[string]strin service.ObjectMeta.Labels = make(map[string]string) } for key, value := range vars { - service.ObjectMeta.Labels[key] = value + // Delete the label if passed an empty string, otherwise set or update it + if value == "" { + delete(service.ObjectMeta.Labels, key) + } else { + service.ObjectMeta.Labels[key] = value + } } return nil } diff --git a/pkg/serving/config_changes_test.go b/pkg/serving/config_changes_test.go index 3855ecbd87..3a55d2be70 100644 --- a/pkg/serving/config_changes_test.go +++ b/pkg/serving/config_changes_test.go @@ -300,6 +300,24 @@ func TestUpdateLabelsExisting(t *testing.T) { } } +func TestUpdateLabelsRemoveExisting(t *testing.T) { + service := getV1alpha1Service() + service.ObjectMeta.Labels = map[string]string{"a": "foo", "b": "bar"} + labels := map[string]string{ + "a": "foo", + "b": "", + } + error := UpdateServiceLabels(service, labels) + assert.NilError(t, err) + expected := map[string]string{ + "a": "foo", + } + actual := service.ObjectMeta.Labels + if !reflect.DeepEqual(labels, actual) { + t.Fatalf("Labels did not match expected %v found %v", labels, actual) + } +} + // ========================================================================================================= func getV1alpha1RevisionTemplateWithOldFields() (*servingv1alpha1.RevisionTemplateSpec, *corev1.Container) { From 578a4d50429251404ea5686e94274f32e51b9499 Mon Sep 17 00:00:00 2001 From: Aaron Lerner Date: Tue, 6 Aug 2019 18:41:41 -0700 Subject: [PATCH 06/18] Fix test --- pkg/serving/config_changes_test.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/pkg/serving/config_changes_test.go b/pkg/serving/config_changes_test.go index 3a55d2be70..ad15558042 100644 --- a/pkg/serving/config_changes_test.go +++ b/pkg/serving/config_changes_test.go @@ -307,14 +307,14 @@ func TestUpdateLabelsRemoveExisting(t *testing.T) { "a": "foo", "b": "", } - error := UpdateServiceLabels(service, labels) + err := UpdateServiceLabels(service, labels) assert.NilError(t, err) expected := map[string]string{ "a": "foo", } actual := service.ObjectMeta.Labels - if !reflect.DeepEqual(labels, actual) { - t.Fatalf("Labels did not match expected %v found %v", labels, actual) + if !reflect.DeepEqual(expected, actual) { + t.Fatalf("Labels did not match expected %v found %v", expected, actual) } } From 664f7035191db484fb6665fe54c9202008f465ef Mon Sep 17 00:00:00 2001 From: Aaron Lerner Date: Tue, 6 Aug 2019 18:43:27 -0700 Subject: [PATCH 07/18] Add test case for label removal --- pkg/kn/commands/service/service_update_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/kn/commands/service/service_update_test.go b/pkg/kn/commands/service/service_update_test.go index 3ecca70514..f21b2085cc 100644 --- a/pkg/kn/commands/service/service_update_test.go +++ b/pkg/kn/commands/service/service_update_test.go @@ -398,10 +398,10 @@ func TestServiceUpdateLabelWhenEmpty(t *testing.T) { func TestServiceUpdateLabelExisting(t *testing.T) { original := newEmptyService() - original.ObjectMeta.Labels = map[string]string{"already": "here"} + original.ObjectMeta.Labels = map[string]string{"already": "here", "tobe": "removed"} action, updated, _, err := fakeServiceUpdate(original, []string{ - "service", "update", "foo", "-l", "already=gone", "--label", "b=cookie", "--async"}, false) + "service", "update", "foo", "-l", "already=gone", "--label=tobe=", "--label", "b=cookie", "--async"}, false) if err != nil { t.Fatal(err) From 8cc9d57203aab6243e15f581af3eb3d990ac9db6 Mon Sep 17 00:00:00 2001 From: Aaron Lerner Date: Tue, 6 Aug 2019 19:05:35 -0700 Subject: [PATCH 08/18] Wrap error message with flag --- go.mod | 2 +- pkg/kn/commands/parsing_helper.go | 4 ++-- pkg/kn/commands/parsing_helper_test.go | 10 +++++----- pkg/kn/commands/service/configuration_edit_flags.go | 9 +++++---- 4 files changed, 13 insertions(+), 12 deletions(-) diff --git a/go.mod b/go.mod index f3e4e36950..8a1904faf9 100644 --- a/go.mod +++ b/go.mod @@ -25,7 +25,7 @@ require ( github.com/mitchellh/go-homedir v1.1.0 github.com/modern-go/reflect2 v1.0.1 // indirect github.com/peterbourgon/diskv v2.0.1+incompatible // indirect - github.com/pkg/errors v0.8.1 // indirect + github.com/pkg/errors v0.8.1 github.com/spf13/cobra v0.0.3 github.com/spf13/pflag v1.0.3 github.com/spf13/viper v1.3.1 diff --git a/pkg/kn/commands/parsing_helper.go b/pkg/kn/commands/parsing_helper.go index f97e493ddf..cf94982cca 100644 --- a/pkg/kn/commands/parsing_helper.go +++ b/pkg/kn/commands/parsing_helper.go @@ -21,12 +21,12 @@ import ( // 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 respsective values. -func MapFromArray(arr []string, delimiter string, flag string) (map[string]string, error) { +func MapFromArray(arr []string, delimiter string) (map[string]string, error) { returnMap := map[string]string{} for _, pairStr := range arr { pairSlice := strings.SplitN(pairStr, delimiter, 2) if len(pairSlice) <= 1 { - return nil, fmt.Errorf("%s argument requires a value that contains the %q character; got %q", flag, delimiter, pairStr) + return nil, fmt.Errorf("Argument requires a value that contains the %q character; got %q", delimiter, pairStr) } returnMap[pairSlice[0]] = pairSlice[1] } diff --git a/pkg/kn/commands/parsing_helper_test.go b/pkg/kn/commands/parsing_helper_test.go index 0dd38b5690..57a97593b9 100644 --- a/pkg/kn/commands/parsing_helper_test.go +++ b/pkg/kn/commands/parsing_helper_test.go @@ -29,7 +29,7 @@ func TestMapFromArray(t *testing.T) { } func testMapFromArray(t *testing.T, input []string, delimiter string, expected map[string]string) { - actual, err := MapFromArray(input, delimiter, "--flag") + actual, err := MapFromArray(input, delimiter) assert.NilError(t, err) if !reflect.DeepEqual(expected, actual) { t.Fatalf("Map did not match expected: %s\nFound: %s", expected, actual) @@ -38,12 +38,12 @@ func testMapFromArray(t *testing.T, input []string, delimiter string, expected m func TestMapFromArrayNoDelimiter(t *testing.T) { input := []string{"good=value", "badvalue"} - _, err := MapFromArray(input, "=", "--flag") - assert.ErrorContains(t, err, "argument requires") + _, err := MapFromArray(input, "=") + assert.ErrorContains(t, err, "Argument requires") } func TestMapFromArrayEmptyValue(t *testing.T) { input := []string{""} - _, err := MapFromArray(input, "=", "--flag") - assert.ErrorContains(t, err, "argument requires") + _, err := MapFromArray(input, "=") + assert.ErrorContains(t, err, "Argument requires") } diff --git a/pkg/kn/commands/service/configuration_edit_flags.go b/pkg/kn/commands/service/configuration_edit_flags.go index cbf64bb66d..892bcde289 100644 --- a/pkg/kn/commands/service/configuration_edit_flags.go +++ b/pkg/kn/commands/service/configuration_edit_flags.go @@ -18,6 +18,7 @@ import ( commands "github.com/knative/client/pkg/kn/commands" servinglib "github.com/knative/client/pkg/serving" servingv1alpha1 "github.com/knative/serving/pkg/apis/serving/v1alpha1" + errors "github.com/pkg/errors" "github.com/spf13/cobra" corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/resource" @@ -72,9 +73,9 @@ func (p *ConfigurationEditFlags) Apply(service *servingv1alpha1.Service, cmd *co } if cmd.Flags().Changed("env") { - envMap, err := commands.MapFromArray(p.Env, "=", "--env") + envMap, err := commands.MapFromArray(p.Env, "=") if err != nil { - return err + return errors.Wrap(err, "Invalid --env") } err = servinglib.UpdateEnvVars(template, envMap) if err != nil { @@ -137,9 +138,9 @@ func (p *ConfigurationEditFlags) Apply(service *servingv1alpha1.Service, cmd *co } if cmd.Flags().Changed("label") { - labelMap, err := commands.MapFromArray(p.Labels, "=", "--label") + labelMap, err := commands.MapFromArray(p.Labels, "=") if err != nil { - return err + return errors.Wrap(err, "Invalid --label") } err = servinglib.UpdateServiceLabels(service, labelMap) if err != nil { From 2a3fb20448a207290aa8382d32c83e0a55c2dc6e Mon Sep 17 00:00:00 2001 From: Aaron Lerner Date: Tue, 6 Aug 2019 19:06:36 -0700 Subject: [PATCH 09/18] Update docs to include --label --- docs/cmd/kn_service_create.md | 1 + docs/cmd/kn_service_update.md | 1 + 2 files changed, 2 insertions(+) diff --git a/docs/cmd/kn_service_create.md b/docs/cmd/kn_service_create.md index 4f0a2bed05..b51d103252 100644 --- a/docs/cmd/kn_service_create.md +++ b/docs/cmd/kn_service_create.md @@ -46,6 +46,7 @@ kn service create NAME --image IMAGE [flags] --force Create service forcefully, replaces existing service if any. -h, --help help for create --image string Image to run. + -l, --label stringArray Service label to set. NAME=value; provide an empty string for the value to remove a label. You may provide this flag any number of times to set multiple labels. --limits-cpu string The limits on the requested CPU (e.g., 1000m). --limits-memory string The limits on the requested memory (e.g., 1024Mi). --max-scale int Maximal number of replicas. diff --git a/docs/cmd/kn_service_update.md b/docs/cmd/kn_service_update.md index 707a37652e..7ebdc5ad07 100644 --- a/docs/cmd/kn_service_update.md +++ b/docs/cmd/kn_service_update.md @@ -33,6 +33,7 @@ kn service update NAME [flags] -e, --env stringArray Environment variable to set. NAME=value; you may provide this flag any number of times to set multiple environment variables. -h, --help help for update --image string Image to run. + -l, --label stringArray Service label to set. NAME=value; provide an empty string for the value to remove a label. You may provide this flag any number of times to set multiple labels. --limits-cpu string The limits on the requested CPU (e.g., 1000m). --limits-memory string The limits on the requested memory (e.g., 1024Mi). --max-scale int Maximal number of replicas. From 7cd588ec59b29160500e92441d910ce6384293b8 Mon Sep 17 00:00:00 2001 From: Aaron Lerner Date: Wed, 7 Aug 2019 16:45:19 -0700 Subject: [PATCH 10/18] Update labels on both services and revisions --- .../service/configuration_edit_flags.go | 2 +- pkg/serving/config_changes.go | 10 +++- pkg/serving/config_changes_test.go | 51 +++++++++++++++---- 3 files changed, 50 insertions(+), 13 deletions(-) diff --git a/pkg/kn/commands/service/configuration_edit_flags.go b/pkg/kn/commands/service/configuration_edit_flags.go index 892bcde289..60205ff6c4 100644 --- a/pkg/kn/commands/service/configuration_edit_flags.go +++ b/pkg/kn/commands/service/configuration_edit_flags.go @@ -142,7 +142,7 @@ func (p *ConfigurationEditFlags) Apply(service *servingv1alpha1.Service, cmd *co if err != nil { return errors.Wrap(err, "Invalid --label") } - err = servinglib.UpdateServiceLabels(service, labelMap) + err = servinglib.UpdateLabels(service, template, labelMap) if err != nil { return err } diff --git a/pkg/serving/config_changes.go b/pkg/serving/config_changes.go index 9150bd8e77..db2ccfe2e7 100644 --- a/pkg/serving/config_changes.go +++ b/pkg/serving/config_changes.go @@ -152,17 +152,23 @@ func UpdateResources(template *servingv1alpha1.RevisionTemplateSpec, requestsRes return nil } -// UpdateServiceLabels updates the labels on a service -func UpdateServiceLabels(service *servingv1alpha1.Service, vars map[string]string) error { +// UpdateLabels updates the labels identically on a service and template. +// Does not overwrite the entire Labels field, only makes the requested updates +func UpdateLabels(service *servingv1alpha1.Service, template *servingv1alpha1.RevisionTemplateSpec, vars map[string]string) error { if service.ObjectMeta.Labels == nil { service.ObjectMeta.Labels = make(map[string]string) } + if template.ObjectMeta.Labels == nil { + template.ObjectMeta.Labels = make(map[string]string) + } for key, value := range vars { // Delete the label if passed an empty string, otherwise set or update it if value == "" { delete(service.ObjectMeta.Labels, key) + delete(template.ObjectMeta.Labels, key) } else { service.ObjectMeta.Labels[key] = value + template.ObjectMeta.Labels[key] = value } } return nil diff --git a/pkg/serving/config_changes_test.go b/pkg/serving/config_changes_test.go index ad15558042..bb78fd6299 100644 --- a/pkg/serving/config_changes_test.go +++ b/pkg/serving/config_changes_test.go @@ -272,50 +272,73 @@ func testUpdateEnvVarsBoth(t *testing.T, template *servingv1alpha1.RevisionTempl } func TestUpdateLabelsNew(t *testing.T) { - service := getV1alpha1Service() + service, template, _ := getV1alpha1Service() + labels := map[string]string{ "a": "foo", "b": "bar", } - err := UpdateServiceLabels(service, labels) + err := UpdateLabels(service, template, labels) assert.NilError(t, err) + actual := service.ObjectMeta.Labels if !reflect.DeepEqual(labels, actual) { - t.Fatalf("Labels did not match expected %v found %v", labels, actual) + t.Fatalf("Service labels did not match expected %v found %v", labels, actual) + } + + actual = template.ObjectMeta.Labels + if !reflect.DeepEqual(labels, actual) { + t.Fatalf("Template labels did not match expected %v found %v", labels, actual) } } func TestUpdateLabelsExisting(t *testing.T) { - service := getV1alpha1Service() + service, template, _ := getV1alpha1Service() service.ObjectMeta.Labels = map[string]string{"a": "foo"} + template.ObjectMeta.Labels = map[string]string{"a": "foo"} + labels := map[string]string{ "a": "notfood", "b": "bar", } - err := UpdateServiceLabels(service, labels) + err := UpdateLabels(service, template, labels) assert.NilError(t, err) + actual := service.ObjectMeta.Labels if !reflect.DeepEqual(labels, actual) { t.Fatalf("Labels did not match expected %v found %v", labels, actual) } + + actual = template.ObjectMeta.Labels + if !reflect.DeepEqual(labels, actual) { + t.Fatalf("Template labels did not match expected %v found %v", labels, actual) + } } func TestUpdateLabelsRemoveExisting(t *testing.T) { - service := getV1alpha1Service() + service, template, _ := getV1alpha1Service() service.ObjectMeta.Labels = map[string]string{"a": "foo", "b": "bar"} + template.ObjectMeta.Labels = map[string]string{"a": "foo", "b": "bar"} + labels := map[string]string{ "a": "foo", "b": "", } - err := UpdateServiceLabels(service, labels) + err := UpdateLabels(service, template, labels) assert.NilError(t, err) expected := map[string]string{ "a": "foo", } + actual := service.ObjectMeta.Labels if !reflect.DeepEqual(expected, actual) { t.Fatalf("Labels did not match expected %v found %v", expected, actual) } + + actual = template.ObjectMeta.Labels + if !reflect.DeepEqual(expected, actual) { + t.Fatalf("Template labels did not match expected %v found %v", expected, actual) + } } // ========================================================================================================= @@ -323,6 +346,10 @@ func TestUpdateLabelsRemoveExisting(t *testing.T) { func getV1alpha1RevisionTemplateWithOldFields() (*servingv1alpha1.RevisionTemplateSpec, *corev1.Container) { container := &corev1.Container{} template := &servingv1alpha1.RevisionTemplateSpec{ + ObjectMeta: metav1.ObjectMeta{ + Name: "template-foo", + Namespace: "default", + }, Spec: servingv1alpha1.RevisionSpec{ DeprecatedContainer: container, }, @@ -333,6 +360,10 @@ func getV1alpha1RevisionTemplateWithOldFields() (*servingv1alpha1.RevisionTempla func getV1alpha1Config() (*servingv1alpha1.RevisionTemplateSpec, *corev1.Container) { containers := []corev1.Container{{}} template := &servingv1alpha1.RevisionTemplateSpec{ + ObjectMeta: metav1.ObjectMeta{ + Name: "template-foo", + Namespace: "default", + }, Spec: servingv1alpha1.RevisionSpec{ RevisionSpec: v1beta1.RevisionSpec{ PodSpec: v1beta1.PodSpec{ @@ -344,8 +375,8 @@ func getV1alpha1Config() (*servingv1alpha1.RevisionTemplateSpec, *corev1.Contain return template, &containers[0] } -func getV1alpha1Service() *servingv1alpha1.Service { - template, _ := getV1alpha1RevisionTemplateWithOldFields() +func getV1alpha1Service() (*servingv1alpha1.Service, *servingv1alpha1.RevisionTemplateSpec, *corev1.Container) { + template, container := getV1alpha1RevisionTemplateWithOldFields() service := &servingv1alpha1.Service{ TypeMeta: metav1.TypeMeta{ Kind: "Service", @@ -363,7 +394,7 @@ func getV1alpha1Service() *servingv1alpha1.Service { }, }, } - return service + return service, template, container } func assertNoV1alpha1Old(t *testing.T, template *servingv1alpha1.RevisionTemplateSpec) { From 6ba099b98634549a0497f9bc70a0411042eb0556 Mon Sep 17 00:00:00 2001 From: Aaron Lerner Date: Wed, 7 Aug 2019 18:20:23 -0700 Subject: [PATCH 11/18] Add some test cases around weirder user input --- pkg/kn/commands/parsing_helper_test.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/pkg/kn/commands/parsing_helper_test.go b/pkg/kn/commands/parsing_helper_test.go index 57a97593b9..1533b3da5d 100644 --- a/pkg/kn/commands/parsing_helper_test.go +++ b/pkg/kn/commands/parsing_helper_test.go @@ -25,7 +25,8 @@ 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"}, ",", map[string]string{"only": "split,once"}) + testMapFromArray(t, []string{"only,split,once", "just,once,"}, ",", map[string]string{"only": "split,once", "just": "once,"}) + testMapFromArray(t, []string{"empty=", "="}, "=", map[string]string{"empty": "", "": ""}) } func testMapFromArray(t *testing.T, input []string, delimiter string, expected map[string]string) { @@ -37,7 +38,7 @@ func testMapFromArray(t *testing.T, input []string, delimiter string, expected m } func TestMapFromArrayNoDelimiter(t *testing.T) { - input := []string{"good=value", "badvalue"} + input := []string{"badvalue"} _, err := MapFromArray(input, "=") assert.ErrorContains(t, err, "Argument requires") } From da2029d5eac14d62ebb0ed2429b9c36875e71e87 Mon Sep 17 00:00:00 2001 From: Aaron Lerner Date: Wed, 7 Aug 2019 18:22:33 -0700 Subject: [PATCH 12/18] Change unset behavior and allow setting empty env and label --- pkg/kn/commands/parsing_helper.go | 15 ++++ pkg/kn/commands/parsing_helper_test.go | 17 ++++ .../service/configuration_edit_flags.go | 10 ++- .../commands/service/service_update_test.go | 9 +- pkg/serving/config_changes.go | 35 +++++--- pkg/serving/config_changes_test.go | 83 ++++++++++++++----- 6 files changed, 127 insertions(+), 42 deletions(-) diff --git a/pkg/kn/commands/parsing_helper.go b/pkg/kn/commands/parsing_helper.go index cf94982cca..1ea4dd55c7 100644 --- a/pkg/kn/commands/parsing_helper.go +++ b/pkg/kn/commands/parsing_helper.go @@ -32,3 +32,18 @@ func MapFromArray(arr []string, delimiter string) (map[string]string, error) { } return returnMap, nil } + +// SplitArrayBySuffix splits an array into items with a given suffix and those without. +// It removes the suffix from each item before returning. +func SplitArrayBySuffix(arr []string, suffix string) ([]string, []string) { + withoutSuffix := []string{} + withSuffix := []string{} + for _, elem := range arr { + if strings.HasSuffix(elem, suffix) { + withSuffix = append(withSuffix, elem[:len(elem)-len(suffix)]) + } else { + withoutSuffix = append(withoutSuffix, elem) + } + } + return withoutSuffix, withSuffix +} diff --git a/pkg/kn/commands/parsing_helper_test.go b/pkg/kn/commands/parsing_helper_test.go index 1533b3da5d..dca41ef946 100644 --- a/pkg/kn/commands/parsing_helper_test.go +++ b/pkg/kn/commands/parsing_helper_test.go @@ -48,3 +48,20 @@ func TestMapFromArrayEmptyValue(t *testing.T) { _, err := MapFromArray(input, "=") assert.ErrorContains(t, err, "Argument requires") } + +func TestSplitArrayBySuffix(t *testing.T) { + testSplitArrayBySuffix(t, []string{"without", "with-"}, "-", []string{"without"}, []string{"with"}) + testSplitArrayBySuffix(t, []string{"no", "suffix"}, "-", []string{"no", "suffix"}, []string{}) + testSplitArrayBySuffix(t, []string{"only()", "suffix()"}, "()", []string{}, []string{"only", "suffix"}) + testSplitArrayBySuffix(t, []string{"only. ", ".one..", "end..."}, ".", []string{"only. "}, []string{".one.", "end.."}) +} + +func testSplitArrayBySuffix(t *testing.T, input []string, suffix string, expectedWithoutSuffix []string, expectedWithSuffix []string) { + withoutSuffix, withSuffix := SplitArrayBySuffix(input, suffix) + if !reflect.DeepEqual(expectedWithoutSuffix, withoutSuffix) { + t.Fatalf("Without suffix did not match expected: %s\nFound: %s", expectedWithoutSuffix, withoutSuffix) + } + if !reflect.DeepEqual(expectedWithSuffix, withSuffix) { + t.Fatalf("With suffix did not match expected: %s\nFound: %s", expectedWithSuffix, withSuffix) + } +} diff --git a/pkg/kn/commands/service/configuration_edit_flags.go b/pkg/kn/commands/service/configuration_edit_flags.go index 60205ff6c4..be9f046d2c 100644 --- a/pkg/kn/commands/service/configuration_edit_flags.go +++ b/pkg/kn/commands/service/configuration_edit_flags.go @@ -73,11 +73,12 @@ func (p *ConfigurationEditFlags) Apply(service *servingv1alpha1.Service, cmd *co } if cmd.Flags().Changed("env") { - envMap, err := commands.MapFromArray(p.Env, "=") + envToUpdateOrSet, envToRemove := commands.SplitArrayBySuffix(p.Env, "-") + envMap, err := commands.MapFromArray(envToUpdateOrSet, "=") if err != nil { return errors.Wrap(err, "Invalid --env") } - err = servinglib.UpdateEnvVars(template, envMap) + err = servinglib.UpdateEnvVars(template, envMap, envToRemove) if err != nil { return err } @@ -138,11 +139,12 @@ func (p *ConfigurationEditFlags) Apply(service *servingv1alpha1.Service, cmd *co } if cmd.Flags().Changed("label") { - labelMap, err := commands.MapFromArray(p.Labels, "=") + labelsToUpdate, labelsToRemove := commands.SplitArrayBySuffix(p.Labels, "-") + labelsMap, err := commands.MapFromArray(labelsToUpdate, "=") if err != nil { return errors.Wrap(err, "Invalid --label") } - err = servinglib.UpdateLabels(service, template, labelMap) + err = servinglib.UpdateLabels(service, template, labelsMap, labelsToRemove) if err != nil { return err } diff --git a/pkg/kn/commands/service/service_update_test.go b/pkg/kn/commands/service/service_update_test.go index f21b2085cc..91c0789ed4 100644 --- a/pkg/kn/commands/service/service_update_test.go +++ b/pkg/kn/commands/service/service_update_test.go @@ -232,11 +232,14 @@ func TestServiceUpdateEnv(t *testing.T) { if err != nil { t.Fatal(err) } + template.Spec.DeprecatedContainer.Env = []corev1.EnvVar{ + {Name: "EXISTING", Value: "thing"}, + } servinglib.UpdateImage(template, "gcr.io/foo/bar:baz") action, updated, _, err := fakeServiceUpdate(orig, []string{ - "service", "update", "foo", "-e", "TARGET=Awesome", "--async"}, false) + "service", "update", "foo", "-e", "TARGET=Awesome", "--env", "EXISTING-", "--async"}, false) if err != nil { t.Fatal(err) @@ -401,7 +404,7 @@ func TestServiceUpdateLabelExisting(t *testing.T) { original.ObjectMeta.Labels = map[string]string{"already": "here", "tobe": "removed"} action, updated, _, err := fakeServiceUpdate(original, []string{ - "service", "update", "foo", "-l", "already=gone", "--label=tobe=", "--label", "b=cookie", "--async"}, false) + "service", "update", "foo", "-l", "already=gone", "--label=tobe-", "--label", "b=", "--async"}, false) if err != nil { t.Fatal(err) @@ -411,7 +414,7 @@ func TestServiceUpdateLabelExisting(t *testing.T) { expectedLabels := map[string]string{ "already": "gone", - "b": "cookie"} + "b": ""} actualLabels := updated.ObjectMeta.Labels if !reflect.DeepEqual(actualLabels, expectedLabels) { diff --git a/pkg/serving/config_changes.go b/pkg/serving/config_changes.go index db2ccfe2e7..6bfdd66475 100644 --- a/pkg/serving/config_changes.go +++ b/pkg/serving/config_changes.go @@ -28,12 +28,13 @@ import ( // UpdateEnvVars gives the configuration all the env var values listed in the given map of // vars. Does not touch any environment variables not mentioned, but it can add // new env vars and change the values of existing ones. -func UpdateEnvVars(template *servingv1alpha1.RevisionTemplateSpec, vars map[string]string) error { +func UpdateEnvVars(template *servingv1alpha1.RevisionTemplateSpec, toUpdate map[string]string, toRemove []string) error { container, err := ContainerOfRevisionTemplate(template) if err != nil { return err } - container.Env = updateEnvVarsFromMap(container.Env, vars) + envVars := updateEnvVarsFromMap(container.Env, toUpdate) + container.Env = removeEnvVars(envVars, toRemove) return nil } @@ -154,22 +155,20 @@ func UpdateResources(template *servingv1alpha1.RevisionTemplateSpec, requestsRes // UpdateLabels updates the labels identically on a service and template. // Does not overwrite the entire Labels field, only makes the requested updates -func UpdateLabels(service *servingv1alpha1.Service, template *servingv1alpha1.RevisionTemplateSpec, vars map[string]string) error { +func UpdateLabels(service *servingv1alpha1.Service, template *servingv1alpha1.RevisionTemplateSpec, toUpdate map[string]string, toRemove []string) error { if service.ObjectMeta.Labels == nil { service.ObjectMeta.Labels = make(map[string]string) } if template.ObjectMeta.Labels == nil { template.ObjectMeta.Labels = make(map[string]string) } - for key, value := range vars { - // Delete the label if passed an empty string, otherwise set or update it - if value == "" { - delete(service.ObjectMeta.Labels, key) - delete(template.ObjectMeta.Labels, key) - } else { - service.ObjectMeta.Labels[key] = value - template.ObjectMeta.Labels[key] = value - } + for key, value := range toUpdate { + service.ObjectMeta.Labels[key] = value + template.ObjectMeta.Labels[key] = value + } + for _, key := range toRemove { + delete(service.ObjectMeta.Labels, key) + delete(template.ObjectMeta.Labels, key) } return nil } @@ -198,3 +197,15 @@ func updateEnvVarsFromMap(env []corev1.EnvVar, vars map[string]string) []corev1. } return env } + +func removeEnvVars(env []corev1.EnvVar, toRemove []string) []corev1.EnvVar { + for _, name := range toRemove { + for i, envVar := range env { + if envVar.Name == name { + env = append(env[:i], env[i+1:]...) + break + } + } + } + return env +} diff --git a/pkg/serving/config_changes_test.go b/pkg/serving/config_changes_test.go index bb78fd6299..d3b822a2c3 100644 --- a/pkg/serving/config_changes_test.go +++ b/pkg/serving/config_changes_test.go @@ -82,7 +82,7 @@ func testUpdateEnvVarsNew(t *testing.T, template *servingv1alpha1.RevisionTempla "a": "foo", "b": "bar", } - err := UpdateEnvVars(template, env) + err := UpdateEnvVars(template, env, []string{}) assert.NilError(t, err) found, err := EnvToMap(container.Env) assert.NilError(t, err) @@ -108,7 +108,7 @@ func testUpdateEnvVarsAppendOld(t *testing.T, template *servingv1alpha1.Revision env := map[string]string{ "b": "bar", } - err := UpdateEnvVars(template, env) + err := UpdateEnvVars(template, env, []string{}) assert.NilError(t, err) expected := map[string]string{ @@ -139,7 +139,7 @@ func testUpdateEnvVarsModify(t *testing.T, revision *servingv1alpha1.RevisionTem env := map[string]string{ "a": "fancy", } - err := UpdateEnvVars(revision, env) + err := UpdateEnvVars(revision, env, []string{}) assert.NilError(t, err) expected := map[string]string{ @@ -153,6 +153,36 @@ func testUpdateEnvVarsModify(t *testing.T, revision *servingv1alpha1.RevisionTem } } +func TestUpdateEnvVarsRemove(t *testing.T) { + template, container := getV1alpha1RevisionTemplateWithOldFields() + testUpdateEnvVarsRemove(t, template, container) + assertNoV1alpha1(t, template) + + template, container = getV1alpha1Config() + testUpdateEnvVarsRemove(t, template, container) + assertNoV1alpha1Old(t, template) +} + +func testUpdateEnvVarsRemove(t *testing.T, revision *servingv1alpha1.RevisionTemplateSpec, container *corev1.Container) { + container.Env = []corev1.EnvVar{ + {Name: "a", Value: "foo"}, + {Name: "b", Value: "bar"}, + } + remove := []string{"b"} + err := UpdateEnvVars(revision, map[string]string{}, remove) + assert.NilError(t, err) + + expected := map[string]string{ + "a": "foo", + } + + found, err := EnvToMap(container.Env) + assert.NilError(t, err) + if !reflect.DeepEqual(expected, found) { + t.Fatalf("Env did not match expected %v, found %v", expected, found) + } +} + func TestUpdateMinScale(t *testing.T) { template, _ := getV1alpha1RevisionTemplateWithOldFields() err := UpdateMinScale(template, 10) @@ -239,23 +269,26 @@ func checkPortUpdate(t *testing.T, template *servingv1alpha1.RevisionTemplateSpe func TestUpdateEnvVarsBoth(t *testing.T) { template, container := getV1alpha1RevisionTemplateWithOldFields() - testUpdateEnvVarsBoth(t, template, container) + testUpdateEnvVarsAll(t, template, container) assertNoV1alpha1(t, template) template, container = getV1alpha1Config() - testUpdateEnvVarsBoth(t, template, container) + testUpdateEnvVarsAll(t, template, container) assertNoV1alpha1Old(t, template) } -func testUpdateEnvVarsBoth(t *testing.T, template *servingv1alpha1.RevisionTemplateSpec, container *corev1.Container) { +func testUpdateEnvVarsAll(t *testing.T, template *servingv1alpha1.RevisionTemplateSpec, container *corev1.Container) { container.Env = []corev1.EnvVar{ {Name: "a", Value: "foo"}, - {Name: "c", Value: "caroline"}} + {Name: "c", Value: "caroline"}, + {Name: "d", Value: "byebye"}, + } env := map[string]string{ "a": "fancy", "b": "boo", } - err := UpdateEnvVars(template, env) + remove := []string{"d"} + err := UpdateEnvVars(template, env, remove) assert.NilError(t, err) expected := map[string]string{ @@ -278,7 +311,7 @@ func TestUpdateLabelsNew(t *testing.T) { "a": "foo", "b": "bar", } - err := UpdateLabels(service, template, labels) + err := UpdateLabels(service, template, labels, []string{}) assert.NilError(t, err) actual := service.ObjectMeta.Labels @@ -294,24 +327,31 @@ func TestUpdateLabelsNew(t *testing.T) { func TestUpdateLabelsExisting(t *testing.T) { service, template, _ := getV1alpha1Service() - service.ObjectMeta.Labels = map[string]string{"a": "foo"} - template.ObjectMeta.Labels = map[string]string{"a": "foo"} + service.ObjectMeta.Labels = map[string]string{"a": "foo", "b": "bar"} + template.ObjectMeta.Labels = map[string]string{"a": "foo", "b": "bar"} labels := map[string]string{ - "a": "notfood", - "b": "bar", + "a": "notfoo", + "c": "bat", + "d": "", } - err := UpdateLabels(service, template, labels) + err := UpdateLabels(service, template, labels, []string{}) assert.NilError(t, err) + expected := map[string]string{ + "a": "notfoo", + "b": "bar", + "c": "bat", + "d": "", + } actual := service.ObjectMeta.Labels - if !reflect.DeepEqual(labels, actual) { - t.Fatalf("Labels did not match expected %v found %v", labels, actual) + if !reflect.DeepEqual(expected, actual) { + t.Fatalf("Labels did not match expected %v found %v", expected, actual) } actual = template.ObjectMeta.Labels - if !reflect.DeepEqual(labels, actual) { - t.Fatalf("Template labels did not match expected %v found %v", labels, actual) + if !reflect.DeepEqual(expected, actual) { + t.Fatalf("Template labels did not match expected %v found %v", expected, actual) } } @@ -320,11 +360,8 @@ func TestUpdateLabelsRemoveExisting(t *testing.T) { service.ObjectMeta.Labels = map[string]string{"a": "foo", "b": "bar"} template.ObjectMeta.Labels = map[string]string{"a": "foo", "b": "bar"} - labels := map[string]string{ - "a": "foo", - "b": "", - } - err := UpdateLabels(service, template, labels) + remove := []string{"b"} + err := UpdateLabels(service, template, map[string]string{}, remove) assert.NilError(t, err) expected := map[string]string{ "a": "foo", From 202f086760726bfa7ddbe271cfdaa84856c241fc Mon Sep 17 00:00:00 2001 From: Aaron Lerner Date: Wed, 7 Aug 2019 18:26:15 -0700 Subject: [PATCH 13/18] Update docs for new unset behavior --- docs/cmd/kn_service_create.md | 4 ++-- docs/cmd/kn_service_update.md | 4 ++-- pkg/kn/commands/service/configuration_edit_flags.go | 8 ++++++-- 3 files changed, 10 insertions(+), 6 deletions(-) diff --git a/docs/cmd/kn_service_create.md b/docs/cmd/kn_service_create.md index b51d103252..ed4ac72fe0 100644 --- a/docs/cmd/kn_service_create.md +++ b/docs/cmd/kn_service_create.md @@ -42,11 +42,11 @@ kn service create NAME --image IMAGE [flags] --async Create service and don't wait for it to become ready. --concurrency-limit int Hard Limit of concurrent requests to be processed by a single replica. --concurrency-target int Recommendation for when to scale up based on the concurrent number of incoming request. Defaults to --concurrency-limit when given. - -e, --env stringArray Environment variable to set. NAME=value; you may provide this flag any number of times to set multiple environment variables. + -e, --env stringArray Environment variable to set. NAME=value; you may provide this flag any number of times to set multiple environment variables. To unset, specify the environment variable name followed by a "-" (e.g., NAME-). --force Create service forcefully, replaces existing service if any. -h, --help help for create --image string Image to run. - -l, --label stringArray Service label to set. NAME=value; provide an empty string for the value to remove a label. You may provide this flag any number of times to set multiple labels. + -l, --label stringArray Service label to set. name=value; you may provide this flag any number of times to set multiple labels. To unset, specify the label name followed by a "-" (e.g., name-). --limits-cpu string The limits on the requested CPU (e.g., 1000m). --limits-memory string The limits on the requested memory (e.g., 1024Mi). --max-scale int Maximal number of replicas. diff --git a/docs/cmd/kn_service_update.md b/docs/cmd/kn_service_update.md index 7ebdc5ad07..b82f2ee06b 100644 --- a/docs/cmd/kn_service_update.md +++ b/docs/cmd/kn_service_update.md @@ -30,10 +30,10 @@ kn service update NAME [flags] --async Update service and don't wait for it to become ready. --concurrency-limit int Hard Limit of concurrent requests to be processed by a single replica. --concurrency-target int Recommendation for when to scale up based on the concurrent number of incoming request. Defaults to --concurrency-limit when given. - -e, --env stringArray Environment variable to set. NAME=value; you may provide this flag any number of times to set multiple environment variables. + -e, --env stringArray Environment variable to set. NAME=value; you may provide this flag any number of times to set multiple environment variables. To unset, specify the environment variable name followed by a "-" (e.g., NAME-). -h, --help help for update --image string Image to run. - -l, --label stringArray Service label to set. NAME=value; provide an empty string for the value to remove a label. You may provide this flag any number of times to set multiple labels. + -l, --label stringArray Service label to set. name=value; you may provide this flag any number of times to set multiple labels. To unset, specify the label name followed by a "-" (e.g., name-). --limits-cpu string The limits on the requested CPU (e.g., 1000m). --limits-memory string The limits on the requested memory (e.g., 1024Mi). --max-scale int Maximal number of replicas. diff --git a/pkg/kn/commands/service/configuration_edit_flags.go b/pkg/kn/commands/service/configuration_edit_flags.go index be9f046d2c..7fcc772d90 100644 --- a/pkg/kn/commands/service/configuration_edit_flags.go +++ b/pkg/kn/commands/service/configuration_edit_flags.go @@ -46,7 +46,8 @@ func (p *ConfigurationEditFlags) AddUpdateFlags(command *cobra.Command) { command.Flags().StringVar(&p.Image, "image", "", "Image to run.") command.Flags().StringArrayVarP(&p.Env, "env", "e", []string{}, "Environment variable to set. NAME=value; you may provide this flag "+ - "any number of times to set multiple environment variables.") + "any number of times to set multiple environment variables. "+ + "To unset, specify the environment variable name followed by a \"-\" (e.g., NAME-).") command.Flags().StringVar(&p.RequestsFlags.CPU, "requests-cpu", "", "The requested CPU (e.g., 250m).") command.Flags().StringVar(&p.RequestsFlags.Memory, "requests-memory", "", "The requested memory (e.g., 64Mi).") command.Flags().StringVar(&p.LimitsFlags.CPU, "limits-cpu", "", "The limits on the requested CPU (e.g., 1000m).") @@ -56,7 +57,10 @@ func (p *ConfigurationEditFlags) AddUpdateFlags(command *cobra.Command) { command.Flags().IntVar(&p.ConcurrencyTarget, "concurrency-target", 0, "Recommendation for when to scale up based on the concurrent number of incoming request. Defaults to --concurrency-limit when given.") command.Flags().IntVar(&p.ConcurrencyLimit, "concurrency-limit", 0, "Hard Limit of concurrent requests to be processed by a single replica.") command.Flags().Int32VarP(&p.Port, "port", "p", 0, "The port where application listens on.") - command.Flags().StringArrayVarP(&p.Labels, "label", "l", []string{}, "Service label to set. NAME=value; provide an empty string for the value to remove a label. You may provide this flag any number of times to set multiple labels.") + command.Flags().StringArrayVarP(&p.Labels, "label", "l", []string{}, + "Service label to set. name=value; you may provide this flag "+ + "any number of times to set multiple labels. "+ + "To unset, specify the label name followed by a \"-\" (e.g., name-).") } func (p *ConfigurationEditFlags) AddCreateFlags(command *cobra.Command) { From 3f7d0caf5b3b9c5c5fc336ea7326a71e32dda0ab Mon Sep 17 00:00:00 2001 From: Aaron Lerner Date: Thu, 8 Aug 2019 14:20:51 -0700 Subject: [PATCH 14/18] Make single keys to map to empty values --- pkg/kn/commands/parsing_helper.go | 37 +++++++++--------- pkg/kn/commands/parsing_helper_test.go | 39 ++++++++----------- .../service/configuration_edit_flags.go | 22 +++++++++-- .../commands/service/service_create_test.go | 16 +++++--- .../commands/service/service_update_test.go | 11 ++++-- 5 files changed, 70 insertions(+), 55 deletions(-) diff --git a/pkg/kn/commands/parsing_helper.go b/pkg/kn/commands/parsing_helper.go index 1ea4dd55c7..61727414d1 100644 --- a/pkg/kn/commands/parsing_helper.go +++ b/pkg/kn/commands/parsing_helper.go @@ -19,31 +19,30 @@ import ( "strings" ) -// 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 respsective values. +func MapFromArrayAllowingSingles(arr []string, delimiter string) (map[string]string, error) { + return mapFromArray(arr, delimiter, true) +} + func MapFromArray(arr []string, delimiter string) (map[string]string, error) { + return mapFromArray(arr, delimiter, false) +} + +// 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 respsective values. +// If allowSingles is true, values without a delimiter will be added as keys pointing to empty strings +func mapFromArray(arr []string, delimiter string, allowSingles bool) (map[string]string, error) { returnMap := map[string]string{} for _, pairStr := range arr { pairSlice := strings.SplitN(pairStr, delimiter, 2) if len(pairSlice) <= 1 { - return nil, fmt.Errorf("Argument requires a value that contains the %q character; got %q", delimiter, pairStr) - } - returnMap[pairSlice[0]] = pairSlice[1] - } - return returnMap, nil -} - -// SplitArrayBySuffix splits an array into items with a given suffix and those without. -// It removes the suffix from each item before returning. -func SplitArrayBySuffix(arr []string, suffix string) ([]string, []string) { - withoutSuffix := []string{} - withSuffix := []string{} - for _, elem := range arr { - if strings.HasSuffix(elem, suffix) { - withSuffix = append(withSuffix, elem[:len(elem)-len(suffix)]) + if len(pairSlice) == 0 || !allowSingles { + return nil, fmt.Errorf("Argument requires a value that contains the %q character; got %q", delimiter, pairStr) + } else { + returnMap[pairSlice[0]] = "" + } } else { - withoutSuffix = append(withoutSuffix, elem) + returnMap[pairSlice[0]] = pairSlice[1] } } - return withoutSuffix, withSuffix + return returnMap, nil } diff --git a/pkg/kn/commands/parsing_helper_test.go b/pkg/kn/commands/parsing_helper_test.go index dca41ef946..398233c10e 100644 --- a/pkg/kn/commands/parsing_helper_test.go +++ b/pkg/kn/commands/parsing_helper_test.go @@ -15,7 +15,6 @@ package commands import ( - "reflect" "testing" "gotest.tools/assert" @@ -32,36 +31,32 @@ func TestMapFromArray(t *testing.T) { func testMapFromArray(t *testing.T, input []string, delimiter string, expected map[string]string) { actual, err := MapFromArray(input, delimiter) assert.NilError(t, err) - if !reflect.DeepEqual(expected, actual) { - t.Fatalf("Map did not match expected: %s\nFound: %s", expected, actual) - } + assert.DeepEqual(t, expected, actual) } func TestMapFromArrayNoDelimiter(t *testing.T) { input := []string{"badvalue"} - _, err := MapFromArray(input, "=") + _, err := MapFromArray(input, "+") assert.ErrorContains(t, err, "Argument requires") + assert.ErrorContains(t, err, "+") } -func TestMapFromArrayEmptyValue(t *testing.T) { - input := []string{""} - _, err := MapFromArray(input, "=") - assert.ErrorContains(t, err, "Argument requires") +func TestMapFromArrayNoDelimiterAllowingSingles(t *testing.T) { + input := []string{"okvalue"} + actual, err := MapFromArrayAllowingSingles(input, "+") + expected := map[string]string{"okvalue": ""} + assert.NilError(t, err) + assert.DeepEqual(t, expected, actual) } -func TestSplitArrayBySuffix(t *testing.T) { - testSplitArrayBySuffix(t, []string{"without", "with-"}, "-", []string{"without"}, []string{"with"}) - testSplitArrayBySuffix(t, []string{"no", "suffix"}, "-", []string{"no", "suffix"}, []string{}) - testSplitArrayBySuffix(t, []string{"only()", "suffix()"}, "()", []string{}, []string{"only", "suffix"}) - testSplitArrayBySuffix(t, []string{"only. ", ".one..", "end..."}, ".", []string{"only. "}, []string{".one.", "end.."}) +func TestMapFromArrayEmptyValueEmptyDelimiter(t *testing.T) { + input := []string{""} + _, err := MapFromArray(input, "") + assert.ErrorContains(t, err, "Argument requires") } -func testSplitArrayBySuffix(t *testing.T, input []string, suffix string, expectedWithoutSuffix []string, expectedWithSuffix []string) { - withoutSuffix, withSuffix := SplitArrayBySuffix(input, suffix) - if !reflect.DeepEqual(expectedWithoutSuffix, withoutSuffix) { - t.Fatalf("Without suffix did not match expected: %s\nFound: %s", expectedWithoutSuffix, withoutSuffix) - } - if !reflect.DeepEqual(expectedWithSuffix, withSuffix) { - t.Fatalf("With suffix did not match expected: %s\nFound: %s", expectedWithSuffix, withSuffix) - } +func TestMapFromArrayEmptyValueEmptyDelimiterAllowingSingles(t *testing.T) { + input := []string{""} + _, err := MapFromArrayAllowingSingles(input, "") + assert.ErrorContains(t, err, "Argument requires") } diff --git a/pkg/kn/commands/service/configuration_edit_flags.go b/pkg/kn/commands/service/configuration_edit_flags.go index 7fcc772d90..1803e020d7 100644 --- a/pkg/kn/commands/service/configuration_edit_flags.go +++ b/pkg/kn/commands/service/configuration_edit_flags.go @@ -15,6 +15,8 @@ package service import ( + "strings" + commands "github.com/knative/client/pkg/kn/commands" servinglib "github.com/knative/client/pkg/serving" servingv1alpha1 "github.com/knative/serving/pkg/apis/serving/v1alpha1" @@ -77,11 +79,17 @@ func (p *ConfigurationEditFlags) Apply(service *servingv1alpha1.Service, cmd *co } if cmd.Flags().Changed("env") { - envToUpdateOrSet, envToRemove := commands.SplitArrayBySuffix(p.Env, "-") - envMap, err := commands.MapFromArray(envToUpdateOrSet, "=") + envMap, err := commands.MapFromArrayAllowingSingles(p.Env, "=") if err != nil { return errors.Wrap(err, "Invalid --env") } + envToRemove := []string{} + for name := range envMap { + if strings.HasSuffix(name, "-") { + envToRemove = append(envToRemove, name[:len(name)-1]) + delete(envMap, name) + } + } err = servinglib.UpdateEnvVars(template, envMap, envToRemove) if err != nil { return err @@ -143,11 +151,17 @@ func (p *ConfigurationEditFlags) Apply(service *servingv1alpha1.Service, cmd *co } if cmd.Flags().Changed("label") { - labelsToUpdate, labelsToRemove := commands.SplitArrayBySuffix(p.Labels, "-") - labelsMap, err := commands.MapFromArray(labelsToUpdate, "=") + labelsMap, err := commands.MapFromArrayAllowingSingles(p.Labels, "=") 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) + } + } err = servinglib.UpdateLabels(service, template, labelsMap, labelsToRemove) if err != nil { return err diff --git a/pkg/kn/commands/service/service_create_test.go b/pkg/kn/commands/service/service_create_test.go index 004d7aac76..56e2d476d0 100644 --- a/pkg/kn/commands/service/service_create_test.go +++ b/pkg/kn/commands/service/service_create_test.go @@ -158,7 +158,7 @@ func TestServiceCreateImageSync(t *testing.T) { func TestServiceCreateEnv(t *testing.T) { action, created, _, err := fakeServiceCreate([]string{ - "service", "create", "foo", "--image", "gcr.io/foo/bar:baz", "-e", "A=DOGS", "--env", "B=WOLVES", "--async"}, false, false) + "service", "create", "foo", "--image", "gcr.io/foo/bar:baz", "-e", "A=DOGS", "--env", "B=WOLVES", "--env=EMPTY", "--async"}, false, false) if err != nil { t.Fatal(err) @@ -167,8 +167,10 @@ func TestServiceCreateEnv(t *testing.T) { } expectedEnvVars := map[string]string{ - "A": "DOGS", - "B": "WOLVES"} + "A": "DOGS", + "B": "WOLVES", + "EMPTY": "", + } template, err := servinglib.RevisionTemplateOfService(created) actualEnvVars, err := servinglib.EnvToMap(template.Spec.DeprecatedContainer.Env) @@ -470,7 +472,7 @@ func TestServiceCreateEnvForce(t *testing.T) { func TestServiceCreateLabel(t *testing.T) { action, created, _, err := fakeServiceCreate([]string{ - "service", "create", "foo", "--image", "gcr.io/foo/bar:baz", "-l", "a=mouse", "--label", "b=cookie", "--async"}, false, false) + "service", "create", "foo", "--image", "gcr.io/foo/bar:baz", "-l", "a=mouse", "--label", "b=cookie", "--label=empty", "--async"}, false, false) if err != nil { t.Fatal(err) @@ -479,8 +481,10 @@ func TestServiceCreateLabel(t *testing.T) { } expectedLabels := map[string]string{ - "a": "mouse", - "b": "cookie"} + "a": "mouse", + "b": "cookie", + "empty": "", + } actualLabels := created.ObjectMeta.Labels template, err := servinglib.RevisionTemplateOfService(created) diff --git a/pkg/kn/commands/service/service_update_test.go b/pkg/kn/commands/service/service_update_test.go index 91c0789ed4..42d23b4113 100644 --- a/pkg/kn/commands/service/service_update_test.go +++ b/pkg/kn/commands/service/service_update_test.go @@ -234,12 +234,13 @@ func TestServiceUpdateEnv(t *testing.T) { } template.Spec.DeprecatedContainer.Env = []corev1.EnvVar{ {Name: "EXISTING", Value: "thing"}, + {Name: "OTHEREXISTING"}, } servinglib.UpdateImage(template, "gcr.io/foo/bar:baz") action, updated, _, err := fakeServiceUpdate(orig, []string{ - "service", "update", "foo", "-e", "TARGET=Awesome", "--env", "EXISTING-", "--async"}, false) + "service", "update", "foo", "-e", "TARGET=Awesome", "--env", "EXISTING-", "--env=OTHEREXISTING-=whatever", "--async"}, false) if err != nil { t.Fatal(err) @@ -381,7 +382,7 @@ func TestServiceUpdateLabelWhenEmpty(t *testing.T) { original := newEmptyService() action, updated, _, err := fakeServiceUpdate(original, []string{ - "service", "update", "foo", "-l", "a=mouse", "--label", "b=cookie", "--async"}, false) + "service", "update", "foo", "-l", "a=mouse", "--label", "b=cookie", "-l=single", "--async"}, false) if err != nil { t.Fatal(err) @@ -390,8 +391,10 @@ func TestServiceUpdateLabelWhenEmpty(t *testing.T) { } expectedLabels := map[string]string{ - "a": "mouse", - "b": "cookie"} + "a": "mouse", + "b": "cookie", + "single": "", + } actualLabels := updated.ObjectMeta.Labels if !reflect.DeepEqual(actualLabels, expectedLabels) { From 29dc7d5a42937043412122ad57ffb2df692bd824 Mon Sep 17 00:00:00 2001 From: Aaron Lerner Date: Thu, 8 Aug 2019 14:24:53 -0700 Subject: [PATCH 15/18] Move helper to util --- pkg/kn/commands/service/configuration_edit_flags.go | 6 +++--- pkg/{kn/commands => util}/parsing_helper.go | 2 +- pkg/{kn/commands => util}/parsing_helper_test.go | 2 +- 3 files changed, 5 insertions(+), 5 deletions(-) rename pkg/{kn/commands => util}/parsing_helper.go (99%) rename pkg/{kn/commands => util}/parsing_helper_test.go (99%) diff --git a/pkg/kn/commands/service/configuration_edit_flags.go b/pkg/kn/commands/service/configuration_edit_flags.go index 1803e020d7..aefab59823 100644 --- a/pkg/kn/commands/service/configuration_edit_flags.go +++ b/pkg/kn/commands/service/configuration_edit_flags.go @@ -17,8 +17,8 @@ package service import ( "strings" - commands "github.com/knative/client/pkg/kn/commands" servinglib "github.com/knative/client/pkg/serving" + util "github.com/knative/client/pkg/util" servingv1alpha1 "github.com/knative/serving/pkg/apis/serving/v1alpha1" errors "github.com/pkg/errors" "github.com/spf13/cobra" @@ -79,7 +79,7 @@ func (p *ConfigurationEditFlags) Apply(service *servingv1alpha1.Service, cmd *co } if cmd.Flags().Changed("env") { - envMap, err := commands.MapFromArrayAllowingSingles(p.Env, "=") + envMap, err := util.MapFromArrayAllowingSingles(p.Env, "=") if err != nil { return errors.Wrap(err, "Invalid --env") } @@ -151,7 +151,7 @@ func (p *ConfigurationEditFlags) Apply(service *servingv1alpha1.Service, cmd *co } if cmd.Flags().Changed("label") { - labelsMap, err := commands.MapFromArrayAllowingSingles(p.Labels, "=") + labelsMap, err := util.MapFromArrayAllowingSingles(p.Labels, "=") if err != nil { return errors.Wrap(err, "Invalid --label") } diff --git a/pkg/kn/commands/parsing_helper.go b/pkg/util/parsing_helper.go similarity index 99% rename from pkg/kn/commands/parsing_helper.go rename to pkg/util/parsing_helper.go index 61727414d1..0deff0994b 100644 --- a/pkg/kn/commands/parsing_helper.go +++ b/pkg/util/parsing_helper.go @@ -12,7 +12,7 @@ // See the License for the specific language governing permissions and // limitations under the License. -package commands +package util import ( "fmt" diff --git a/pkg/kn/commands/parsing_helper_test.go b/pkg/util/parsing_helper_test.go similarity index 99% rename from pkg/kn/commands/parsing_helper_test.go rename to pkg/util/parsing_helper_test.go index 398233c10e..aa1c9e94b1 100644 --- a/pkg/kn/commands/parsing_helper_test.go +++ b/pkg/util/parsing_helper_test.go @@ -12,7 +12,7 @@ // See the License for the specific language governing permissions and // limitations under the License. -package commands +package util import ( "testing" From a962ae03874fc257ce6b8487f3c3dddaec8673d9 Mon Sep 17 00:00:00 2001 From: Aaron Lerner Date: Thu, 8 Aug 2019 15:57:07 -0700 Subject: [PATCH 16/18] Use assert.DeepEqual --- pkg/serving/config_changes_test.go | 32 ++++++++---------------------- 1 file changed, 8 insertions(+), 24 deletions(-) diff --git a/pkg/serving/config_changes_test.go b/pkg/serving/config_changes_test.go index d3b822a2c3..a3a3f9c393 100644 --- a/pkg/serving/config_changes_test.go +++ b/pkg/serving/config_changes_test.go @@ -86,9 +86,7 @@ func testUpdateEnvVarsNew(t *testing.T, template *servingv1alpha1.RevisionTempla assert.NilError(t, err) found, err := EnvToMap(container.Env) assert.NilError(t, err) - if !reflect.DeepEqual(env, found) { - t.Fatalf("Env did not match expected %v found %v", env, found) - } + assert.DeepEqual(t, env, found) } func TestUpdateEnvVarsAppendOld(t *testing.T) { @@ -118,9 +116,7 @@ func testUpdateEnvVarsAppendOld(t *testing.T, template *servingv1alpha1.Revision found, err := EnvToMap(container.Env) assert.NilError(t, err) - if !reflect.DeepEqual(expected, found) { - t.Fatalf("Env did not match expected %v, found %v", env, found) - } + assert.DeepEqual(t, expected, found) } func TestUpdateEnvVarsModify(t *testing.T) { @@ -148,9 +144,7 @@ func testUpdateEnvVarsModify(t *testing.T, revision *servingv1alpha1.RevisionTem found, err := EnvToMap(container.Env) assert.NilError(t, err) - if !reflect.DeepEqual(expected, found) { - t.Fatalf("Env did not match expected %v, found %v", env, found) - } + assert.DeepEqual(t, expected, found) } func TestUpdateEnvVarsRemove(t *testing.T) { @@ -178,9 +172,7 @@ func testUpdateEnvVarsRemove(t *testing.T, revision *servingv1alpha1.RevisionTem found, err := EnvToMap(container.Env) assert.NilError(t, err) - if !reflect.DeepEqual(expected, found) { - t.Fatalf("Env did not match expected %v, found %v", expected, found) - } + assert.DeepEqual(t, expected, found) } func TestUpdateMinScale(t *testing.T) { @@ -345,14 +337,10 @@ func TestUpdateLabelsExisting(t *testing.T) { } actual := service.ObjectMeta.Labels - if !reflect.DeepEqual(expected, actual) { - t.Fatalf("Labels did not match expected %v found %v", expected, actual) - } + assert.DeepEqual(t, expected, actual) actual = template.ObjectMeta.Labels - if !reflect.DeepEqual(expected, actual) { - t.Fatalf("Template labels did not match expected %v found %v", expected, actual) - } + assert.DeepEqual(t, expected, actual) } func TestUpdateLabelsRemoveExisting(t *testing.T) { @@ -368,14 +356,10 @@ func TestUpdateLabelsRemoveExisting(t *testing.T) { } actual := service.ObjectMeta.Labels - if !reflect.DeepEqual(expected, actual) { - t.Fatalf("Labels did not match expected %v found %v", expected, actual) - } + assert.DeepEqual(t, expected, actual) actual = template.ObjectMeta.Labels - if !reflect.DeepEqual(expected, actual) { - t.Fatalf("Template labels did not match expected %v found %v", expected, actual) - } + assert.DeepEqual(t, expected, actual) } // ========================================================================================================= From a80f54f4fa24ff56d784787710bde68e56f3a0bb Mon Sep 17 00:00:00 2001 From: Aaron Lerner Date: Fri, 9 Aug 2019 12:34:29 -0700 Subject: [PATCH 17/18] Use new mock test + check both service and revision --- .../service/service_create_mock_test.go | 55 +++++++++++++++++++ .../commands/service/service_create_test.go | 29 ---------- .../commands/service/service_update_test.go | 29 ++++++---- 3 files changed, 73 insertions(+), 40 deletions(-) diff --git a/pkg/kn/commands/service/service_create_mock_test.go b/pkg/kn/commands/service/service_create_mock_test.go index d9da9eacf1..9f6f9ce766 100644 --- a/pkg/kn/commands/service/service_create_mock_test.go +++ b/pkg/kn/commands/service/service_create_mock_test.go @@ -20,11 +20,14 @@ import ( "github.com/knative/pkg/apis" "gotest.tools/assert" + corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "github.com/knative/serving/pkg/apis/serving/v1alpha1" "github.com/knative/client/pkg/kn/commands" + servinglib "github.com/knative/client/pkg/serving" knclient "github.com/knative/client/pkg/serving/v1alpha1" "github.com/knative/client/pkg/util" @@ -55,6 +58,58 @@ func TestServiceCreateImageMock(t *testing.T) { r.Validate() } +func TestServiceCreateLabel(t *testing.T) { + client := knclient.NewMockKnClient(t) + + r := client.Recorder() + r.GetService("foo", nil, errors.NewNotFound(v1alpha1.Resource("service"), "foo")) + + service := getService("foo") + expected := map[string]string{ + "a": "mouse", + "b": "cookie", + "empty": "", + } + service.ObjectMeta.Labels = expected + template, err := servinglib.RevisionTemplateOfService(service) + assert.NilError(t, err) + template.ObjectMeta.Labels = expected + template.Spec.DeprecatedContainer.Image = "gcr.io/foo/bar:baz" + r.CreateService(service, nil) + + output, err := executeCommand(client, "create", "foo", "--image", "gcr.io/foo/bar:baz", "-l", "a=mouse", "--label", "b=cookie", "--label=empty", "--async") + assert.NilError(t, err) + assert.Assert(t, util.ContainsAll(output, "created", "foo", "default")) + + r.Validate() +} + +func getService(name string) *v1alpha1.Service { + service := &v1alpha1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: name, + Namespace: "default", + }, + Spec: v1alpha1.ServiceSpec{ + DeprecatedRunLatest: &v1alpha1.RunLatestType{ + Configuration: v1alpha1.ConfigurationSpec{ + DeprecatedRevisionTemplate: &v1alpha1.RevisionTemplateSpec{ + Spec: v1alpha1.RevisionSpec{ + DeprecatedContainer: &corev1.Container{ + Resources: corev1.ResourceRequirements{ + Limits: corev1.ResourceList{}, + Requests: corev1.ResourceList{}, + }, + }, + }, + }, + }, + }, + }, + } + return service +} + func getServiceWithUrl(name string, urlName string) *v1alpha1.Service { service := v1alpha1.Service{} url, _ := apis.ParseURL(urlName) diff --git a/pkg/kn/commands/service/service_create_test.go b/pkg/kn/commands/service/service_create_test.go index 56e2d476d0..efab0580a5 100644 --- a/pkg/kn/commands/service/service_create_test.go +++ b/pkg/kn/commands/service/service_create_test.go @@ -469,32 +469,3 @@ func TestServiceCreateEnvForce(t *testing.T) { t.Fatalf("wrong output: %s", output) } } - -func TestServiceCreateLabel(t *testing.T) { - action, created, _, err := fakeServiceCreate([]string{ - "service", "create", "foo", "--image", "gcr.io/foo/bar:baz", "-l", "a=mouse", "--label", "b=cookie", "--label=empty", "--async"}, false, false) - - if err != nil { - t.Fatal(err) - } else if !action.Matches("create", "services") { - t.Fatalf("Bad action %v", action) - } - - expectedLabels := map[string]string{ - "a": "mouse", - "b": "cookie", - "empty": "", - } - actualLabels := created.ObjectMeta.Labels - - template, err := servinglib.RevisionTemplateOfService(created) - if err != nil { - t.Fatal(err) - } else if template.Spec.DeprecatedContainer.Image != "gcr.io/foo/bar:baz" { - t.Fatalf("wrong image set: %v", template.Spec.DeprecatedContainer.Image) - } else if !reflect.DeepEqual( - actualLabels, - expectedLabels) { - t.Fatalf("wrong labels %v", created.ObjectMeta.Labels) - } -} diff --git a/pkg/kn/commands/service/service_update_test.go b/pkg/kn/commands/service/service_update_test.go index 42d23b4113..aa3cb234e4 100644 --- a/pkg/kn/commands/service/service_update_test.go +++ b/pkg/kn/commands/service/service_update_test.go @@ -390,21 +390,25 @@ func TestServiceUpdateLabelWhenEmpty(t *testing.T) { t.Fatalf("Bad action %v", action) } - expectedLabels := map[string]string{ + expected := map[string]string{ "a": "mouse", "b": "cookie", "single": "", } - actualLabels := updated.ObjectMeta.Labels + actual := updated.ObjectMeta.Labels + assert.DeepEqual(t, expected, actual) - if !reflect.DeepEqual(actualLabels, expectedLabels) { - t.Fatalf("wrong labels %v", actualLabels) - } + template, err := servinglib.RevisionTemplateOfService(updated) + assert.NilError(t, err) + actual = template.ObjectMeta.Labels + assert.DeepEqual(t, expected, actual) } func TestServiceUpdateLabelExisting(t *testing.T) { original := newEmptyService() original.ObjectMeta.Labels = map[string]string{"already": "here", "tobe": "removed"} + originalTemplate, _ := servinglib.RevisionTemplateOfService(original) + originalTemplate.ObjectMeta.Labels = map[string]string{"already": "here", "tobe": "removed"} action, updated, _, err := fakeServiceUpdate(original, []string{ "service", "update", "foo", "-l", "already=gone", "--label=tobe-", "--label", "b=", "--async"}, false) @@ -415,14 +419,17 @@ func TestServiceUpdateLabelExisting(t *testing.T) { t.Fatalf("Bad action %v", action) } - expectedLabels := map[string]string{ + expected := map[string]string{ "already": "gone", - "b": ""} - actualLabels := updated.ObjectMeta.Labels - - if !reflect.DeepEqual(actualLabels, expectedLabels) { - t.Fatalf("wrong labels %v", actualLabels) + "b": "", } + actual := updated.ObjectMeta.Labels + assert.DeepEqual(t, expected, actual) + + template, err := servinglib.RevisionTemplateOfService(updated) + assert.NilError(t, err) + actual = template.ObjectMeta.Labels + assert.DeepEqual(t, expected, actual) } func newEmptyService() *v1alpha1.Service { From dc17266ad75519e9c19d7fa0a2a7ea65e5981723 Mon Sep 17 00:00:00 2001 From: Aaron Lerner Date: Fri, 9 Aug 2019 13:10:14 -0700 Subject: [PATCH 18/18] Use new method and correct year --- pkg/kn/commands/service/service_create_mock_test.go | 2 +- pkg/util/parsing_helper_test.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/kn/commands/service/service_create_mock_test.go b/pkg/kn/commands/service/service_create_mock_test.go index 361b5dd654..f453656781 100644 --- a/pkg/kn/commands/service/service_create_mock_test.go +++ b/pkg/kn/commands/service/service_create_mock_test.go @@ -75,7 +75,7 @@ func TestServiceCreateLabel(t *testing.T) { template.Spec.DeprecatedContainer.Image = "gcr.io/foo/bar:baz" r.CreateService(service, nil) - output, err := executeCommand(client, "create", "foo", "--image", "gcr.io/foo/bar:baz", "-l", "a=mouse", "--label", "b=cookie", "--label=empty", "--async") + output, err := executeServiceCommand(client, "create", "foo", "--image", "gcr.io/foo/bar:baz", "-l", "a=mouse", "--label", "b=cookie", "--label=empty", "--async") assert.NilError(t, err) assert.Assert(t, util.ContainsAll(output, "created", "foo", "default")) diff --git a/pkg/util/parsing_helper_test.go b/pkg/util/parsing_helper_test.go index aa1c9e94b1..686c18015e 100644 --- a/pkg/util/parsing_helper_test.go +++ b/pkg/util/parsing_helper_test.go @@ -1,4 +1,4 @@ -// Copyright © 2018 The Knative Authors +// Copyright © 2019 The Knative Authors // // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License.