Contributing to Ansible checklist

[cben]

https://docs.ansible.com/ansible/dev_guide/developing_modules_in_groups.html#before-you-start-coding

• pass pep8 --ignore=E402 --max-line-length=160 cleanly

• support Python 2.6 and Python 3.5+

  • I believe we use 2.7 dict comprehension syntax

• GPLv3 license => Add GNU General Public License V3 license #60

• Shared code can be placed into lib/ansible/module_utils/ (under BSD license) — will this apply as soon as we extract shared code (Share MIQ API boilerplate across modules #36)?

• Shared documentation (for example describing common arguments) can be placed in lib/ansible/utils/module_docs_fragments/— nice!

• With great power comes a duty to keep modules up to date

• Although not required, unit and/or integration tests are strongly recommended

  • we don't have any integration tests (against an actual MIQ). [WIP] Connected ManageIQ + Openshift containers in one playbook #38 could be a start...

• Naming Convention — I think this will apply when we actually contribute into ansible repo?

https://docs.ansible.com/ansible/dev_guide/developing_modules_checklist.html

summary of top-level bold bullets:

• The shebang must always be #!/usr/bin/python. This allows ansible_python_interpreter to work

• Modules must have ANSIBLE_METADATA section

• Documentation: Make sure it exists — we have it, but there is looong checklist there...

  • Verify that a GPL 3 License header is included.

• Predictable user interface — IMHO we're not bad on guidelines there

• Informative responses

  • Please note, that for >= 2.0, it is required that return data to be documented.
  • Return diff if in diff mode.

• Code — we're not bad but:

  • Modules should not do the job of other modules, that is what roles are for. Less magic is more.
    • manageiq_provider + validate + refresh perhaps violates this
  • Support check mode.

• Exceptions: The module must handle them.

• then follows flat soup of many more things:

  • The module must not use sys.exit() –> use fail_json() from the module object.

  • Import custom packages in try/except and handled with fail_json() in main() e.g.

  • The return structure should be consistent, even if NA/None are used for keys normally returned under other options.

  • Are module actions idempotent? If not document in the descriptions or the notes. — we're striving, but some modules have "complicated idempotency" worth documenting

  • Import ansible.module_utils code in the same place as you import other libraries. In older code, this was done at the bottom of the file but that’s no longer needed.

  • Do not use wildcards for importing other python modules (ex: from ansible.module_utils.basic import *).

  • The module must have a main function that wraps the normal execution.

  • Call your main() from a conditional so that it would be possible to import them into unittests

  • Try to normalize parameters with other modules, you can have aliases for when user is more familiar with underlying API name for the option — we're trying

  • Being pep8 compliant is nice, but not a requirement.

  • Avoid ‘action/command‘, they are imperative and not declarative, there are other ways to express the same thing

  • Do not add list or info state options to an existing module - create a new _facts module.

    • facts modules must return facts in the ansible_facts field of the result dictionary
    • modules that are purely about fact gathering need to implement check_mode.

  • Return values must be able to be serialized as json via the python stdlib json library.

  • When fetching URLs, please use either fetch_url or open_url from ansible.module_utils.urls rather than urllib2; urllib2 does not natively verify TLS certificates and so is insecure for https.

    • I wonder how API client libs are normally handled? We're so far on the border, we could drop the lib and use HTTP correctly with not much effort. But I hope they're not flat against using libs...

  • Basic auth: module_utils.api has some helpers ... — not applicable

    • ... may want to fallback on environment variables for default values — we do.

• Windows modules checklist — irrelevant

[cben]

This is outdated, we've started contributing to ansible/ansible, manageiq_user landed (ansible/ansible#26641).

cc @yaacov in case you haven't seen this and is still useful...

[yaacov]

@cben 👍

a. thanks
b. TL;DR :-)

[cben]

Closing as irrelevant, some of this has happened, some hasn't, but most modules are already upstreamed (#85).
cc @elad661 in case this still helps you.