Skip to content

Commit

Permalink
Update EnvVars in alphabetical order of Env key name
Browse files Browse the repository at this point in the history
  • Loading branch information
@toVersus
toVersus committed Sep 10, 2019
1 parent 0af7df2 commit f0f8c47
Show file tree
Hide file tree
Showing 5 changed files with 58 additions and 171 deletions.
28 changes: 7 additions & 21 deletions pkg/kn/commands/service/configuration_edit_flags.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -135,20 +135,19 @@ func (p *ConfigurationEditFlags) Apply(
}

if cmd.Flags().Changed("env") {
envVars, err := envVarsFromSlice(p.Env, "=")
envMap, err := util.MapFromArrayAllowingSingles(p.Env, "=")
if err != nil {
return errors.Wrap(err, "Invalid --env")
}
envVarsToAdd := envVars[:0]
envVarsToRemove := make(map[string]bool)
for _, env := range envVars {
if strings.HasSuffix(env.Name, "-") {
envVarsToRemove[env.Name[:len(env.Name)-1]] = true
envMapToRemove := make(map[string]bool)
for key := range envMap {
if strings.HasSuffix(key, "-") {
envMapToRemove[key[:len(key)-1]] = true
delete(envMap, key)
continue
}
envVarsToAdd = append(envVarsToAdd, env)
}
err = servinglib.UpdateEnvVars(template, envVarsToAdd, envVarsToRemove)
err = servinglib.UpdateEnvVars(template, envMap, envMapToRemove)
if err != nil {
return err
}
Expand Down Expand Up @@ -293,16 +292,3 @@ func (p *ConfigurationEditFlags) AnyMutation(cmd *cobra.Command) bool {
}
return false
}

func envVarsFromSlice(envFlags []string, delimiter string) ([]corev1.EnvVar, error) {
envMap, envOrdered, err := util.ParseFlagsWithOrdering(envFlags, delimiter)
if err != nil {
return nil, err
}

envVars := []corev1.EnvVar{}
for _, flag := range envOrdered {
envVars = append(envVars, corev1.EnvVar{Name: flag, Value: envMap[flag]})
}
return envVars, nil
}
48 changes: 17 additions & 31 deletions pkg/serving/config_changes.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import (
"context"
"errors"
"fmt"
"sort"
"strconv"
"strings"

Expand All @@ -31,14 +32,13 @@ 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.
func UpdateEnvVars(template *servingv1alpha1.RevisionTemplateSpec, toUpdate []corev1.EnvVar, toRemove map[string]bool) error {
// new env vars and change the values of existing ones, then sort by environment key name.
func UpdateEnvVars(template *servingv1alpha1.RevisionTemplateSpec, toUpdate map[string]string, toRemove map[string]bool) error {
container, err := ContainerOfRevisionTemplate(template)
if err != nil {
return err
}
envVars := mergeEnvVarsWithStableOrder(container.Env, toUpdate, toRemove)
container.Env = envVars
container.Env = mergeEnvVarsWithStableOrder(container.Env, toUpdate, toRemove)
return nil
}

Expand Down Expand Up @@ -235,41 +235,27 @@ func UpdateLabels(service *servingv1alpha1.Service, template *servingv1alpha1.Re

// =======================================================================================

func mergeEnvVarsWithStableOrder(src []corev1.EnvVar, target []corev1.EnvVar, toRemove map[string]bool) []corev1.EnvVar {
// dict records all EnvVars with fixed capacity
dict := make(map[string]string, len(src)+len(target))
for _, v := range target {
dict[v.Name] = v.Value
}
srcUpdated := src[:0]
// mergeEnvVarsWithStableOrder merges source EnvVars into target and sorts by Env key name.
func mergeEnvVarsWithStableOrder(src []corev1.EnvVar, target map[string]string, toRemove map[string]bool) []corev1.EnvVar {
for _, v := range src {
// Register only non-duplicate EnvVars, which results in overwriting old EnvVars
if _, ok := dict[v.Name]; ok {
// Don't change the order of updated items
srcUpdated = append(srcUpdated, v)
if _, ok := toRemove[v.Name]; ok {
continue
}
if _, ok := toRemove[v.Name]; !ok {
dict[v.Name] = v.Value
// Only append non-duplicate key-value pair
srcUpdated = append(srcUpdated, v)
// Register only non-duplicate EnvVars, which results in overwriting old EnvVars
if _, ok := target[v.Name]; !ok {
target[v.Name] = v.Value
}
}

// merged EnvVars keep the original order of equal elements
merged := []corev1.EnvVar{}
for _, v := range srcUpdated {
if _, ok := dict[v.Name]; ok {
merged = append(merged, corev1.EnvVar{Name: v.Name, Value: dict[v.Name]})
// Delete dict item for avoiding duplicated entries
delete(dict, v.Name)
}
}
for _, v := range target {
if _, ok := dict[v.Name]; ok {
merged = append(merged, corev1.EnvVar{Name: v.Name, Value: dict[v.Name]})
}
for key, val := range target {
merged = append(merged, corev1.EnvVar{Name: key, Value: val})
}

// Sort by Env key name
sort.SliceStable(merged, func(i, j int) bool {
return merged[i].Name < merged[j].Name
})

return merged
}
62 changes: 33 additions & 29 deletions pkg/serving/config_changes_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -79,10 +79,12 @@ func TestUpdateEnvVarsNew(t *testing.T) {

func testUpdateEnvVarsNew(t *testing.T, template *servingv1alpha1.RevisionTemplateSpec, container *corev1.Container) {
env := []corev1.EnvVar{
{"a", "foo", nil},
{"b", "bar", nil},
{Name: "a", Value: "foo"},
{Name: "b", Value: "bar"},
}
err := UpdateEnvVars(template, env, map[string]bool{})
found, err := EnvToMap(env)
assert.NilError(t, err)
err = UpdateEnvVars(template, found, map[string]bool{})
assert.NilError(t, err)
assert.DeepEqual(t, env, container.Env)
}
Expand Down Expand Up @@ -148,8 +150,8 @@ func testUpdateEnvVarsAppendOld(t *testing.T, template *servingv1alpha1.Revision
{Name: "a", Value: "foo"},
}

env := []corev1.EnvVar{
{Name: "b", Value: "bar"},
env := map[string]string{
"b": "bar",
}
err := UpdateEnvVars(template, env, map[string]bool{})
assert.NilError(t, err)
Expand All @@ -175,8 +177,8 @@ func TestUpdateEnvVarsModify(t *testing.T) {
func testUpdateEnvVarsModify(t *testing.T, revision *servingv1alpha1.RevisionTemplateSpec, container *corev1.Container) {
container.Env = []corev1.EnvVar{
{Name: "a", Value: "foo"}}
env := []corev1.EnvVar{
{"a", "fancy", nil},
env := map[string]string{
"a": "fancy",
}
err := UpdateEnvVars(revision, env, map[string]bool{})
assert.NilError(t, err)
Expand Down Expand Up @@ -206,7 +208,7 @@ func testUpdateEnvVarsRemove(t *testing.T, revision *servingv1alpha1.RevisionTem
{Name: "b", Value: "bar"},
}
remove := map[string]bool{"b": true}
err := UpdateEnvVars(revision, []corev1.EnvVar{}, remove)
err := UpdateEnvVars(revision, map[string]string{}, remove)
assert.NilError(t, err)

expected := []corev1.EnvVar{
Expand Down Expand Up @@ -323,18 +325,18 @@ func testUpdateEnvVarsAll(t *testing.T, template *servingv1alpha1.RevisionTempla
{Name: "c", Value: "caroline"},
{Name: "d", Value: "byebye"},
}
env := []corev1.EnvVar{
{Name: "a", Value: "fancy"},
{Name: "b", Value: "boo"},
env := map[string]string{
"a": "fancy",
"b": "boo",
}
remove := map[string]bool{"d": true}
err := UpdateEnvVars(template, env, remove)
assert.NilError(t, err)

expected := []corev1.EnvVar{
{Name: "a", Value: "fancy"},
{Name: "c", Value: "caroline"},
{Name: "b", Value: "boo"},
{Name: "c", Value: "caroline"},
}

assert.DeepEqual(t, expected, container.Env)
Expand Down Expand Up @@ -410,15 +412,15 @@ func TestMergeEnvVarsWithStableOrder(t *testing.T) {
tests := []struct {
name string
src []corev1.EnvVar
target []corev1.EnvVar
target map[string]string
toRemove map[string]bool
want []corev1.EnvVar
}{
{
name: "add new EnvVar from nothing in there",
src: []corev1.EnvVar{},
target: []corev1.EnvVar{
{Name: "FOO", Value: "foo"},
target: map[string]string{
"FOO": "foo",
},
toRemove: map[string]bool{},
want: []corev1.EnvVar{
Expand All @@ -430,24 +432,24 @@ func TestMergeEnvVarsWithStableOrder(t *testing.T) {
src: []corev1.EnvVar{
{Name: "FOO", Value: "foo"},
},
target: []corev1.EnvVar{},
target: map[string]string{},
toRemove: map[string]bool{},
want: []corev1.EnvVar{
{Name: "FOO", Value: "foo"},
},
},
{
name: "add non-duplicated EnvVars",
name: "add non-duplicate EnvVars",
src: []corev1.EnvVar{
{Name: "FOO", Value: "foo"},
},
target: []corev1.EnvVar{
{Name: "BAR", Value: "bar"},
target: map[string]string{
"BAR": "bar",
},
toRemove: map[string]bool{},
want: []corev1.EnvVar{
{Name: "FOO", Value: "foo"},
{Name: "BAR", Value: "bar"},
{Name: "FOO", Value: "foo"},
},
},
{
Expand All @@ -457,15 +459,15 @@ func TestMergeEnvVarsWithStableOrder(t *testing.T) {
{Name: "BAR", Value: "bar"},
{Name: "BAZ", Value: "baz"},
},
target: []corev1.EnvVar{
{Name: "FOO", Value: "foov2"},
{Name: "QUX", Value: "qux"},
target: map[string]string{
"FOO": "foov2",
"QUX": "qux",
},
toRemove: map[string]bool{},
want: []corev1.EnvVar{
{Name: "FOO", Value: "foov2"},
{Name: "BAR", Value: "bar"},
{Name: "BAZ", Value: "baz"},
{Name: "FOO", Value: "foov2"},
{Name: "QUX", Value: "qux"},
},
},
Expand All @@ -476,6 +478,7 @@ func TestMergeEnvVarsWithStableOrder(t *testing.T) {
{Name: "BAR", Value: "bar"},
{Name: "BAZ", Value: "baz"},
},
target: map[string]string{},
toRemove: map[string]bool{"FOO": true, "BAR": true},
want: []corev1.EnvVar{
{Name: "BAZ", Value: "baz"},
Expand All @@ -488,10 +491,11 @@ func TestMergeEnvVarsWithStableOrder(t *testing.T) {
{Name: "BAR", Value: "bar"},
{Name: "BAZ", Value: "baz"},
},
target: map[string]string{},
toRemove: map[string]bool{"WHAT": true, "BAR": true},
want: []corev1.EnvVar{
{Name: "FOO", Value: "foo"},
{Name: "BAZ", Value: "baz"},
{Name: "FOO", Value: "foo"},
},
},
{
Expand All @@ -501,14 +505,14 @@ func TestMergeEnvVarsWithStableOrder(t *testing.T) {
{Name: "BAR", Value: "bar"},
{Name: "BAZ", Value: "baz"},
},
target: []corev1.EnvVar{
{Name: "FOO", Value: "foov2"},
{Name: "QUX", Value: "qux"},
target: map[string]string{
"FOO": "foov2",
"QUX": "qux",
},
toRemove: map[string]bool{"WHAT": true, "BAR": true},
want: []corev1.EnvVar{
{Name: "FOO", Value: "foov2"},
{Name: "BAZ", Value: "baz"},
{Name: "FOO", Value: "foov2"},
{Name: "QUX", Value: "qux"},
},
},
Expand Down
31 changes: 1 addition & 30 deletions pkg/util/parsing_helper.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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{}
Expand All @@ -45,32 +45,3 @@ func mapFromArray(arr []string, delimiter string, allowSingles bool) (map[string
}
return returnMap, nil
}

// ParseFlagsWithOrdering takes a slice of strings where each item is a key value pair
// separated by a delimiter and returns both parsed map and ordering slice.
// The value of parsed map is overwritten by the last found value and
// the key is appended to ordering slice when first found.
func ParseFlagsWithOrdering(flags []string, delimiter string) (map[string]string, []string, error) {
parsedMap := make(map[string]string, len(flags))
flagOrdered := flags[:0]
for _, flag := range flags {
pair := strings.SplitN(flag, delimiter, 2)
switch len(pair) {
case 0:
return nil, nil, fmt.Errorf("argument requires a value that contains the %q character; got %q", delimiter, pair)
case 1:
if _, ok := parsedMap[pair[0]]; !ok {
flagOrdered = append(flagOrdered, pair[0])
}
parsedMap[pair[0]] = ""
case 2:
if _, ok := parsedMap[pair[0]]; !ok {
flagOrdered = append(flagOrdered, pair[0])
}
parsedMap[pair[0]] = pair[1]
default:
return nil, nil, fmt.Errorf("argument having more than one %q delimiter; got %q", delimiter, pair)
}
}
return parsedMap, flagOrdered, nil
}
Loading

0 comments on commit f0f8c47

Please sign in to comment.