saltstack/salt — pull request #5528 by ageron (2013-06-13 21:05:47+00:00)
Title: Upgrade kwargs parsing

Hi,

Here's another set of commits meant to make salt command line arguments and keyword arguments simpler to use.

__Change 1: dictionaries in arguments and keyword argument values *must* be formatted with curly brackets.__

I was frustrated with the fact that the following bombs:

salt '*' cmd.run 'echo "Hello: world"'

First, the master passes the argument string untouched to the minions as ``'echo "Hello: world"'``. But then it is further parsed by the minions into a dictionary ``{'echo "Hello': 'world"'}``.

Ouch. Not at all what you would expect, don't you think?

With the existing argument parsing, you need to escape strings quite often, like this:

salt '*' cmd.run '"echo \"Hello: world\""'

Now if you add kwargs to the mix, it gets ugly:

salt '*' cmd.run '"echo \"Hello: $FIRST_NAME\""' '"env={FIRST_NAME: \"Joe\"}"'

This is really tough to get right. Unnecessarily so, I reckon.

The problem is mostly due to YAML: it considers almost anything as a dictionary when it sees a colon ``:``.

So this pull request adds a new rule: all dictionaries in arguments and kwargs should be formatted with curly brackets.

This greatly simplifies a lot of commands. The one above still works but it can be simplified into this:

salt '*' cmd.run 'echo "Hello: $FIRST_NAME"' 'env={FIRST_NAME: "Joe"}'

It might be a breaking change for some people, I'm afraid. But how much so, I don't know: who actually passes dictionaries as arguments without curly braces (on purpose)? Here's code that won't work anymore:

salt '*' func.expects_dict 'Hello: world'

It should be replaced with this:

salt '*' func.expects_dict '{Hello: world}'

I really think this change will save people a lot of headaches, as colons creep up all over the place and yaml tends to think everything's a dictionary, which leads to escaping strings all the time, and that's just painful.

__Change 2: keyword argument names *must* be valid python identifiers.__

The only constraint on the kwarg name today is that it should not include a space. I made it a bit more strict by only allowing properly formatted python identifiers (take a look at the regex in ``salt/utils/__init__.py``). This will avoid bugs with this kind of calls:

salt '*' some.display hello ====== how are you?

The function would be passed the kwarg ``{"=": "====="}``, most likely a bug.
Here's another similar bug:

salt '*' genius.solve_equation "cos(x)=0.5"

Instead of having argument ``"cos(x)=0.5"``, we end up with kwarg ``"cos(x)"`` with value ``0.5``. Most likely not what you want. So now in these two examples, all arguments will be understood as regular arguments (as strings), not kwargs.

Again, a breaking change... but really, even though it's *possible*, who would actually have used this "functionality" (I'd rather call it a parsing bug actually)? I don't think changing this will actually break anyone's code. Again, I may be wrong.

__Change 3 (implemented but not activated): Kwarg parsing *before* YAML parsing__

Keyword argument parsing is currently done *after* yamlifying the arguments. This means that it is impossible to pass the string ``"a=b"`` as an argument to a function, like this:

salt '*' some.func '"a=b"'

The first round of yaml parsing results in removing the internal double-quotes, so when the kwarg parsing kicks in, it detects an kwarg ``a``... not what's expected.

So I fixed this: there's just one round of parsing now, and kwargs detection is applied first, before yamlifying (yamlifying applies only to arguments and keyword argument-values, not kwarg names). In the process, I removed a few redundancies in the code, so don't be surprised to see a ``parse_arg`` and ``parse_args`` disappear and ``detect_kwargs`` renamed. I hope no-one used them elsewhere (they were purely internal code, from what I can see, not API, so it should be ok).

Yet again it's a breaking change, but this time a bit more severe. Let's go back to the example above:

salt '*' cmd.run '"echo \"Hello: $FIRST_NAME\""' '"env={FIRST_NAME: \"Joe\"}"'

This command will now call ``cmd.run`` with two string arguments, instead of one string and one ``env`` keyword argument dictionary. Yet it used to work, and it was actually the only way to make it work. So this change is fairly severe, people will have to fix a few commands. So for now I deactivated this change in the code: I put ``YAMLIFY_ARGS_BEFORE_PARSING_KWARGS=True`` at the beginning of ``minions.py``. Just set it to ``False`` to enjoy the new parsing.

This may need to be a config file option or something, I don't know yet. Any ideas?

__Change 4: any argument can be passed as a kwarg__

I don't understand why the code limits keyword arguments to just the arguments that have default values. This is not standard python behavior, so it's really confusing and error prone. For example, say there's a function like this:

# some.py
def func(x, y, z=3):
return (x, y, z)

This function can be called in the following ways:

salt '*' some.func 1 2 3 # x=1 y=2 z=3
salt '*' some.func 1 2 z=3 # x=1 y=2 z=3
salt '*' some.func 1 z=3 2 # x=1 y=2 z=3
salt '*' some.func z=3 1 2 # x=1 y=2 z=3
salt '*' some.func 1 2 # x=1 y=2 z=3

So far so good. But the following would not give the expected result:

salt '*' some.func x=1 y=2 z=3 # x="x=1" y="y=2" z=3 !!

This is really weird. It took effort to code, so I'm guessing it might be useful for some reason, but I really don't see what.

So I changed this behavior (and it actually simplifies the code): now all arguments can be passed as keyword arguments, whether they have a default value or not. It's more like python (although there are still a few differences, for example it's possible to pass regular args after kwargs).

__Change 5: added ``test.arg`` and ``test.arg_repr``__

Lastly, to see what arguments are actually passed, I added ``test.arg`` and ``test.arg_repr``. The first one is output as yaml by default, which is pretty but not clear enough when you want to be absolutely sure whether a specific argument was passed as a string or an int, for example. That's why I also added ``arg_repr`` which returns ``{"args": repr(args), "kwargs": repr(kwargs)}``.

__Conclusion__

I have used the new parsing all day long, and it was really pleasant to use. :smile: I hope I didn't go too far, though: how can we make sure that we don't break peoples existing code?