vultr.cloud

[resmo]

Ansible Galaxy: https://galaxy.ansible.com/vultr/cloud
GitHub: https://github.com/vultr/ansible-collection-vultr
Issues Tracker: https://github.com/vultr/ansible-collection-vultr
GitHub Handles: @resmo @Oogy @ddymko
Is the collection part of Automation Hub: No
We meet Collection Requirements: Yes

Successor of ngine_io.vultr which uses the deprecated v1 Vultr API.

[Andersson007]

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
  MUST FIX: I see 1.0.0-rc
• has a policy of releasing, versioning and deprecation announced to contributors and users in some way
  MUST FIX: Failed to find it. If i missed it somehow, could you please point me to the source. If it doesn't exist, as an example Releasing, Versioning and Deprecation community.general#582 can be used.
• 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
  COMMENT: It looks like the collection follows semver but as i can't see the releasing policy where it's explicitly said, it can be not obvious for contributors / users (sorry if i missed the policy somehow, please point out)
• follows licensing rules
• follows the Ansible documentation standards and the style guide
  MUST FIX: Use U() with an URL in the doc fragment
  COMMENT: There's inconsistency in ret val descriptions as some of them end with a period and some don't. Would be nice to fix. The standard way is to end them with a period (took it from the doc guidelines). Be careful, descriptions of EXAMPLES shouldn't end with a period, so the examples are fine.
• 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)
  Question: Which Python version the collection supports (i would suggest having it in README)? In the requirements is said that "In the other environment, collections MUST support Python 2 (version 2.7) and Python 3 (Version 3.6 and higher), unless required libraries do not support these versions. Collections SHOULD also support Python v2.6 and v3.5 if all required libraries support this version."
• 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
• modules (or plugins) from ansible-core use ansible.builtin. as a FQCN prefix
  MUST FIX there are many cases of using debug (in particular in info modules) that don't use ansible.builtin FQCN
• 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
• public plugins, roles and playbooks do not use files outside of meta/, plugins/, roles/, and playbooks/

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
• collection repository should not contain any large objects (binaries) comparatively to the current Galaxy tarball size limit of 20 MB. For example, package installers for testing purposes shouldn't be added.
• collection repository should not contain any unnecessary files like, for example, temporary files created by development tools. Temporary files MUST be added to .gitignore.

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
  MUST FIX: Sanity and integration tests don't run against the release commit, only the workflow called Deploy
• all CI tests run regularly (nightly, or at least once per week)
  MUST FIX only sanity tests run daily (i'm not sure if sanity only is enough)

[Andersson007]

Also releasing is a manual task (but executed by github CI) - do i understand correctly that you run the WF manually against a tag and the WF publishes the collection on Galaxy?
If yes, is it possible to trigger all the tests in the WF and not publish the collection if some tests failed?

@resmo what do you think? (i really wanna see the collection included ASAP, we can do it in a minor release now, i.e. at any time)

[resmo]

@Andersson007
I'm not necessarily against it though, but here are my thoughts on it:

1. cloud integration tests are "expensive": they take a long time, can hit the rate limit, and depend on external resources (API). For some resources, it is necessary to run only one test job because it relies on resources that do not change dynamically and therefore the test would fail.
2. our CI is comprehensive and has ~90% percent coverage. Other collection do not test as comprehensively, thus can meet the policy more easily. Some collections have no insight into how and if they run the tests (e.g. amazon.aws).
3. creating dependencies between CI workflows in Github are not "easy" to do and error prone.
4. a human can better assess if a failed CI is relevant to blocking a release.

I wished this would be a soft policy.

[Andersson007]

@resmo thanks for explaining! I think we should change the inclusion policy but it'll take time, I'll create a topic ASAP.
For now, is it possible to run only sanity tests in that job?
The policy says all but from the context of the doc sanity tests are required, integration and units are optional.
What do you think?

[resmo]

@Andersson007 yes, sanity only would be fine! Can do that.

[Andersson007]

@resmo cool, please let me know when it's done. Nothing else from me, besides that LGTM.

@felixfontein feel free to review the collection, I'll change the category.

[Andersson007]

@resmo hello! Thank you for submitting the collection!
I've just finished the first round of review and have found several things to improve (see the checklist above).
Could you please take a look?
Thank you!

[resmo]

@Andersson007 @felixfontein v1.0.0 has been released.

[resmo]

Also see ansible-community/ansible-build-data#128

[Andersson007]

hi @resmo ! The process is a bit more complex, see the Inclusion WF.
Briefly:

1. I'll check my review once more
2. Then I'll ask the Committee to conduct the second review
3. If the second reviewer says that everything is fine, we'll create a corresponding inclusion topic
4. If there are no objections from the Committee, the inclusion decision will be considered made in a certain date (usually several days after the topic is created)

[Andersson007]

@resmo

@Andersson007 yes, sanity only would be fine! Can do that.

cool, please let me know when it's done. Nothing else from me, besides that LGTM.

@felixfontein feel free to review the collection (IIRC you said that you were interested), I've changed the category.

[mariolenz]

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.

Note for reviewers: If you don't know how to check any of the points below, please ask maintainers of the collection you're reviewing or a Steering Committee member for clarifications in comments of corresponding inclusion discussion.

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
  COMMENT: Maybe you should add more details, like a roadmap or that you're releasing a new major version for every new ansible-core major version. (Background: When a new ansible-core version is released, an old version will go EOL. But you cannot drop support for an old ansible-core version without releasing a new major version because this is considered a breaking change.)
• have a Code of Conduct (CoC) compatible with the Ansible Code of Conduct (CoC)
  COMMENT: Since you're mentioning the Ansible Code of Conduct, it might help contributors if you link to it.
• 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)
• 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
• 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
• public plugins, roles and playbooks do not use files outside of meta/, plugins/, roles/, and playbooks/

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
• collection repository should not contain any large objects (binaries) comparatively to the current Galaxy tarball size limit of 20 MB. For example, package installers for testing purposes shouldn't be added.
• collection repository should not contain any unnecessary files like, for example, temporary files created by development tools. Temporary files MUST be added to .gitignore.
  COMMENT: I didn't see any, but it might be a good idea to add some more stuff to .gitignore to avoid this happening in the future. For example *.py[cod]. See the collection_template for ideas. Although there are a lot of things you probably won't need and I'm missing some IDE specific things there, it's a good starting point.

Tests:

Note for reviewers: If you don't know how to check the points below, please ask maintainers of the collection you're reviewing how you can do it.

• In most cases, CI is set up via GitHub Actions.
• Check .yml files in the .github/workflows directory. There must be at least the sanity section under jobs containing the ansible-test sanity command running against all supported ansible-core versions that must be also listed there, for example, - stable-2.11, - stable-2.12, - stable-2.13.
• Check workflow runs by clicking the Actions tab in the repository's page - you're interested in Scheduled runs, runs against release commits and runs against pull requests.
• If there are no workflows in the Actions tab, ask the collection maintainers how CI is implemented.

• passed ansible-test sanity
• if test/sanity/ignore*.txt exists, it MUST 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 MUST run against every pull request
• all CI tests MUST run regularly (nightly, or at least once per week)
• sanity tests MUST run against a commit that releases the collection; if they don't pass, the collection won't be released

[mariolenz]

@Andersson007 @felixfontein I'm still trying to get the hang on reviewing on inclusion requests. Could you please have a look at this:

• has a policy of releasing, versioning and deprecation announced to contributors and users in some way
  COMMENT: Maybe you should add more details, like a roadmap or that you're releasing a new major version for every new ansible-core major version. (Background: When a new ansible-core version is released, an old version will go EOL. But you cannot drop support for an old ansible-core version without releasing a new major version because this is considered a breaking change.)
• have a Code of Conduct (CoC) compatible with the Ansible Code of Conduct (CoC)
  COMMENT: Since you're mentioning the Ansible Code of Conduct, it might help contributors if you link to it.

I don't think this is a MUST, but is it OK to comment on this like I did?

[felixfontein]

Yes, that's totally fine!

[felixfontein]

Regarding major releases: I would make them less frequent than ansible-core major releases, but the question of how many ansible-core versions a collection should support (to avoid CI overload) is something we haven't found a good solution for yet...

[felixfontein]

I tested the docs build for this collection, it passed without Sphinx warnings. That's good and makes @samccann happy :)

Some quick docs notes:

1. short_description for the modules ends with a period, though it shouldn't
2. Avoid type: complex for return values. Use type: dict or type: list with elements: dict instead. This helps users a lot more than type: complex.

[felixfontein]

I took another look at the collection, this time in more detail, and I didn't found anything else than the two above points. And I also agree that linking to the Ansible Code of Conduct is a good idea :)

[Andersson007]

I re-checked, everything is fixed

[mariolenz]

@Andersson007 @felixfontein I'm not really sure what to do with this inclusion request. vultr.cloud seems to have only one person working on it. It looks like a one-man-show to me.

I don't want to say that @resmo is doing a bad job. But having only one maintainer (or, in this case, even one contributor) looks like a risk to to me.

Additionally, there haven't been any new releases since May 26. And, as far as I can see, only one commit...

Maybe there haven't been any feature requests or bug reports because people don't use this collection since it's not part of the community package. But, anyway, there's still the problem that only one person is working in this collection.

[ddymko]

I am no longer a part of Vultr however there is a team dedicated to maintaining the open source organization.

CC @optik-aper

[felixfontein]

That's a good question. We did add collections with only one maintainer in the past (community.dns with me as maintainer). And right now we have multiple collections included with zero maintainers (community.azure, community.google, google.cloud, maybe some more). I don't think we have a criterion for number of active maintainers, but I think it depends a lot on what we see in the repo, and whether we know the persons involved. In this case, I'm pretty sure that @resmo will do a good job (based on his work in ansible/ansible - he had commit rights there for a long time - and my personal experience). Obviously things can always change, but we finally have some rules for that by now.

For this specific collection there also seems to be some involvement from Vultr as it seems (looking at the list in the first post in this discussion), so I think this should be no problem anyway.

[optik-aper]

@resmo @ddymko Thanks for all the work on this so far--I'm happy to get involved and help out wherever makes sense. I imagine most of the concern is with future issues coming from the community. I'm not good at predicting the future but I can at least say I'll be checking when issues come in and I or someone else on the team will be there to help resolve them. Additionally, it'd be good to loop the project in when API changes happen so that the latest options are present in the package.

@mariolenz I'm part of the team at Vultr that maintains the open source projects as @ddymko noted. What do we need to get in place to see this through? Are you only waiting on the "COMMENT" items above? Thanks!

[resmo]

Thanks @felixfontein for the words, and @ddymko for your support and @optik-aper for the commitment.

Let me point out a few things:

1. This collection is a re-implementation for an existing collection currently in ansible package which uses the deprecated vultr v1 API. It is clear the users use the current available one. The v1 collection is currently maintained by ... one person ... me (I didn't find a way yet to clone me).
2. The new collection is under the hands of vultr under their "name" gh/vultr. This is a commitment that the company will take care. we can not see this commitment of the "community" for collections located in gh/ansible-collections.
3. community don't magically appear, having one maintainer being around and reactive is better then 3 no-showing-ups only maintain on paper.
4. Some words about "not much change": cloud APIs usually don't change much, once their are finalized. Having cloud modules and collections with no changes and issues doesn't necessary mean, they are not used. I know many happy users. Further having a ~90% test coverage proves the collections work as intended.

Thanks for your time and understanding.

[mariolenz]

Thanks for the clarification @felixfontein!

In this case, I'm pretty sure that @resmo will do a good job (based on his work in ansible/ansible - he had commit rights there for a long time - and my personal experience).

Don't get me wrong, I didn't want to doubt this.

Thanks @resmo for the things you've pointed out!

I'm sorry, I didn't want to cause any confusion. But this is the first (or second?) time I'm doing a review and didn't want to go wrong.

No objections from my side, although I'm not sure if this comment was meant as a MUST fix or just a recommendation. Everything else seems to be cleared up, I think.

Again: Sorry if I caused any confusion.

[Andersson007]

@resmo hi, how about

• vultr.cloud #44 (reply in thread)
• vultr.cloud #44 (comment)
  I don't see any other things to change, so when they are done, we could proceed with the inclusion

[resmo]

@Andersson007 @mariolenz @felixfontein I addressed the remaining issues and releases 1.0.1 of the collections. Let me know if I missed anything to get it included.

[felixfontein]

@resmo you now have some type: list return values that do not have elements: ... :) Would be great if you could also fix that, but I'm happy if this happens in the next release (after inclusion).

[Andersson007]

• 1 for inclusion

[mariolenz]

+1 for inclusion

[Andersson007]

Thanks folks! I created a topic ansible-community/community-topics#137.
If no objections, the collection will be considered included 2022-09-13.

[Andersson007]

Included, thanks everyone! Will appear in the package with the next Ansible 6 minor release