only set term if rsh is running /bin/sh #12386
Conversation
|
@smarterclayton ptal |
|
(this is effectively running |
| term := fmt.Sprintf("TERM=%s", util.Env("TERM", "xterm")) | ||
| o.Command = append([]string{"env", term}, o.Command...) | ||
|
|
||
| if len(o.Command) == 1 && o.Command[0] == "/bin/sh" { |
There was a problem hiding this comment.
This should probably be checking against o.Executable
| o.Command = append([]string{"env", term}, o.Command...) | ||
|
|
||
| if len(o.Command) == 1 && o.Command[0] == "/bin/sh" { | ||
| termsh := fmt.Sprintf("TERM=%s /bin/sh", util.Env("TERM", "xterm")) |
There was a problem hiding this comment.
Should be o.Executable here too.
There was a problem hiding this comment.
Also, you may have broken quotes here if term has spaces.
There was a problem hiding this comment.
Probably need to use %q
|
@smarterclayton thanks, updated. |
|
|
||
| if len(o.Command) == 1 && o.Command[0] == o.Executable { | ||
| termsh := fmt.Sprintf("TERM=%q %s", util.Env("TERM", "xterm"), o.Executable) | ||
| o.Command = append(o.Command, "-c", termsh) |
There was a problem hiding this comment.
my only fear now is that since we're doing this for any value of --shell the user passes, if they pass a --shell value that doesn't support "-c", things will be broken. That was part of my motivation to only do this logic when the shell was explicitly "/bin/sh".
There was a problem hiding this comment.
I think anyone passing a custom executable or custom command is probably on their own. However, I would probably expect this to work for --shell=/bin/bash. Ultimately this is a losing game, so it's probably ok to only do this substitution in exactly one case - no command, and o.Executable is the default value.
There was a problem hiding this comment.
i agree if they pass a custom executable they are on their own, but if we tack a "-c" onto their custom executable and break it, we've screwed them and given them no real recourse (well, oc exec). i'll change it back to only enter this block if it's /bin/sh.
|
@smarterclayton updated again, to a more conservative approach (only sets TERM if shell is /bin/sh) |
|
@smarterclayton bump |
|
@smarterclayton since you're in review mode.......bump |
|
@smarterclayton bump. |
|
Test case? :) |
|
[test] |
|
|
||
| # test that rsh inherits the TERM variable by default | ||
| oldterm=$TERM | ||
| TERM=test_terminal |
There was a problem hiding this comment.
Put this on each line.
There was a problem hiding this comment.
I.e. inside the test case
also set it without using env so it will work in containers that don't have /bin/env
|
LGTM [merge] |
|
Evaluated for origin merge up to 8afb300 |
|
Evaluated for origin test up to 8afb300 |
|
continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/12928/) (Base Commit: 68b25d5) (Image: devenv-rhel7_5693) |
|
continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/12926/) (Base Commit: a3e2498) |
also set it without using env so it will work in containers
that don't have /bin/env
fixes #12348