Hi
I would like to share some infos about what I look for in modules, or what I learned, what does make a module a good module.
It resulted in some kind of checklist for reviewers or module developers, maybe it helps for others or could be extended and formated for docs.ansible.com:
* Documentation
* available?
* Remove unnecessary doc like `aliases: `?
* `default: null for `required: true` is not necessary, removed?
* version not a float number and value the current development
version?
* arguments camelCase snake_case mixed? --> Prefer snake_case.
* arguments in doc and dict identical?
* password / secret arguments no_log=True set?
* Requirements documented?
* Author set? New format for extras repo?
* Made use of U() for urls, C() for files and options, I() for params, M() for modules?
* GPL License header?
* Examples
* available?
* reproducible?
* Module use check_mode? Could it be modified to use it? Documented?
* Module handles exception? (exceptions are bugs)
* Ensure module not uses sys.exit() --> fail_json() from util
* import custom packages in try/except and handled with fail_json() in main() e.g.
try:
import foo
HAS_LIB=True
except:
HAS_LIB=False
* Are module actions idempotent? If not --> Documented in notes?
* import module snippets `from ansible.module_utils.basic import *`? at the bottom?
Very nice, this is very helpful, I'll ping the team and probably add
this to the development docs.
also, I would add:
- shebang should always be #!/usr/bin/python
- required should always be present, be it true or false
- if required is false you need to document default, even if its 'null'
- as stated, uncaught exceptions are bugs, give out useful messages on
what you were doing and you can add the exception message to that.
- avoid catchall exceptions, they are not very useful unless the
underlying API gives very good error messages pertaining the attempted
action.
- try to normalize parameters with other modules, you can have aliases
for when user is more familiar with underlying API name for the option
- pep8 is nice, but not a requirement. Specifically, the 80 column
limit now hinders readability more that it improves it (even when
reading code on my phone)
- avoid 'action/command', they are imperative and not declarative,
there are other ways to express the same thing
- sometimes you want to split the module, specially if you are adding
a list/info state, you want a _facts version
- if you are asking 'how can i have a module execute other modules'
... you want to write a role
Thanks Rene. Badly needed and a good start. My hope is that this can turn into a checklist that will allow anyone to do triage on new module submissions.
Perhaps a couple of these can be implemented as tests?
Please add a check for documenting module return data. That functionality was added recently and is extremely helpful for designing playbooks.
Perhaps someone would be willing to put together a first cut of this as a PR to the contrib.md for Extras?
Hello Brian,
I'm working on an iDRAC module and using 'action' for the wsman action to be taken on the iDRAC. I was eventually planning on submitting the module for inclusion in extras. Would the fact that I am using 'action' in this way prevent the module from being included?
Thanks,
Hank
Hi Hank,
We try to avoid having imperative language in the options, we prefer a
declarative syntax, we do have modules that violate this rule but we
would like to start enforcing it better now. That said, if you can
make a good case why it should stay 'action' we are willing to listen.
In the end, the change to the docs and spec is normally trivial.