cloud.common

[jillr]

Ansible Galaxy: https://galaxy.ansible.com/cloud/common
GitHub: https://github.com/ansible-collections/cloud.common
Issues Tracker: https://github.com/ansible-collections/cloud.common/issues
GitHub Handles: @goneri @Akasurde @jillr @abikouo
Is the collection part of Automation Hub: Yes
We meet Collection Requirements: Optimistic Yes (I have to review actually but we should be close) We've got some docs work to do at minimum. Will get that ASAP.

[jillr]

Docs update PR: ansible-collections/cloud.common#71

[tadeboro]

Thank you for your submission, @jillr.

The items in the checklist can be in one of the following states:

• Items with a checkmark and no attached comment are OK and need no intervention from your side.
• Items with a checkmark that contain a comment will not block the inclusion process, but by following the advice given in the comment, the collection's quality can be improved.
• Items with no checkmark and no extra details are waiting for evaluation.
• Items with no checkmark that contain a comment need your attention.

Feel free to ask any questions and provide feedback. Thanks in advance for your understanding and patience.

For details about the following points, refer to the Collection Requirements.

Every comment should say whether the reviewer expects it to be addressed, or whether it's optional.

Public availability and communication:

• published on Ansible Galaxy Ansible Galaxy with version 1.0.0 or later
• has a policy of releasing, versioning and deprecation announced to contributors and users in some way
• have a Code of Conduct (CoC) compatible with the Ansible Code of Conduct (CoC)
• have a publicly available issue tracker that does not require a paid level of service to create an account or view issues
• have a public SCM repository, releases of the collection are tagged in this repository

Standards and documentation:

• adheres to semantic versioning
• follows licensing rules
• follows the Ansible documentation standards and the style guide
• follows development conventions; as well as these other requirements:
  • modules that only gather information are named <something>_info
  • modules that return ansible_facts are named <something>_facts and do not return non-facts
  • other modules must not allow querying information using specific state option values, or similar mechanisms (like state=get or state=query). These features should be moved to <something>_info or <something>_fact modules.
  • check_mode is supported in all _info and _facts modules
• supports Python 2.6 or greater and Python 3.5 or greater. If it does not, read the full guidelines to see if you qualify for an exception and document the unsupported Python versions in the collection README.md and in every module and plugin (or in doc fragments)
  COMMENT: Only supports Python 3.6 because modules built using turbo modules are expected to use aiohttp.
• only uses the allowed plugin types in the plugins/ directory
  NOT SURE WHAT TO DO: While technically the collection only uses allowed plugin types, it does it in an unconventional way. The code in https://github.com/ansible-collections/cloud.common/blob/d43c018190e2d50940050ce5f3d3c70ae663de1a/plugins/module_utils/turbo/server.py#L198-L200 breaks the import rules for module utilities.
• has README.md
• documentation, examples, and return sections use FQCNs for the M(..) format macros when referring to modules
• modules (or plugins) from ansible-core use ansible.builtin. as a FQCN prefix
• documentation and return sections use version_added: containing the collection version for which an option, module or plugin was added
• FQCNs are used for extends_documentation_fragment:, unless you are referring to doc_fragments from ansible-core

Collection management:

• galaxy.yml having tags field set
• collection dependencies must have a lower bound on the version which is at least 1.0.0, and are all part of the ansible package
• meta/runtime.yml defines the minimal version of Ansible which the collection works with
• has changelog, preferably with changelogs/changelog.yaml

Tests:

• passed ansible-test sanity
• if test/sanity/ignore*.txt exists, it does not contain error codes listed here
• has CI tests up and running against each of the "major versions" of ansible-base/ansible-core that the collection supports
• all CI tests run against every pull request
• all CI tests run against a commit that releases the collection; if they don't pass, the collection won't be released
• all CI tests run regularly (nightly, or at least once per week)

[tadeboro]

I did an initial review of the collection and things are in great shape, but there are a few things still missing.

[tadeboro]

From my side, the code is ready for inclusion into the Ansible 5 Python package. But before that can happen, a new minor release needs to happen to include the license changes and fix the semver rules break that happened in 2.0.4.

[goneri]

Hi @tadeboro,

Thank you for your time. Why do we need to run scheduled jobs in CI when we do cross-project gating? We stopped doing scheduled CI jobs of the VMware collections because nobody really care about the results.

[gundalow]

We really would like to see nightly (or worst case weekly) CI runs. These collections have various external dependencies which change from day to day (ansible-core, ansible.netcommon, etc).

There have been on occasions time when Zuul-CI tested collections have broken and nobody has noticed.

We stopped doing scheduled CI jobs of the VMware collections because nobody really care about the results.

If CI fails and nobody cares, then that's a problem

[jillr]

In the case of cloud.common it's a good question though. If we add a weekly vmware_rest job, and that includes cross-project tests of cloud.common, is a separate job definition of just cloud.common also required? This collection only provided utilities for use by other collections.

[gundalow]

In the case of cloud.common it's a good question though. If we add a weekly vmware_rest job, and that includes cross-project tests of cloud.common, is a separate job definition of just cloud.common also required? This collection only provided utilities for use by other collections.

That sounds like a reasonable compromise.

[tadeboro]

I don't particularly appreciate adding exceptions because they usually unnecessarily complicate things (in code and general life scenarios). The rules we have should apply to all collections that want to be part of the Ansible package. During the Ansible 3 and 4 cycles, quite a few collections added scheduled runs to their CIs and no one really complained about this.

That being said, I see why running tests on a schedule might be a bit meaningless here. But I wonder whether it is worth deviating from the general advice when we could solve this issue by adding a weekly CI run to the setup and be done with it.

[jillr]

Jobs have been added here ansible/ansible-zuul-jobs#1111

[goneri]

@gundalow to clarify. We do care about our CI. We used to run some periodical jobs with different vcenter version. Sometime they were failing and since this was not blocking the day to day activity of the developers, the problem was often just unnoticed.
kubernetes.core, community.okd and vmware.vmware_rest CI jobs depend on cloud.common, so I'm confident we will identify any problem quickly.

[felixfontein]

@gundalow to clarify. We do care about our CI. We used to run some periodical jobs with different vcenter version. Sometime they were failing and since this was not blocking the day to day activity of the developers, the problem was often just unnoticed.

You might want to set up a notification system so that if nightly jobs fail, someone responsible is informed (by email) and can take a quick look to judge whether the failure is something that needs fixing, or is just a random occurence (network problems, ...).

I've set up such monitoring for all collections I'm maintaining (with AZP it is easy to set up; with GHA only the person who activated the workflows gets informed, which is not so great...; no idea how this works with Zuul) and it has been very helpful from time to time to identify problems that show up in nightly CI, but have no direct affect on usage (like newly failing sanity tests).

[jillr]

We'll be building dashboards for CI jobs (across all our collections) in Q4 (hopefully). We have a CI vanguard role where one engineer rotates being the CI-responsible person each 2-week sprint, so there's not a single person all the time who owns CI for any one collection. We have a bit of a gap right now where we don't have these tools yet but we are working on it.

[goneri]

I get notification when periodic jobs fail, but since we've all got a lot things to keep track off, we tend to focus on those that are day to day blockers. And a broken CI that prevent the merge of new PR is one of those. This is the reason why I value more cross-project testing.

This being said, I don't want to soon like I will act in bad faith. I will keep an eye on these new periodic jobs :-).

[felixfontein]

Ansible Collections Checklist (short version)

For details about the following points, refer to the Collection Requirements.

Every comment should say whether the reviewer expects it to be addressed, or whether it's optional.

Public availability and communication:

• published on Ansible Galaxy with version 1.0.0 or later
• has a policy of releasing, versioning and deprecation announced to contributors and users in some way
• have a Code of Conduct (CoC) compatible with the Ansible Code of Conduct (CoC)
• have a publicly available issue tracker that does not require a paid level of service to create an account or view issues
• have a public SCM repository, releases of the collection are tagged in this repository

Standards and documentation:

• adheres to semantic versioning
  MUST FIX The collection does not adhere to semantic versioning, since new features are added in bugfix releases: for example, version 2.0.4 adds turbo support for lookup plugins. This should have been a new minor release.
• follows licensing rules
  As @tadeboro pointed out, files in module_utils and plugin_utils don't have copyright headers.
• follows the Ansible documentation standards and the style guide
  SHOULD FIX The example plugins do not use proper sentences for descriptions (other than short_description).
• follows development conventions; as well as these other requirements:
• supports Python 2.6 or greater and Python 3.5 or greater. If it does not, read the full guidelines to see if you qualify for an exception and document the unsupported Python versions in the collection README.md and in every module and plugin (or in doc fragments)
  MUST FIX The README does not mention that Python 3.6 or newer is required.
• only uses the allowed plugin types in the plugins/ directory
• has README.md
• documentation, examples, and return sections use FQCNs for the M(..) format macros when referring to modules
  MUST FIX The turbo_demo module doesn't use FQCN in the examples.
• modules (or plugins) from ansible-core use ansible.builtin. as a FQCN prefix
• documentation and return sections use version_added: containing the collection version for which an option, module or plugin was added
• FQCNs are used for extends_documentation_fragment:, unless you are referring to doc_fragments from ansible-core

IMPORTANT The plugin specific code in plugins/module_utils/turbo/server.py (which imports parts of ansible-core that must not be imported in module_utils) should really have been put into plugins/plugin_utils/turbo/server.py (or somewhere else in plugin_utils). Unfortunately it's too late now (except by adding a breaking change). I still suggest to move the code, and deprecate the compatibility stub left in module_utils.

Collection management:

• galaxy.yml having tags field set
• collection dependencies must have a lower bound on the version which is at least 1.0.0, and are all part of the ansible package
• meta/runtime.yml defines the minimal version of Ansible which the collection works with
• has changelog, preferably with changelogs/changelog.yaml

Tests:

• passed ansible-test sanity
• if test/sanity/ignore*.txt exists, it does not contain error codes listed here
  SHOULD FIX Instead of skipping Python versions for individual files in ignore-2.12.txt, use ansible-test configuration (see https://github.com/ansible/ansible/blob/devel/test/lib/ansible_test/config/config.yml for a template)
• has CI tests up and running against each of the "major versions" of ansible-base/ansible-core that the collection supports
• all CI tests run against every pull request
• all CI tests run against a commit that releases the collection; if they don't pass, the collection won't be released
• all CI tests run regularly (nightly, or at least once per week)

[goneri]

Hi! Thank you both for the review. Is there any inclusion blocker left?

[tadeboro]

Anything that has a MUST FIX comment is a blocker. I must admit that I did not have time to review things for the second time. It is possible that not everything listed needs your attention, but judging by the review from 8 days ago, there are at least a few things that are still blocking the inclusion.

[goneri]

oh, sorry. It's a bit hard to trace what need to be done with the current format.

• copyright: Add BSD headers to the plugins/*_utils files cloud.common#80
• semver: it's unfortunate but indeed we should have pushed the lookup plugins in a 2.1.0 release.
• we don't plan to support anything older than 3.6 anytime soon, the fact the turbo mode only works with 3.6+ is mentioned here: https://github.com/ansible-collections/cloud.common/blob/main/README_ansible_turbo.module.rst#how-can-i-enable-ansibleturbomodule
• FQCN in example: turbo_demo: use FQCN in example cloud.common#81
• the example module description: turob_demo: finish the description sentence with a . cloud.common#82

[felixfontein]

* semver: it's unfortunate but indeed we should have pushed the lookup plugins in a 2.1.0 release.

Are you planning on releasing 2.1.0 soon?

* we don't plan to support anything older than 3.6 anytime soon, the fact the turbo mode only works with 3.6+ is mentioned here: https://github.com/ansible-collections/cloud.common/blob/main/README_ansible_turbo.module.rst#how-can-i-enable-ansibleturbomodule

Since all modules and lookups included in the collection use turbo, this should also be documented in the main README.

[goneri]

I will push a 2.1.0 as soon as we've got ansible-collections/cloud.common#76 merged. The modules of the collections are just some examples.

[goneri]

hum, I would like to resolve ansible-collections/vmware.vmware_rest#262 before a 2.1.0.

[tadeboro]

There are less than two weeks left for collections to get ready for inclusion in Ansible 5 (see roadmap for more details). This candidate still has unresolved issues and is not ready for inclusion right now.

[goneri]

@tadeboro 2.1.0 has been publish last week and address all the problem AFAIK.

[tadeboro]

@tadeboro 2.1.0 has been publish last week and address all the problem AFAIK.

I still see one MUST FIX comment in @felixfontein's review. And a quick look at the readme file confirmed that this is still not resolved. So cloud.common is not ready yet.

[goneri]

I answered. This collection and it's modules are not mean to be used directly, this collection is a dependency (like a library). For now the collection only ship the "turbo mode", but we can later publish some other module_utils. And they may this time support py2.

[goneri]

Better README.md ansible-collections/cloud.common#93

It flatten the two README in one single file and clarify the requirements.

[felixfontein]

Once ansible-collections/cloud.common#93 is merged I'm happy with this collection being included in Ansible 5.

[gundalow]

ansible-collections/cloud.common#93 has been merged

[tadeboro]

The community members accepted the cloud.common collection into Ansible 5 (ansible-community/ansible-build-data#73). Thank you all for your patience and cooperation.