diff options
Diffstat (limited to 'ansible_collections/openstack/cloud/docs/reviewing.md')
| -rw-r--r-- | ansible_collections/openstack/cloud/docs/reviewing.md | 66 |
1 files changed, 66 insertions, 0 deletions
diff --git a/ansible_collections/openstack/cloud/docs/reviewing.md b/ansible_collections/openstack/cloud/docs/reviewing.md new file mode 100644 index 000000000..75ef25d4d --- /dev/null +++ b/ansible_collections/openstack/cloud/docs/reviewing.md @@ -0,0 +1,66 @@ +# 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? |