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
network.networks()provide filters to reduce the number of returned values.
- Sanity check
module_kwargs. Some modules try to be clever and add checks to fail early instead of letting
openstacksdkor OpenStack API handle incompatible arguments.
choicesin 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.
choicesin module attributes still valid? Module code might be written long ago and thus the choices might be horrible outdated.
- Does a module use
nameas module options for resource names instead of e.g.
portmodule? Rename those attributes to
nameto 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
- Is documentation in
EXAMPLESup to date?
RETURNlist all values which are returned by the module?
- Are descriptions, keys, names, types etc. in
RETURNup to date and sorted?
- For example,
type: complexoften can be changed to
returned: always, but can be nulloften has to be changed to
returned: always, but can be emptyor shorter
- Are there any values in
RETURNwhich 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.
- For example,
- Do integration tests have assertions of module's return values?
RETURNdocumentation and assertions in integration tests match?
- Do all modules use
to_dict(computed=False)before returning values?
idis already part of most resource dictionaries returned from modules, we can safely drop dedicated
self.exit_json()calls. We will not loose data and we break backward compatibility anyway.
EXAMPLESdocumentation 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?
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
- 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?