catching typos

[dseomn]

Consider this example:

$ cat salt/file/typo_test.sls 
typo_test:
  test.nop:
  - requires:  # should be 'require'
    - does not exist
$ salt-ssh [REDACTED] state.apply typo_test testing=true  # should be test=true
[REDACTED]:

Summary for [REDACTED]
------------
Succeeded: 1
Failed:    0
------------
Total states run:     1
Total run time:   0.338 ms

There are two typos, and salt seems to completely ignore both of them. Has anybody thought about changing that so that either one of those would cause an error, or at least a warning?

I think this is somewhat complicated by all the usage of **kwargs because of all the shared args (like require) that can be parsed in different places. It seems like it would be possible to catch those typos though somehow. Maybe add a decorator to functions that records which arguments each function parses as an attribute of the function, then the glue code could check if there are any leftover args that nothing is using?

[bdrx312]

I think it would be possible to implement something like this, but would probably require a lot of work and changes based on how the salt code is currently written. A large part of the problem stems from the design decision that the generic state and requisite arguments for the state compiler like require are at the same same level of the definition hierarchy as the arguments to the state module functions themselves.
If instead of states being designed to have a structure like this:

state_run_cmd:
  cmd.run:
    - name: "echo test"
    - require:
       - requirement

they had a structure that differentiated the arguments to the function from the arguments to the state compiler something like this:

state_my_pkgs:
  pkg.installed:
    - pkgs:
      - cowsay
  require:
    - requirement

or

state_run_cmd:
  cmd.run:
    - name: "echo test"
    - kwargs:
      - pkgs:
        - cowsay  
    - require:
      - requirement

The implementation could more easily avoid argument name conflicts as well as more easily do validation on the state compiler specific arguments.

With the current structure the other issue with doing validation that extra arguments are not passed is with the encapsulation and separation of logic. The state compiler does not know or care about the arguments that are being passed to the state module functions; it just parses all the ones it knows and cares about and then passes the remaining arguments to state module function. Like you said the issue is with **kwargs since python does not care if you pass extra kwarg arguments that are not actually used. Each execution module would then either have to be required to not use **kwargs in the function signature or do its own validation of the arguments not used if it did.

[dseomn]

A large part of the problem stems from the design decision that the generic state and requisite arguments for the state compiler like require are at the same same level of the definition hierarchy as the arguments to the state module functions themselves.

Completely agreed. I think changing that would be the most theoretically sound fix, but I doubt it's worth the effort at this point.

With the current structure the other issue with doing validation that extra arguments are not passed is with the encapsulation and separation of logic. The state compiler does not know or care about the arguments that are being passed to the state module functions; it just parses all the ones it knows and cares about and then passes the remaining arguments to state module function. Like you said the issue is with **kwargs since python does not care if you pass extra kwarg arguments that are not actually used. Each execution module would then either have to be required to not use **kwargs in the function signature or do its own validation of the arguments not used if it did.

I was thinking of something gradual, to catch typos in the most common states/modules first. Like I'm guessing that catching typos in states.file.managed, states.cmd.run, states.pkg.installed, modules.state.apply, and a handful others could catch maybe 80% of the typos. And I think that would be a lot less work than trying to solve the problem everywhere? E.g., if we could change salt/states/file.py from

def managed(..., **kwargs):

to

@handles_arguments('name', 'source', 'source_hash', ...)
def managed(..., **kwargs):

then also change the code that calls it to catch any arguments that aren't part of either the requisite system or the args to @handles_arguments, then I think that would give us a lot of the benefit for a lot less work than fixing everything. If the function isn't decorated with @handles_arguments, then it would work the same as today. Though I'm not a fan of duplicating the argument list between the function signature and the decorator. https://docs.python.org/3/library/inspect.html#introspecting-callables-with-the-signature-object would avoid that, but I could see that getting messy.

[bdrx312]

I think the handles_arguments would not be necessary if the function like managed in your example was just changed to explicitly enumerate the arguments to the function instead of using **kwargs and then the python TypeError exception could be caught and handled.

[dseomn]

Huh, I think you're right. For some reason I thought they all included **kwargs because the requisite args were passed to them and including **kwargs kept them from breaking when new requisites were added. But from a quick test below it looks like they only get their own args, not the global ones. In that case, why are so many functions defined with **kwargs in the first place? That seems like an easy fix.

$ cat salt/file/_states/kwargs_test.py 
def foo(name, **kwargs):
    return {
        "name": name,
        "changes": {},
        "result": False,
        "comment": repr(kwargs),
    }
$ cat salt/file/kwargs_test.sls 
other_state:
  test.nop: []

my_state_id:
  kwargs_test.foo:
  - quux: baz
  - require:
    - other_state
$ salt-ssh [REDACTED] state.apply kwargs_test
[REDACTED]:
----------
          ID: my_state_id
    Function: kwargs_test.foo
      Result: False
     Comment: {'quux': 'baz'}
     Started: 23:28:18.719106
    Duration: 0.331 ms
     Changes:   

Summary for [REDACTED]
------------
Succeeded: 1
Failed:    1
------------
Total states run:     2
Total run time:   0.613 ms

[max-arnold]

Some typos can be spotted by using a syntax highlighter https://salt.tips/text-editor-plugins-for-salt-states-and-yaml-jinja/

More complex checks can be addressed with a linter (maybe even with a custom check) https://salt-lint.readthedocs.io/en/latest/