-
Notifications
You must be signed in to change notification settings - Fork 661
Update logic for shell workdir #40
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
Conversation
Changes to the previous implementation are: * If there are no volume mounts then there is no fallback to the host $HOME directory (because we know it isn't mounted, so no need to display an additional error). * If the --workdir does not exist in the guest, we fallback to the host $PWD first before trying the host $HOME (because it is less surprising to stay in the current directory than to switch to one that is different from the requested one). Signed-off-by: Jan Dubois <[email protected]>
var changeDirCmd string | ||
workDir := clicontext.String("workdir") | ||
if workDir == "" { | ||
changeDirCmd = "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.
What is this? Shouldn't this default to currentDir? (even when there is no mount)
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.
How can currentDir
(from the host) work when there are no mounts? It is unlikely that a directory with the same name exists inside the VM, but even if it does, it will have different content.
I mean, potentially you could be in /tmp
and then change to /tmp
inside the guest, but when does that ever make sense? I think when you don't specify a --workdir
with a VM that has no mounts, you should end up in the home directory inside the VM.
So false
is just something that returns a non-zero exit code, because when there are mounts, the command becomes:
false || cd currentDir || cd homeDir
We must have something on the left side of the ||
that fails (but doesn't show an error on stderr), so the cd currentDir
will be tried next.
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.
Thanks for explanation. After merging this, I'll add logrus.Debug for better readability
…vmnet' is owned by someone else)' CI was failing due to the recent breaking change of git > Cloning into 'vde_vmnet'... > HEAD is now at a4b489e Merge pull request lima-vm#40 from AkihiroSuda/dev > git submodule update --init > fatal: unsafe repository ('/Users/runner/work/lima/lima/vde_vmnet' is owned by someone else) > To add an exception for this directory, call: > > git config --global --add safe.directory /Users/runner/work/lima/lima/vde_vmnet > make: *** [install.vde-2] Error 128 Signed-off-by: Akihiro Suda <[email protected]> Signed-off-by: Akihiro Suda <[email protected]>
…vmnet' is owned by someone else)' CI was failing due to the recent breaking change of git > Cloning into 'vde_vmnet'... > HEAD is now at a4b489e Merge pull request lima-vm#40 from AkihiroSuda/dev > git submodule update --init > fatal: unsafe repository ('/Users/runner/work/lima/lima/vde_vmnet' is owned by someone else) > To add an exception for this directory, call: > > git config --global --add safe.directory /Users/runner/work/lima/lima/vde_vmnet > make: *** [install.vde-2] Error 128 Signed-off-by: Akihiro Suda <[email protected]>
…vmnet' is owned by someone else)' CI was failing due to the recent breaking change of git > Cloning into 'vde_vmnet'... > HEAD is now at a4b489e Merge pull request lima-vm#40 from AkihiroSuda/dev > git submodule update --init > fatal: unsafe repository ('/Users/runner/work/lima/lima/vde_vmnet' is owned by someone else) > To add an exception for this directory, call: > > git config --global --add safe.directory /Users/runner/work/lima/lima/vde_vmnet > make: *** [install.vde-2] Error 128 Signed-off-by: Akihiro Suda <[email protected]>
…vmnet' is owned by someone else)' CI was failing due to the recent breaking change of git > Cloning into 'vde_vmnet'... > HEAD is now at a4b489e Merge pull request lima-vm#40 from AkihiroSuda/dev > git submodule update --init > fatal: unsafe repository ('/Users/runner/work/lima/lima/vde_vmnet' is owned by someone else) > To add an exception for this directory, call: > > git config --global --add safe.directory /Users/runner/work/lima/lima/vde_vmnet > make: *** [install.vde-2] Error 128 Signed-off-by: Akihiro Suda <[email protected]>
Changes to the previous implementation are:
If there are no volume mounts then there is no fallback to the host
$HOME
directory (because we know it isn't mounted, so no need to display an additional error).If the
--workdir
does not exist in the guest, we fallback to the host$PWD
first before trying the host$HOME
(because it is less surprising to stay in the current directory than to switch to one that is different from the requested one).Testing with no mounts
No error message when
--workdir
is not specified, and only a single error message when the requested workdir does not exist:Testing with a
/tmp
mount, but no home directory mountFalls back to the host
$PWD
and$HOME
. Uses guest$HOME
when none of them exist: