-
Notifications
You must be signed in to change notification settings - Fork 575
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
snap: support "command: foo $ENV_STRING" #3995
Conversation
5637186
to
114293b
Compare
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.
Have a look at my review. It mostly looks good but I have a few ideas for your consideration.
cmd/snap-exec/main.go
Outdated
tmpCmdArgv := strings.Split(cmdAndArgs, " ") | ||
cmd := tmpCmdArgv[0] | ||
|
||
cmdArgs := make([]string, 0, len(tmpCmdArgv[1:])) |
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'd feel better if this was a separate function that can be tested in isolation.
tests/lib/snaps/basic-run/bin/echo
Outdated
@@ -0,0 +1,3 @@ | |||
#!/bin/sh | |||
|
|||
echo $1 |
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.
Nitpick, use "$@"
instead of $1
as that has special expansion rules and lets us use multiple arguments.
@@ -0,0 +1,6 @@ | |||
name: basic-run |
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.
There is a snap called test-snapd-sh
that lets you run sh directly without any fuss. Would it suffice?
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, what I need is a env variable inside the command:
part of the app. I could add that to another test snap of course if you think this is overkill.
@@ -15,9 +15,6 @@ prepare: | | |||
restore: | | |||
rm basic-hooks_1.0_all.snap | |||
|
|||
restore: | |
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.
Nice catch :)
tests/main/snap-run/task.yaml
Outdated
. $TESTSLIB/snaps.sh | ||
install_local basic-run | ||
|
||
restore: | |
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 that we don't need to remove those snaps as install_local
handles that now.
@@ -15,9 +15,6 @@ prepare: | | |||
restore: | |
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 we don't need to remove those manually as install_local
handles that now.
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.
Maybe I'm missing something, but I don't see any cleanup code in install_local() in snaps.sh. It would be very desirable though.
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.
Right, sorry. What I meant was that install_local caches the built snap at a separate location and that we don't have to remove the built snap explicitly.
cmd/snap-exec/main.go
Outdated
// strings.Split() is ok here because we validate all app fields | ||
// and the whitelist is pretty strict (see | ||
// snap/validate.go:appContentWhitelist) | ||
tmpCmdArgv := strings.Split(cmdAndArgs, " ") |
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.
Can you please add a comment to https://github.com/snapcore/snapd/blob/master/snap/validate.go#L168 so that if the regexp there ever contains "
, '
or
` then we need to adjust shell argument parser used here.
cmd/snap-exec/main.go
Outdated
for _, kv := range env { | ||
l := strings.SplitN(kv, "=", 2) | ||
if len(l) == 2 { | ||
if k == l[0] { |
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.
You could use a map to get rid of the quadratic complexity here.
@mvo5 the tests fail on |
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.
Woot, thank you, LGTM
osutil/env.go
Outdated
// EnvMap takes a list of "key=value" strings and transforms them into | ||
// a map. | ||
func EnvMap(env []string) map[string]string { | ||
out := make(map[string]string) |
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.
you might as well make it of size len(env)
func expandEnvCmdArgs(args []string, env map[string]string) []string { | ||
cmdArgs := make([]string, 0, len(args)) | ||
for _, arg := range args { | ||
maybeExpanded := os.Expand(arg, func(k string) string { |
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 need to look into what defining the function inside the for looks like, compared to defining it outside; a priori it makes me nervous :-)
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.
... benchmarking says they're indistinguishable. Fair 'nuf.
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 checking!
Tiny branch that will allow to use environment strings in the "command:" part of meta/snap.yaml. Needs some more tests but pushing now to get the benefit of spread tests (here at the airport that is a bit tricky to run locally with the given network).