Update EnvVars in alphabetical order of Env key name (#389)

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)

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)
 

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()
+}

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,

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) {

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{}