7d9de2858a
Co-Authored-By: Sagi Shnaidman <sshnaidm@redhat.com> Change-Id: Ib94adb1c6d6237800db13b3cc243e0897aa6a49f
4.8 KiB
4.8 KiB
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()
ornetwork.networks()
provide filters to reduce the number of returned values. - Sanity check
argument_spec
andmodule_kwargs
. Some modules try to be clever and add checks to fail early instead of lettingopenstacksdk
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
inport
module? Rename those attributes toname
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
andEXAMPLES
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 totype: list
/elements: dict
. returned: always, but can be null
often has to be changed toreturned: always, but can be empty
or shorterreturned: 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.
- For example,
- Do integration tests have assertions of module's return values?
- Does
RETURN
documentation and assertions in integration tests match? - Does
RETURN
documentation andself.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 dedicatedid
attributes inself.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 assearch_users()
allow to filter retrieved results with function parameterfilters
. openstacksdk's proxy layer does not provide an equivalent and thus the use ofsearch_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?