Updated docs

Co-Authored-By: Sagi Shnaidman <sshnaidm@redhat.com>

Change-Id: Ib94adb1c6d6237800db13b3cc243e0897aa6a49f

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?