-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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
[Autoscaler] Precisely match docker HOME #12020
Conversation
The current grep will match any env variable keyed by HOME. This will include some unwanted variables like PYTHONHOME, PROJECT_HOME, etc. Depending on the order of the environment variable, the subsequent docker setup command might fail.
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.
LGTM! Thanks for finding/fixing this!
@@ -669,7 +669,7 @@ def _docker_expand_user(self, string, any_char=False): | |||
if user_pos > -1: | |||
if self.home_dir is None: | |||
self.home_dir = self.ssh_command_runner.run( | |||
"docker exec {} env | grep HOME | cut -d'=' -f2".format( | |||
"docker exec {} env | grep ^HOME= | cut -d'=' -f2".format( |
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.
Should we add a fallback option? (Like if $HOME is not set)?
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.
Not sure about that. if HOME isn't set we should probably fail loudly basically it basically means user's env is broken.
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.
many, many commands rely on it
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.
Oh! Well then I'm fine with assuming that it is set :)
@ijrsvt I updated this PR with |
BEAUTIFUL! I'm always amazed by the available shell commands :) |
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.
Looks good--following my comments/questions from the last review!
The current grep will match any env variable keyed by HOME. This will
include some unwanted variables like PYTHONHOME, PROJECT_HOME, etc.
Depending on the order of the environment variable, the subsequent
docker setup command might fail.
Why are these changes needed?
Related issue number
Checks
scripts/format.sh
to lint the changes in this PR.