From 9b52fc83451c3944d3e46a5a63873b00f1db4a1b Mon Sep 17 00:00:00 2001 From: Tsubasa Nagasawa Date: Thu, 12 Sep 2019 17:12:32 +0900 Subject: [PATCH] Update EnvVars in alphabetical order of Env key name (#389) --- .../service/configuration_edit_flags.go | 12 ++-- pkg/kn/commands/service/create_mock_test.go | 26 +++++++ .../service/service_update_mock_test.go | 71 +++++++++++++++++++ pkg/serving/config_changes.go | 19 +++-- pkg/serving/config_changes_test.go | 45 +++++------- pkg/util/parsing_helper.go | 2 +- 6 files changed, 136 insertions(+), 39 deletions(-) create mode 100644 pkg/kn/commands/service/service_update_mock_test.go diff --git a/pkg/kn/commands/service/configuration_edit_flags.go b/pkg/kn/commands/service/configuration_edit_flags.go index 15ba637f1a..97630d0f9e 100644 --- a/pkg/kn/commands/service/configuration_edit_flags.go +++ b/pkg/kn/commands/service/configuration_edit_flags.go @@ -17,13 +17,13 @@ package service import ( "strings" - errors "github.com/pkg/errors" + "github.com/pkg/errors" "github.com/spf13/cobra" corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/resource" "knative.dev/client/pkg/kn/flags" servinglib "knative.dev/client/pkg/serving" - util "knative.dev/client/pkg/util" + "knative.dev/client/pkg/util" servingv1alpha1 "knative.dev/serving/pkg/apis/serving/v1alpha1" ) @@ -140,10 +140,10 @@ func (p *ConfigurationEditFlags) Apply( 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) + for key := range envMap { + if strings.HasSuffix(key, "-") { + envToRemove = append(envToRemove, key[:len(key)-1]) + delete(envMap, key) } } err = servinglib.UpdateEnvVars(template, envMap, envToRemove) diff --git a/pkg/kn/commands/service/create_mock_test.go b/pkg/kn/commands/service/create_mock_test.go index 7cbd77d0c3..105edd3d29 100644 --- a/pkg/kn/commands/service/create_mock_test.go +++ b/pkg/kn/commands/service/create_mock_test.go @@ -56,6 +56,32 @@ func TestServiceCreateImageMock(t *testing.T) { r.Validate() } +func TestServiceCreateEnvMock(t *testing.T) { + client := knclient.NewMockKnClient(t) + + r := client.Recorder() + r.GetService("foo", nil, errors.NewNotFound(v1alpha1.Resource("service"), "foo")) + + service := getService("foo") + envVars := []corev1.EnvVar{ + {Name: "a", Value: "mouse"}, + {Name: "b", Value: "cookie"}, + {Name: "empty", Value: ""}, + } + template, err := servinglib.RevisionTemplateOfService(service) + assert.NilError(t, err) + template.Spec.GetContainer().Env = envVars + template.Spec.Containers[0].Image = "gcr.io/foo/bar:baz" + template.Annotations = map[string]string{servinglib.UserImageAnnotationKey: "gcr.io/foo/bar:baz"} + r.CreateService(service, nil) + + output, err := executeServiceCommand(client, "create", "foo", "--image", "gcr.io/foo/bar:baz", "-e", "a=mouse", "--env", "b=cookie", "--env=empty", "--async", "--revision-name=") + assert.NilError(t, err) + assert.Assert(t, util.ContainsAll(output, "created", "foo", "default")) + + r.Validate() +} + func TestServiceCreateLabel(t *testing.T) { client := knclient.NewMockKnClient(t) diff --git a/pkg/kn/commands/service/service_update_mock_test.go b/pkg/kn/commands/service/service_update_mock_test.go new file mode 100644 index 0000000000..d9df60fbaf --- /dev/null +++ b/pkg/kn/commands/service/service_update_mock_test.go @@ -0,0 +1,71 @@ +// 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 service + +import ( + "testing" + + "gotest.tools/assert" + corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/api/errors" + + "knative.dev/serving/pkg/apis/serving/v1alpha1" + + servinglib "knative.dev/client/pkg/serving" + knclient "knative.dev/client/pkg/serving/v1alpha1" + + "knative.dev/client/pkg/util" +) + +func TestServiceUpdateEnvMock(t *testing.T) { + client := knclient.NewMockKnClient(t) + + service := getService("foo") + template, err := servinglib.RevisionTemplateOfService(service) + assert.NilError(t, err) + template.Spec.GetContainer().Env = []corev1.EnvVar{ + {Name: "a", Value: "mouse"}, + {Name: "b", Value: "cookie"}, + {Name: "empty", Value: ""}, + } + template.Spec.GetContainer().Image = "gcr.io/foo/bar:baz" + template.Annotations = map[string]string{servinglib.UserImageAnnotationKey: "gcr.io/foo/bar:baz"} + + updated := getService("foo") + template, err = servinglib.RevisionTemplateOfService(updated) + assert.NilError(t, err) + template.Spec.GetContainer().Env = []corev1.EnvVar{ + {Name: "a", Value: "rabbit"}, + {Name: "b", Value: "cookie"}, + } + template.Spec.GetContainer().Image = "gcr.io/foo/bar:baz" + template.Annotations = map[string]string{servinglib.UserImageAnnotationKey: "gcr.io/foo/bar:baz"} + + r := client.Recorder() + r.GetService("foo", nil, errors.NewNotFound(v1alpha1.Resource("service"), "foo")) + r.CreateService(service, nil) + r.GetService("foo", service, nil) + r.UpdateService(updated, nil) + + output, err := executeServiceCommand(client, "create", "foo", "--image", "gcr.io/foo/bar:baz", "-e", "a=mouse", "--env", "b=cookie", "--env=empty", "--async", "--revision-name=") + assert.NilError(t, err) + assert.Assert(t, util.ContainsAll(output, "created", "foo", "default")) + + output, err = executeServiceCommand(client, "update", "foo", "-e", "a=rabbit", "--env=empty-", "--async", "--revision-name=") + assert.NilError(t, err) + assert.Assert(t, util.ContainsAll(output, "updated", "foo", "default")) + + r.Validate() +} diff --git a/pkg/serving/config_changes.go b/pkg/serving/config_changes.go index d438d1792a..fc719d37a7 100644 --- a/pkg/serving/config_changes.go +++ b/pkg/serving/config_changes.go @@ -18,6 +18,7 @@ import ( "context" "errors" "fmt" + "sort" "strconv" "strings" @@ -31,14 +32,20 @@ var UserImageAnnotationKey = "client.knative.dev/user-image" // 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. +// new env vars and change the values of existing ones, then sort by env key name. func UpdateEnvVars(template *servingv1alpha1.RevisionTemplateSpec, toUpdate map[string]string, toRemove []string) error { container, err := ContainerOfRevisionTemplate(template) if err != nil { return err } - envVars := updateEnvVarsFromMap(container.Env, toUpdate) - container.Env = removeEnvVars(envVars, toRemove) + updated := updateEnvVarsFromMap(container.Env, toUpdate) + updated = removeEnvVars(updated, toRemove) + // Sort by env key name + sort.SliceStable(updated, func(i, j int) bool { + return updated[i].Name < updated[j].Name + }) + container.Env = updated + return nil } @@ -235,17 +242,17 @@ func UpdateLabels(service *servingv1alpha1.Service, template *servingv1alpha1.Re // ======================================================================================= -func updateEnvVarsFromMap(env []corev1.EnvVar, vars map[string]string) []corev1.EnvVar { +func updateEnvVarsFromMap(env []corev1.EnvVar, toUpdate map[string]string) []corev1.EnvVar { set := make(map[string]bool) for i := range env { envVar := &env[i] - value, present := vars[envVar.Name] + value, present := toUpdate[envVar.Name] if present { envVar.Value = value set[envVar.Name] = true } } - for name, value := range vars { + for name, value := range toUpdate { if !set[name] { env = append( env, diff --git a/pkg/serving/config_changes_test.go b/pkg/serving/config_changes_test.go index b7fab9cabc..a36385435b 100644 --- a/pkg/serving/config_changes_test.go +++ b/pkg/serving/config_changes_test.go @@ -78,15 +78,15 @@ func TestUpdateEnvVarsNew(t *testing.T) { } func testUpdateEnvVarsNew(t *testing.T, template *servingv1alpha1.RevisionTemplateSpec, container *corev1.Container) { - env := map[string]string{ - "a": "foo", - "b": "bar", + env := []corev1.EnvVar{ + {Name: "a", Value: "foo"}, + {Name: "b", Value: "bar"}, } - err := UpdateEnvVars(template, env, []string{}) + found, err := EnvToMap(env) assert.NilError(t, err) - found, err := EnvToMap(container.Env) + err = UpdateEnvVars(template, found, []string{}) assert.NilError(t, err) - assert.DeepEqual(t, env, found) + assert.DeepEqual(t, env, container.Env) } func TestUpdateEnvVarsAppendOld(t *testing.T) { @@ -149,20 +149,19 @@ func testUpdateEnvVarsAppendOld(t *testing.T, template *servingv1alpha1.Revision container.Env = []corev1.EnvVar{ {Name: "a", Value: "foo"}, } + env := map[string]string{ "b": "bar", } err := UpdateEnvVars(template, env, []string{}) assert.NilError(t, err) - expected := map[string]string{ - "a": "foo", - "b": "bar", + expected := []corev1.EnvVar{ + {Name: "a", Value: "foo"}, + {Name: "b", Value: "bar"}, } - found, err := EnvToMap(container.Env) - assert.NilError(t, err) - assert.DeepEqual(t, expected, found) + assert.DeepEqual(t, expected, container.Env) } func TestUpdateEnvVarsModify(t *testing.T) { @@ -212,13 +211,11 @@ func testUpdateEnvVarsRemove(t *testing.T, revision *servingv1alpha1.RevisionTem err := UpdateEnvVars(revision, map[string]string{}, remove) assert.NilError(t, err) - expected := map[string]string{ - "a": "foo", + expected := []corev1.EnvVar{ + {"a", "foo", nil}, } - found, err := EnvToMap(container.Env) - assert.NilError(t, err) - assert.DeepEqual(t, expected, found) + assert.DeepEqual(t, expected, container.Env) } func TestUpdateMinScale(t *testing.T) { @@ -336,17 +333,13 @@ func testUpdateEnvVarsAll(t *testing.T, template *servingv1alpha1.RevisionTempla err := UpdateEnvVars(template, env, remove) assert.NilError(t, err) - expected := map[string]string{ - "a": "fancy", - "b": "boo", - "c": "caroline", + expected := []corev1.EnvVar{ + {Name: "a", Value: "fancy"}, + {Name: "b", Value: "boo"}, + {Name: "c", Value: "caroline"}, } - 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, container.Env) } func TestUpdateLabelsNew(t *testing.T) { diff --git a/pkg/util/parsing_helper.go b/pkg/util/parsing_helper.go index 49e3df2694..eb41bc7539 100644 --- a/pkg/util/parsing_helper.go +++ b/pkg/util/parsing_helper.go @@ -28,7 +28,7 @@ func MapFromArray(arr []string, delimiter string) (map[string]string, error) { } // 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. +// 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 func mapFromArray(arr []string, delimiter string, allowSingles bool) (map[string]string, error) { returnMap := map[string]string{}