-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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.
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
Bug1277038 - Differ between oc env
updates(removes/adds) the resource(s) envvars and when it doesnt
#6104
Conversation
[test] |
Will conflict/overlap with #5819 |
@liggitt I've already spoken with @Kargakis about the overlap and this PR doesn't solve this issue.
Don't have any strong feelings about both solutions. Thoughts ? |
@Kargakis yes, once the our PR is merged, will rebase. Thx |
@fabianofranz I've updated the PR after #5819 got merged
PTAL |
What do the label/annotate commands do in this case? |
@liggitt I've already proposed this kind of change for the Regarding the |
for _, env := range remove { | ||
_, ok := findEnv(c.Env, env) | ||
if !ok { | ||
infosMsgs[i] = append(infosMsgs[i], fmt.Sprintf("environment variable %q not found in %s container\n", env, c.Name)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So if something has multiple containers, one of which has an envvar, and you remove the envvar, you'll always get a message saying the other container didn't have that var? That doesn't seem necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if user is updating the envars in a pod, by default it will update all the containers. For that reason user should specify --containers
flag I guess.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wouldn't print warnings that an envvar they wanted to remove didn't exist in all containers. If it existed in at least one, that's probably fine
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, how about printing them with glog at some higher verbosity ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nah, if I ask to remove envvar B, and it exists in one container but not another, I don't consider that failure or warning-worthy
@liggitt updated |
continue | ||
outputMsg := "updated" | ||
if !updated[i] { | ||
outputMsg = "nothing to update" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of doing this and nesting all the code, do this:
if !updated[i] {
cmdutil.PrintSuccess(mapper, shortOutput, out, info.Mapping.Resource, info.Name, "didn't need to update")
continue
}
// rest of the code...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also it would be nice if this (no need for update) was handled by CreateTwoWayMergePatch
by returning an empty byte slice and not by us but that's not the case: kubernetes/kubernetes#17022
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually @liggitt if we pin the size of an empty patch (a couple of bytes) to a constant and check for it, wouldn't it be better?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The size of empty patch 2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's {}
, right? Not sure how I feel about checking that. I think I would rather have updateEnv
additionally return a boolean indicating whether any updates were made
@@ -371,7 +384,11 @@ func updateEnv(existing []kapi.EnvVar, env []kapi.EnvVar, remove []string) []kap | |||
covered.Insert(e.Name) | |||
out = append(out, e) | |||
} | |||
return out | |||
envUpdated := true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
instead of using reflection, I'd rather initialize envUpdated to false at the top, and set it to true in the loop where we know we're making changes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then the line which would be setting envUpdate
to false would need to be basically in all three if
statements and also at the end of the for
loops, due to the logic the out
variable is build. Would rather prevent prevent that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still would rather set it explicitly... I think some of the complexity would be reduced by handling each case (removed, modified, added) in its own loop
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, will rewrite the logic and set it explicitly :)
…vars and when it doesnt
@liggitt cleaned the logic a bit as suggested. |
Evaluated for origin test up to 2cbfd4a |
continuous-integration/openshift-jenkins/test FAILURE (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin/7677/) |
var envUpdated bool | ||
var updated = make([]bool, len(infos)) | ||
for i, info := range infos { | ||
updated[i] = false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
defaults to false
already
covered.Insert(e.Name) | ||
out = append(out, newer) | ||
continue | ||
envUpdated = true |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
@jhadvig Bump |
Origin Action Required: Pull request cannot be automatically merged, please rebase your branch from latest HEAD and push again |
Closing due to age and inactivity. Feel free to reopen if you are going to continue working on it. |
When adding/removing envvars with
oc env
cmd, differ between states when the command actually updates the resource(s) and when it doesnt.Eg: When updating resource by adding the envvar with a value that is already present in the resource, or removing a envvar that is not present, the printed output will look accordingly:
replicationcontroller "database-1" nothing to update
instead of
replicationcontroller "database-1" updated
Wasnt sure if we also wanna print due to which envar the resource wasnt updated (eg1. removing envvar thats not present in the resource || updating envar with the same value), since we would have to revrite some parts of the
env.go
and will end up with dup. messages in case user would want to update all rcs/dc with wrong envars(eg1).@fabianofranz thoughts ?