tiltfile: allow commands to be specified as arrays#2914
Conversation
| // an array of strings gets interpreted as a raw argv to exec | ||
| func ValueToCmd(v starlark.Value) (model.Cmd, error) { | ||
| switch x := v.(type) { | ||
| case nil: |
There was a problem hiding this comment.
nit: should this be an error?
(Background: I hate null with a passion)
There was a problem hiding this comment.
ya, i think this gets at the problem of doing deserialization outside of unpack, where we can't tell the difference between "the user specified explicit nil" and "the user didn't specify the argument".
i also wonder if the typed nil problem is going to bite us here
There was a problem hiding this comment.
I've added a commente explaining why this is here.
FWIW, from the UnpackArgs docs:
// Beware: an optional *List, *Dict, Callable, Iterable, or Value variable that is
// not assigned is not a valid Starlark Value, so the caller must
// explicitly handle such cases by interpreting nil as None or some
// computed default.
(though ideally that comment would explicitly say that an unspecified optional param is left as nil)
I only added this case after unit tests in which optional params were unspecified failed because this value is nil, which gives me some level of confidence that we're ok on typed nils, and if we upgrade starlark and that behavior changes, unit tests will catch it.
Also, probably beside the point of this PR, but IIUC, users cannot specify explicit nil in starlark. As hinted at by the starlark comment above, nil is not a valid starlark value. If they pass, e.g. serve_cmd=None, then we'll get a starlark.None rather than a nil, which we can distinguish post-deserialization. We've still got this problem if we pass go primitives, e.g. *int or *string to UnpackArgs, but I haven't yet noticed any pain from that.
| func (s *tiltfileState) localResource(thread *starlark.Thread, fn *starlark.Builtin, args starlark.Tuple, kwargs []starlark.Tuple) (starlark.Value, error) { | ||
| var name, updateCmdStr, serveCmdStr string | ||
| var name string | ||
| var updateCmdVal, serveCmdVal starlark.Value |
There was a problem hiding this comment.
i wonder if we'll eventually want a way to add custom type unpackers to unpackArgs
There was a problem hiding this comment.
Yeah, I suspect everything would be a lot nicer if we never had to pass a starlark.Value to UnpackArgs.
| // an array of strings gets interpreted as a raw argv to exec | ||
| func ValueToCmd(v starlark.Value) (model.Cmd, error) { | ||
| switch x := v.(type) { | ||
| case nil: |
There was a problem hiding this comment.
ya, i think this gets at the problem of doing deserialization outside of unpack, where we can't tell the difference between "the user specified explicit nil" and "the user didn't specify the argument".
i also wonder if the typed nil problem is going to bite us here
3 participants
Problem
At the moment, any place a command is specified in the Tiltfile, it's specified as a string, which then gets wrapped in an sh -c '', which forces the user to deal with shell evaluation.
This means that any time a user constructs a command from starlark variables, they potentially have to worry about those starlark variables containing characters with special meaning to the shell, and we provide no way to escape them, so there's not really anything to be done.
Solution
Allow commands to be specified as either shell-interpreted strings or raw argv, Dockerfile-style.
This PR pretty much consists of:
value.ValueToCmdfunction that takes astarlark.Valueand converts thestring|[]stringto amodel.Cmd, as appropriatemodel.ToShellCmdin the tiltfile dir with calls tovalue.ValueToCmd