7d9de2858a
Co-Authored-By: Sagi Shnaidman <sshnaidm@redhat.com> Change-Id: Ib94adb1c6d6237800db13b3cc243e0897aa6a49f
67 lines
4.8 KiB
Markdown
67 lines
4.8 KiB
Markdown
# 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`](
|
|
https://docs.ansible.com/ansible/latest/dev_guide/developing_modules_documenting.html).
|
|
- `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](
|
|
https://docs.openstack.org/openstacksdk/latest/), e.g. [Identity v3 API](
|
|
https://docs.openstack.org/openstacksdk/latest/user/proxies/identity_v3.html).
|
|
For more detailed descriptions on return values refer to [OpenStack API](https://docs.openstack.org/api-ref/).
|
|
* 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:
|
|
```sh
|
|
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](
|
|
https://meetings.opendev.org/irclogs/%23openstack-sdks/%23openstack-sdks.2022-04-27.log.html) 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?
|