Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

limactl: escape only the value of env variables in shell #1501

Merged
merged 1 commit into from
Apr 28, 2023

Conversation

sam-berning
Copy link
Contributor

#1482

Testing

$ ./_output/bin/limactl shell default HI=$ env 
HI=$
...

jandubois
jandubois previously approved these changes Apr 24, 2023
Copy link
Member

@jandubois jandubois left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel somewhat uneasy about this change, as it is just a heuristics that works for some cases, but not others. But I guess it is an improvement over the current status, so we should still apply it.

An obvious (and fixable) case is having = characters in the value part, but I feel that you will always be able to create edge cases that cannot be handled by simple mechanisms like this, e.g.

$ cat foo\$bar=baz
echo baz
$ ./foo\$bar=baz
baz
$ ./_output/bin/limactl shell default ./foo\$bar=baz
/bin/bash: line 1: ./foo=baz: No such file or directory

I think I would like to see support extended to having = signs in the value, but otherwise this is probably complex enough.

cmd/limactl/shell.go Outdated Show resolved Hide resolved
cmd/limactl/shell.go Outdated Show resolved Hide resolved
jandubois
jandubois previously approved these changes Apr 24, 2023
Copy link
Member

@jandubois jandubois left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, LGTM

cmd/limactl/shell.go Outdated Show resolved Hide resolved
Signed-off-by: Sam Berning <bernings@amazon.com>
@AkihiroSuda AkihiroSuda added this to the v0.16.0 (tentative) milestone Apr 25, 2023
@jandubois jandubois merged commit e402848 into lima-vm:master Apr 28, 2023
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants