Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Bug1277038 - Differ between oc env updates(removes/adds) the resource(s) envvars and when it doesnt #6104

Closed
wants to merge 1 commit into from
+38 −20
Closed

Bug1277038 - Differ between oc env updates(removes/adds) the resource(s) envvars and when it doesnt #6104

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
54 changes: 34 additions & 20 deletions pkg/cmd/cli/cmd/env.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,9 @@ func validateNoOverwrites(meta *kapi.ObjectMeta, labels map[string]string) error
return nil
}

// ParseEnv parses the list of environment variables into kubernetes EnvVar
// ParseEnv parses the list of environment variables into kubernetes EnvVar.
// Returns map of environment variables to be added with their values, array
// of environment variables to be removed and error.
func ParseEnv(spec []string, defaultReader io.Reader) ([]kapi.EnvVar, []string, error) {
env := []kapi.EnvVar{}
exists := sets.NewString()
Expand Down Expand Up @@ -252,15 +254,21 @@ func RunEnv(f *clientcmd.Factory, in io.Reader, out io.Writer, cmd *cobra.Comman
}

skipped := 0
for _, info := range infos {
var envUpdated bool
var updated = make([]bool, len(infos))
for i, info := range infos {
updated[i] = false
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

defaults to false already

ok, err := f.UpdatePodSpecForObject(info.Object, func(spec *kapi.PodSpec) error {
containers, _ := selectContainers(spec.Containers, containerMatch)
if len(containers) == 0 {
fmt.Fprintf(cmd.Out(), "warning: %s/%s does not have any containers matching %q\n", info.Mapping.Resource, info.Name, containerMatch)
return nil
}
for _, c := range containers {
c.Env = updateEnv(c.Env, env, remove)
c.Env, envUpdated = updateEnv(c.Env, env, remove)
if envUpdated {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated[i] = updated[i] || envUpdated

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

all the items in updated array are set to false by default, so no point to check for it, just put the envUpdated value to it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nope, then the last one in wins... need to ||

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but in this case it doesn't matter which wins. Once the updated[i] is set to true it can't be changed to other value in any iteration.

updated[i] = true
}

if list {
fmt.Fprintf(out, "# %s %s, container %s\n", info.Mapping.Resource, info.Name, c.Name)
Expand Down Expand Up @@ -324,6 +332,11 @@ func RunEnv(f *clientcmd.Factory, in io.Reader, out io.Writer, cmd *cobra.Comman

failed := false
for i, info := range infos {
shortOutput := cmdutil.GetFlagString(cmd, "output") == "name"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

move shortOutput := cmdutil.GetFlagString(cmd, "output") == "name" outside the loop

if !updated[i] {
cmdutil.PrintSuccess(mapper, shortOutput, out, info.Mapping.Resource, info.Name, "not updated")
continue
}
newData, err := json.Marshal(objects[i])
if err != nil {
return err
Expand All @@ -338,9 +351,8 @@ func RunEnv(f *clientcmd.Factory, in io.Reader, out io.Writer, cmd *cobra.Comman
failed = true
continue
}
info.Refresh(obj, true)

shortOutput := cmdutil.GetFlagString(cmd, "output") == "name"
info.Refresh(obj, true)
cmdutil.PrintSuccess(mapper, shortOutput, out, info.Mapping.Resource, info.Name, "updated")
}
if failed {
Expand All @@ -349,36 +361,38 @@ func RunEnv(f *clientcmd.Factory, in io.Reader, out io.Writer, cmd *cobra.Comman
return nil
}

func updateEnv(existing []kapi.EnvVar, env []kapi.EnvVar, remove []string) []kapi.EnvVar {
func updateEnv(existing []kapi.EnvVar, env []kapi.EnvVar, remove []string) ([]kapi.EnvVar, bool) {
out := []kapi.EnvVar{}
covered := sets.NewString(remove...)
envUpdated := false
toDelete := sets.NewString(remove...)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

need to track existing names in a set

for _, e := range existing {
if covered.Has(e.Name) {
if toDelete.Has(e.Name) {
envUpdated = true
continue
}
newer, ok := findEnv(env, e.Name)
newer, ok := findEnv(env, e)
if ok {
covered.Insert(e.Name)
out = append(out, newer)
continue
envUpdated = true
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

only if the value has changed, right?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this is probably a tight enough check

}
out = append(out, e)
out = append(out, newer)
}

for _, e := range env {
if covered.Has(e.Name) {
if toDelete.Has(e.Name) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be skipping if the env already exists, not if it was in the set to delete... it's an error to delete and set/update the same envvar in the same command (prevented by validation, I'm pretty sure)

continue
}
covered.Insert(e.Name)
out = append(out, e)
envUpdated = true
}
return out
return out, envUpdated
}

func findEnv(env []kapi.EnvVar, name string) (kapi.EnvVar, bool) {
for _, e := range env {
if e.Name == name {
func findEnv(envs []kapi.EnvVar, env kapi.EnvVar) (kapi.EnvVar, bool) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why the change from looking for a name?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

comment below

for _, e := range envs {
if e.Name == env.Name {
return e, true
}
}
return kapi.EnvVar{}, false
return env, false
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is confusing... I prefer the original impl where a false retval meant you should ignore the returned env

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the idea is to return the new EnvVar in case of overriding the envvar or return the old one

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the old way was clearer... putting a value in and not easily knowing which you get back is confusing

}
4 changes: 4 additions & 0 deletions test/cmd/deployments.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,11 @@ os::cmd::expect_success_and_text 'oc env dc/test-deployment-config --list' 'H=h'
os::cmd::expect_success_and_not_text 'oc env dc/test-deployment-config --list' 'A=a'
os::cmd::expect_success_and_not_text 'oc env dc/test-deployment-config --list' 'C=c'
os::cmd::expect_success_and_not_text 'oc env dc/test-deployment-config --list' 'G=g'
os::cmd::expect_success_and_text 'oc env dc/test-deployment-config TEST=bar' 'not updated'
os::cmd::expect_success_and_text 'oc env dc/test-deployment-config BAR-' 'not updated'
echo "env: ok"


os::cmd::expect_success 'oc deploy test-deployment-config'
os::cmd::expect_success 'oc deploy dc/test-deployment-config'
os::cmd::expect_success 'oc delete deploymentConfigs test-deployment-config'
Expand Down