Skip to content

Commit

Permalink
Revert back to the current structure and just sort by 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 f0f8c47 commit 5d2c38d
Show file tree
Hide file tree
Showing 3 changed files with 47 additions and 148 deletions.
10 changes: 5 additions & 5 deletions pkg/kn/commands/service/configuration_edit_flags.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -139,15 +139,15 @@ func (p *ConfigurationEditFlags) Apply(
if err != nil {
return errors.Wrap(err, "Invalid --env")
}
envMapToRemove := make(map[string]bool)
envKeyToRemove := []string{}
for key := range envMap {
if strings.HasSuffix(key, "-") {
envMapToRemove[key[:len(key)-1]] = true
delete(envMap, key)
if !strings.HasSuffix(key, "-") {
continue
}
envKeyToRemove = append(envKeyToRemove, key[:len(key)-1])
delete(envMap, key)
}
err = servinglib.UpdateEnvVars(template, envMap, envMapToRemove)
err = servinglib.UpdateEnvVars(template, envMap, envKeyToRemove)
if err != nil {
return err
}
Expand Down
57 changes: 37 additions & 20 deletions pkg/serving/config_changes.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,12 +33,19 @@ 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, then sort by environment key name.
func UpdateEnvVars(template *servingv1alpha1.RevisionTemplateSpec, toUpdate map[string]string, toRemove map[string]bool) error {
func UpdateEnvVars(template *servingv1alpha1.RevisionTemplateSpec, toUpdate map[string]string, toRemove []string) error {
container, err := ContainerOfRevisionTemplate(template)
if err != nil {
return err
}
container.Env = mergeEnvVarsWithStableOrder(container.Env, toUpdate, toRemove)
updated := updateEnvVars(container.Env, toUpdate)
removed := removeEnvVars(updated, toRemove)
// Sort by Env key name
sort.SliceStable(removed, func(i, j int) bool {
return removed[i].Name < removed[j].Name
})
container.Env = removed

return nil
}

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

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

// 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 {
if _, ok := toRemove[v.Name]; ok {
continue
func updateEnvVars(env []corev1.EnvVar, toUpdate map[string]string) []corev1.EnvVar {
set := make(map[string]bool)
for i := range env {
envVar := &env[i]
value, present := toUpdate[envVar.Name]
if present {
envVar.Value = value
set[envVar.Name] = true
}
// Register only non-duplicate EnvVars, which results in overwriting old EnvVars
if _, ok := target[v.Name]; !ok {
target[v.Name] = v.Value
}
for name, value := range toUpdate {
if !set[name] {
env = append(
env,
corev1.EnvVar{
Name: name,
Value: value,
})
}
}
return env
}

merged := []corev1.EnvVar{}
for key, val := range target {
merged = append(merged, corev1.EnvVar{Name: key, Value: val})
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
}
}
}

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

return merged
return env
}
128 changes: 5 additions & 123 deletions pkg/serving/config_changes_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ func testUpdateEnvVarsNew(t *testing.T, template *servingv1alpha1.RevisionTempla
}
found, err := EnvToMap(env)
assert.NilError(t, err)
err = UpdateEnvVars(template, found, map[string]bool{})
err = UpdateEnvVars(template, found, []string{})
assert.NilError(t, err)
assert.DeepEqual(t, env, container.Env)
}
Expand Down Expand Up @@ -153,7 +153,7 @@ func testUpdateEnvVarsAppendOld(t *testing.T, template *servingv1alpha1.Revision
env := map[string]string{
"b": "bar",
}
err := UpdateEnvVars(template, env, map[string]bool{})
err := UpdateEnvVars(template, env, []string{})
assert.NilError(t, err)

expected := []corev1.EnvVar{
Expand All @@ -180,7 +180,7 @@ func testUpdateEnvVarsModify(t *testing.T, revision *servingv1alpha1.RevisionTem
env := map[string]string{
"a": "fancy",
}
err := UpdateEnvVars(revision, env, map[string]bool{})
err := UpdateEnvVars(revision, env, []string{})
assert.NilError(t, err)

expected := map[string]string{
Expand All @@ -207,7 +207,7 @@ func testUpdateEnvVarsRemove(t *testing.T, revision *servingv1alpha1.RevisionTem
{Name: "a", Value: "foo"},
{Name: "b", Value: "bar"},
}
remove := map[string]bool{"b": true}
remove := []string{"b"}
err := UpdateEnvVars(revision, map[string]string{}, remove)
assert.NilError(t, err)

Expand Down Expand Up @@ -329,7 +329,7 @@ func testUpdateEnvVarsAll(t *testing.T, template *servingv1alpha1.RevisionTempla
"a": "fancy",
"b": "boo",
}
remove := map[string]bool{"d": true}
remove := []string{"d"}
err := UpdateEnvVars(template, env, remove)
assert.NilError(t, err)

Expand Down Expand Up @@ -408,124 +408,6 @@ func TestUpdateLabelsRemoveExisting(t *testing.T) {
assert.DeepEqual(t, expected, actual)
}

func TestMergeEnvVarsWithStableOrder(t *testing.T) {
tests := []struct {
name string
src []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: map[string]string{
"FOO": "foo",
},
toRemove: map[string]bool{},
want: []corev1.EnvVar{
{Name: "FOO", Value: "foo"},
},
},
{
name: "add no new EnvVar",
src: []corev1.EnvVar{
{Name: "FOO", Value: "foo"},
},
target: map[string]string{},
toRemove: map[string]bool{},
want: []corev1.EnvVar{
{Name: "FOO", Value: "foo"},
},
},
{
name: "add non-duplicate EnvVars",
src: []corev1.EnvVar{
{Name: "FOO", Value: "foo"},
},
target: map[string]string{
"BAR": "bar",
},
toRemove: map[string]bool{},
want: []corev1.EnvVar{
{Name: "BAR", Value: "bar"},
{Name: "FOO", Value: "foo"},
},
},
{
name: "add duplicated EnvVars",
src: []corev1.EnvVar{
{Name: "FOO", Value: "foo"},
{Name: "BAR", Value: "bar"},
{Name: "BAZ", Value: "baz"},
},
target: map[string]string{
"FOO": "foov2",
"QUX": "qux",
},
toRemove: map[string]bool{},
want: []corev1.EnvVar{
{Name: "BAR", Value: "bar"},
{Name: "BAZ", Value: "baz"},
{Name: "FOO", Value: "foov2"},
{Name: "QUX", Value: "qux"},
},
},
{
name: "remove more than one EnvVars",
src: []corev1.EnvVar{
{Name: "FOO", Value: "foo"},
{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"},
},
},
{
name: "remove non-existing EnvVars",
src: []corev1.EnvVar{
{Name: "FOO", Value: "foo"},
{Name: "BAR", Value: "bar"},
{Name: "BAZ", Value: "baz"},
},
target: map[string]string{},
toRemove: map[string]bool{"WHAT": true, "BAR": true},
want: []corev1.EnvVar{
{Name: "BAZ", Value: "baz"},
{Name: "FOO", Value: "foo"},
},
},
{
name: "add duplicated EnvVars and remove more than one EnvVars",
src: []corev1.EnvVar{
{Name: "FOO", Value: "foo"},
{Name: "BAR", Value: "bar"},
{Name: "BAZ", Value: "baz"},
},
target: map[string]string{
"FOO": "foov2",
"QUX": "qux",
},
toRemove: map[string]bool{"WHAT": true, "BAR": true},
want: []corev1.EnvVar{
{Name: "BAZ", Value: "baz"},
{Name: "FOO", Value: "foov2"},
{Name: "QUX", Value: "qux"},
},
},
}

for _, test := range tests {
t.Run(test.name, func(t *testing.T) {
got := mergeEnvVarsWithStableOrder(test.src, test.target, test.toRemove)
assert.DeepEqual(t, got, test.want)
})
}
}

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

func getV1alpha1RevisionTemplateWithOldFields() (*servingv1alpha1.RevisionTemplateSpec, *corev1.Container) {
Expand Down

0 comments on commit 5d2c38d

Please sign in to comment.