Possible separate lint parser to point out common errors in SLS

[quinnypig]

A user on IRC today reported that they had an incorrect mode in a state file; instead of - mode: 755, they had - dirmode: 775.

This threw no errors even during debug. Is there a simple way to get a config parser to throw anything that isn't a valid Salt module call, or is this a harder problem to tackle?

[basepi]

So the problem in this instance is that the state they were using probably had **kwargs, or supported the dirmode arg. (Probably the former) In addition, the mode arg is not required, as it has a default specified in the state. Thus, there's no way to detect the problem in this instance -- Python just goes with the default for mode, and assumes you know what you're doing. =\

[quinnypig]

I'm wondering if it's nutty to have a lint parser written separately that'll digest the YAML file and throw back syntax errors, non-standard calls that aren't part of Salt by default, etc. It'd be a bit noisy, but it would potentially aid in troubleshooting stupid stuff such as "missing a colon" or "calling mode dirmode by mistake."

[basepi]

Possible. I'll put it in approved for future -- gotta get the bugcount down first! =P

[basepi]

Updated the title, will update the labels and milestones.

[techdragon]

This linter may be better as a separate tool, so it can be easily used by external tools such as git commit hooks for CI, and plugins for Sublime Text, Vim, and emacs.

[HontoNoRoger]

+1 for that.
Would be great to have a linting tool integrated into Eclipse.

[jcmcken]

I would suggest that this be integrated directly into salt as a part of the state compilation, and the issue be given much more priority.

Let's think about an analogy:

What happens when you call a Python function with an incorrect number of arguments? With incorrect keyword arguments? You get a TypeError. Because there's no situation under which such a call should be considered valid, nor is it particularly useful for it to be considered valid. It's an unresolved semantic error.

Similarly, the salt compiler should not just simply ignore invalid state arguments. The user should be informed as soon as possible that something is awry. Otherwise, what's likely to happen is that the salt run succeeds, the user goes on their merry way, only to discover down the road that things are misconfigured.

This has operational ramifications and security ramifications. What if a module sets a default password for some protected resource, and the user mistypes password as passwrd? Now that protected resource is using the default password, and the user is none the wiser, leading to a potential for some kind of security breach.

Now as to an implementation:

I'm currently working on a salt testing framework that performs exactly the type of error checking described in the original issue. I can't share the full source at the moment (sadly), but the general gist of it is the following:

import inspect
import importlib

def find_function_errors(function_name, arguments):
    module_name, func_name = function_name.split('.', 1)
    module = importlib.import_module('salt.states.%s' % module_name)
    func = getattr(module, func_name)
    func_args = inspect.getargspec(func).args
    watcher = getattr(module, 'mod_watch', None)
    watcher_args = []
    if watcher:
        watcher_args = inspect.getargspec(watcher).args
        watcher_args.remove('name') # implicit argument
    for arg in arguments:
        for key, val in arg.iteritems():
            if key not in func_args + watcher_args:
                raise TypeError('invalid argument "%s" for function "%s"' % (key, func_name)) 
        

(I'm relatively new to the salt codebase, so this might be missing some functionality)

And you would use it like this (the arguments to the function loaded from the YAML):

>>> find_function_errors('file.managed', [{'dirmode': '0755'}])
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: invalid argument 'dirmode' for function 'file.managed'
>>> find_function_errors('file.managed', [{'mode': '0755'}])
>>>

[Oloremo]

Having a proper linter is a must-have feature and other CM tools have it. And we need a style check aswell!

I understand that due to the whole "render first" approach that could be hard.