1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
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?
|
