reviewing.md

Reviews

How to do a review? What to look for when reviewing patches?

• Should functionality be implemented in Ansible modules or in openstacksdk? Ansible modules should only be "wrappers" for functionality in openstacksdk. Big code chunks are a good indicator that functionality should better be moved to openstacksdk.
• For each function call(s) and code section which has been refactored, does the new code return the same results? Pay special attention whenever a function from openstacksdk's cloud layer has been replaced because those functions often have different semantics than functions of SDK's proxy layer.
• Can API calls (to OpenStack API, not openstacksdk API) be reduced any further to improve performance?
• Can calls to OpenStack API be tweaked to return less data? For example, listing calls such as image.images() or network.networks() provide filters to reduce the number of returned values.
• Sanity check argument_spec and module_kwargs. Some modules try to be clever and add checks to fail early instead of letting openstacksdk or OpenStack API handle incompatible arguments.
• Are choices in module attributes apropriate? Sometimes it makes sense to get rid of the choices because the choices are simply to narrow and might soon be outdated again.
• Are choices in module attributes still valid? Module code might be written long ago and thus the choices might be horrible outdated.
• Does a module use name as module options for resource names instead of e.g. port in port module? Rename those attributes to name to be consistent with other modules and with openstacksdk. When refactoring a module, then add the old attribute as an alias to keep backward compatibility.
• Does the module have integration tests in ci/roles?
• Is documentation in DOCUMENTATION, RETURN and EXAMPLES up to date?
• Does RETURN list all values which are returned by the module?
• Are descriptions, keys, names, types etc. in RETURN up to date and sorted?
  • For example, type: complex often can be changed to type: list / elements: dict.
  • returned: always, but can be null often has to be changed to returned: always, but can be empty or shorter returned: always.
  • Are there any values in RETURN which are not returned by OpenStack SDK any longer?
  • Module return value documentation can be found in OpenStack SDK docs, e.g. Identity v3 API. For more detailed descriptions on return values refer to OpenStack API.
• Do integration tests have assertions of module's return values?
• Does RETURN documentation and assertions in integration tests match?
• Does RETURN documentation and self.exit_json() statements match?
• Do all modules use to_dict(computed=False) before returning values?
• Because id is already part of most resource dictionaries returned from modules, we can safely drop dedicated id attributes in self.exit_json() calls. We will not loose data and we break backward compatibility anyway.
• Is EXAMPLES documentation up to date? When module arguments have been changed, examples have to be updated as well.
• Do integration tests execute successfully in your local dev environment?
  Example:

  ansible-playbook -vvv ci/run-collection.yml \
      -e "sdk_version=1.0.0 cloud=devstack-admin cloud_alt=devstack-alt" \
      --tags floating_ip_info

• Does a patch remove any functionality or break backwards compatibility? The author must give a good explanation for both.
  • One valid reason is that a functionality has never worked before.
  • Not a valid reason for dropping functionality or backwards compatibility is that functions from openstacksdk's proxy layer do not support the functionality from openstacksdk's cloud layer. SDK's cloud layer is not going away and can be used for functionality which openstacksdk's proxy layer does not support. For example, list_* functions from openstacksdk's cloud layer such as search_users() allow to filter retrieved results with function parameter filters. openstacksdk's proxy layer does not provide an equivalent and thus the use of search_users() is perfectly fine.
• Try to look at the patch from user perspective:
  • Will users understand and approve the change(s)?
  • Will the patch break their code? Note: For operators / administrators, a stable and reliable and bug free API is more important than the number of features.
  • If a change breaks or changes the behavior of their code, will it be easy to spot the difference?