define and unit test a standard for execution modules

[jfindlay]

Invoking a standard is fraught with many canonical concerns and disillusionments. The reason I'm going for 'standard' rather than 'best practices', 'conventions', or even 'recommendations' is that exec modules don't really share that much in common, or rather, are very expressively concise in purpose. To reduce user frustration and inconsistency bugs and increase intuitive efficacy, we can over time audit and test exec modules to ensure their common parts are as consistent as can be reasonably done.

Here is what I think is common and how it should be standardized:

1. Execution modules are python files that contain a function named __virtual__, which is used by the loader to determine whether the execution module will be loaded, and a list of functions, which exposes each module's functionality to the user. These functions are the salt minion's low level interface to platform specific and other external systems.
2. __virtual__ should return the module name (__virtualname__ if necessary) if the module can be loaded. Otherwise, it should return a tuple in the form (False, reason) where reason is a string describing why the module cannot be loaded. __virtual__() should not log anything on failure to load the module to keep logging free from irrelevant information. The reason will be provided to the user upon request, when the user attempts to run an unloaded module, for example, if it is returned as described. See this for an example.
3. Each function exposed to the user should implement a single action. Function arguments supply data for that action or modify the action taken, but should not change the action itself.
4. The arguments to the functions exposed to the user should each be documented in the function's docstring with accompanying examples as needed. Each of these functions should have at least one CLI example.
5. The return statements in the functions exposed to the user are free form. If a return is a success condition, __context__['retcode'] should remain 0 and the return should include information on what and why the function was successful. In most cases, the data returned by the utility or library used by the function is sufficient.
6. In a function exposed to the user, if a return statement is a failure or error condition or if a raise statement will be encountered, the execution module function should set __context__['retcode'] before returning or raising as recommended in https://github.com/saltstack/salt/issues/18510#issuecomment-66207072. (This needs to be more explicitly defined.)
7. Exceptions raised should be appropriate to the cause of the exception. See the list of Salt specific exceptions. In particular, CommandExecutionError, CommandNotFoundError, MinionError, PkgParseError, and SaltInvocationError are the most likely errors to be raised by execution modules.
8. Interaction with system utilities should be done with __salt__['cmd.run*'].

[jfindlay]

I see two complementary parts to this standard 1) documentation and 2) unit testing. As usual, @thatch45, @cachedout, @basepi, @UtahDave, @whiteinge, @cro can tell me if I've got anything wrong, or if I'm completely crazy here.

[jfindlay]

1 and 2 should be easy to implement and unit test in a generic way for all exec modules simultaneously. 3 is a code review kind of thing. 4 can possibly be unit tested with the doc builder or in some other way, but each function would need to present the argument documentation in a consistent format, possibly with indicator tokens of some kind other than just whitespace formatting. 5 and 6 will need to be custom unit tested for each exec module. 7 can only be validated by code review. 8 can possibly be linted but will also be caught by code review.

[uvsmtid]

@jfindlay, as discussed these are some JSON output issues: #14725, #25153, #25154. Depending on how they are fixed (centrally through architecture changes or case by case), they may be considered duplicate. If they apply here, they equally apply for #25143.

[jfindlay]

@uvsmtid, the json output is an outputter/returner issue rather than an execution module issue since the execution modules don't really have much control over the output. Would you mind opening a separate issue for it?

[whiteinge]

@jfindlay we can extract structured data about function parameters from docstrings using Sphinx. This would be a wonderful addition to fill out a limitation of the function introspection we already perform. This would be very useful for other parts of Salt as well -- REST API, state validation, etc.

If you would prefer Google-style argument lists that is also an option. We can easily work with either style.

[uvsmtid]

@jfindlay, I'm not sure I understood what should be the separate issue about. If I simply remove my comment above, will it make all of these JSON issues separate from this one?

[jfindlay]

@uvsmtid, sorry for the confusion; nothing needs to be done. This was my fault for not looking at the issues you've filed. Thank you for doing that.

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

If this issue is closed prematurely, please leave a comment and we will gladly reopen the issue.