mirror of
https://opendev.org/openstack/ansible-collections-openstack.git
synced 2026-07-25 09:14:31 +00:00
6ec9548850
At the moment project documentation does not contain any mention on how changelog should be produced. Some use reno, some use fragments. At the moment there is no transition from reno to antsibull format is available, so let's ask contributors to follow ansible's format of changelog fragments. In case we come up with a transition layer from reno to fragment format, or antsibull replacement, we can change documentation. Change-Id: I048f80af849d2872be866e44882d2a8fcbf01cfc Signed-off-by: Dmitriy Rabotyagov <dmitriy.rabotyagov@cleura.com>
69 lines
5.0 KiB
Markdown
69 lines
5.0 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`?
|
|
* Does the patch include a changelog fragment? Every new feature, or important bugfix must include one. Ensure that
|
|
the fragment follows the Ansible format and that `reno` is not used.
|
|
* 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?
|