-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
Use python to expand the cwd instead of environment variables #4882
Conversation
@@ -218,7 +218,7 @@ def setup_base(self): | |||
# Don't use virtualenv bin that doesn't exist yet | |||
bin_path=None, | |||
# Don't use the project's root, some config files can interfere | |||
cwd='$HOME', | |||
cwd=os.path.expanduser("~"), |
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 believe this will pass to the docker container the home from outside rather than the one inside docker
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 wonder if we should just do /
, since it doesn't matter what the cwd is, but we just don't want it to be the users directory?
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.
That probably will raise some permission issues #4808 (comment)
I think we just need to edit the LocalBuilder class to allow shell expansions in cwd
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.
Good call, updated.
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.
Hrm, this still will break on docker though, eh?
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 we do the expansion inside the run
method it shouldn't, docker overrides all the run command I think
Codecov Report
@@ Coverage Diff @@
## master #4882 +/- ##
=======================================
Coverage 76.64% 76.64%
=======================================
Files 158 158
Lines 10054 10054
Branches 1271 1271
=======================================
Hits 7706 7706
Misses 2007 2007
Partials 341 341
|
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.
This did fix the issue in my testing.
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.
Tested this, it doesn't break docker
No description provided.