inspur.sm

[ISIB-Group]

Ansible Galaxy: https://galaxy.ansible.com/inspur/sm
GitHub repo: https://github.com/ISIB-Group/inspur.sm
GitHub repo: https://github.com/ISIB-Group/inspur.sm/issues
Automation Hub: https://cloud.redhat.com/ansible/automation-hub/repo/published/inspur/sm
Contacts: @ISIB-Group

I think our collection should meet the collection requirements of the collection.

[felixfontein]

There are some issues which must be addressed. The most serious is about check mode. All modules claim they support check mode, but nowhere module.check_mode is used - the modules seem to do the same action when run with or without check mode.

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
  • Comment: I can't find any information on that in the collection repository.
• have a Code of Conduct (CoC) compatible with the Ansible Code of Conduct (CoC)
  • Comment: I can't find any information on that in the collection repository.
• have a publicly available issue tracker that does not require a paid level of service to create an account or view issues

Standards and documentation:

• adheres to semantic versioning
• follows licensing rules
• follows the Ansible documentation standards and the style guide
  • Acceptable, though improvements are possible. For example, in DNS Server3 ipv4 or ipv6 address,Required when I(dns_manual=manual). there should be a . instead of , before Required. And the correct capitalization is IPv4 and IPv6 and not ipv4 and ipv6.
• follows development conventions
  • While the modules claim they support check mode, I do not see where this is actually implemented. It looks like the modules do the same action both with and without check mode!
• has README.md
• documentation, examples, and return use FQCNs for M(..), examples, and seealso subsections
  • Comment: examples use collections: keyword and not FQCN
• modules (or plugins) from ansible-base 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
  • Comment: all content seems to have been added for 0.1.0, which was never released on Galaxy.
• FQCNs is used for extends_documentation_fragment:, unless you are referring to doc_fragments from ansible-base

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
  • Comment: There is no meta/runtime.yml file
• 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
  • Comment: CI is only run for stable-2.10 and devel, but not for stable-2.9, which the collection supports according to documentation.
  • Comment: sanity tests specify --python 3.6 to ansible-test sanity --docker; this MUST be removed so that all sanity tests run!
• 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
  • Comment: It is not clear to me whether this is the case.
• all CI tests run regularly (nightly, or at least once per week)
  • Comment: This seems not to be the case.

[ISIB-Group]

• follows development conventions

  • While the modules claim they support check mode, I do not see where this is actually implemented. It looks like the modules do the same action both with and without check mode!

I have tried to add a handling of the check mode in module_utils/ism.py, please see if it works.

It always returns changed == True. Is this always true for all your modules?

Yes, I don't know if that's OK.

[ISIB-Group]

In check mode, it always returns changed == True;Instead of checking the pattern, it will go to the real implementation.

[felixfontein]

• has a policy of releasing, versioning and deprecation announced to contributors and users in some way

  • Comment: I can't find any information on that in the collection repository.

We have added Release Notes to README. md so that we can publish relevant information through logs.

A releasing/versioning/deprecation policy is more something saying that how often you plan to release (can be anything from "once per month" to "whenever we have new bugfixes and releases"), how often you release new major versions ("no new major release planned (since we don't want to remove anything yet)"), and how long things will be deprecated before removal ("if we want to remove something, we deprecate it at least 6 months before that").

• follows development conventions

  • While the modules claim they support check mode, I do not see where this is actually implemented. It looks like the modules do the same action both with and without check mode!

Regarding check mode, is it to ensure that the system is not modified when running check mode?

Yes, and that the modules try to return the same results as when check mode is not specified. This is of course not always possible, but at least the changed state should come close to running without check mode. If that's too complicated to implement, you could simply say supports_check_mode=False.

• documentation, examples, and return use FQCNs for M(..), examples, and seealso subsections

  • Comment: examples use collections: keyword and not FQCN

We have made modifications to this problem,please check if it is correct.

Looks good!

Collection management:

• meta/runtime.yml defines the minimal version of Ansible which the collection works with

  • Comment: There is no meta/runtime.yml file

Added runtime.yml file to support 2.10, and changed the supported Ansible version information in README.md to support 2.10 only.

The README still mentions 2.9.10, you probably want to replace that by 2.10.0.

Tests:

• has CI tests up and running against each of the "major versions" of ansible-base/ansible-core that the collection supports

  • Comment: CI is only run for stable-2.10 and devel, but not for stable-2.9, which the collection supports according to documentation.
  • Comment: sanity tests specify --python 3.6 to ansible-test sanity --docker; this MUST be removed so that all sanity tests run!

Modify the ansible-test.yml file to add Python versions.Please check whether it meets the requirements.

It does run all sanity tests now for all Python versions, though multiple times. You can remove all entries from the python matrix except one (I suggest 3.8), since you no longer add --python to ansible-test sanity --docker you can limit to one Python version for setting up Ansible.

• all CI tests run against a commit that releases the collection; if they don't pass, the collection won't be released

  • Comment: It is not clear to me whether this is the case.

Yes.if they don't pass, the collection won't be released,That's for sure.

👍

• all CI tests run regularly (nightly, or at least once per week)

  • Comment: This seems not to be the case.

What can I do to make CI run regularly? Do I need to run it manually?

See here: https://github.com/ansible-collections/collection_template/blob/main/.github/workflows/ansible-test.yml#L12-L15

[ISIB-Group]

• has a policy of releasing, versioning and deprecation announced to contributors and users in some way

  • Comment: I can't find any information on that in the collection repository.

We developed the release notes and included the roadmap in README.md.

• follows development conventions

  • While the modules claim they support check mode, I do not see where this is actually implemented. It looks like the modules do the same action both with and without check mode!

We will change the supports_check_mode value to False in all modules, and we will gradually support checking mode later.

• all CI tests run regularly (nightly, or at least once per week)

  • Comment: This seems not to be the case.

We have added a script that is timed to execute in ansible-test.yml

[dmsimard]

What can I do to make CI run regularly? Do I need to run it manually?

I wanted to mention that scheduled runs were added in ISIB-Group/inspur.sm@17ef565 and have been running successfully:

[tadeboro]

Hello.

The community has agreed on a list of criteria that must be met to move forward with the inclusion of a collection in the Ansible package. For this discussion, we will use a shortened checklist to verify and record compliance, but you can find the full details in the Collection Requirements documentation.

This is a new and gradual process, and we will edit this comment as we progress along. 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 at least one comment need your attention. Issues in comments marked as required need to be fixed before collection can be included in Ansible. Comments marked as optional are just friendly advice and will not block the inclusion.

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

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

Standards and documentation:

• adheres to semantic versioning
• follows licensing rules
• follows the Ansible documentation standards and the style guide
• follows development conventions
• has README.md
• documentation, examples, and return use FQCNs for M(..), examples, and seealso subsections
• modules (or plugins) from ansible-base 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 is used for extends_documentation_fragment:, unless you are referring to doc_fragments from ansible-base

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)

[ISIB-Group]

The user module provides the ability to create a user (state: present), edit it (for example changing the shell) or delete it (state: absent). This is abstracted by the module which handles idempotency and takes care of running the right commands with the right arguments.

Thank you, I understand what you said, I will revise it, but it may take two days.

[dmsimard]

@ISIB-Group I see you added ad_group, ldap_group, user and user_group modules in ISIB-Group/inspur.sm@1fdd630 which seem like an improvement at first glance. Do you plan to deprecate the previous modules in favor of these new ones ?

[tadeboro]

I rechecked the collection, and as predicted, the easy fixes were indeed implemented. But the main issue with collection modules being a paper-thin wrapper around underlying SDK is still present. To be sure, will the following task make sure the user is gone from the BMC:

- name: Make sure user account is not present on BMC
  inspur.sm.user:
    uname: some_user
    state: absent

Looking at the code, I am inclined to say no, but I would love to be proven wrong.

[ISIB-Group]

I rechecked the collection, and as predicted, the easy fixes were indeed implemented. But the main issue with collection modules being a paper-thin wrapper around underlying SDK is still present. To be sure, will the following task make sure the user is gone from the BMC:

- name: Make sure user account is not present on BMC
  inspur.sm.user:
    uname: some_user
    state: absent

Looking at the code, I am inclined to say no, but I would love to be proven wrong.

This can work, and here's what I did.But you should also add information about the logged-in user.

[root@localhost ~]# ansible localhost -m inspur.sm.user -a 'host=10.48.3.53 username=admin password=Inspur1! state=present uname=wbs upass=12345678 role_id=Administrator priv=kvm,vmm'
localhost | CHANGED => {
"Message": [
"add user success."
],
"State": "Success",
"changed": true
}
[root@localhost ~]# ansible localhost -m inspur.sm.user -a 'host=10.48.3.53 username=admin password=Inspur1! state=absent uname=wbs'
localhost | CHANGED => {
"Message": [
"del user success."
],
"State": "Success",
"changed": true
}

[ISIB-Group]

@ISIB-Group I see you added ad_group, ldap_group, user and user_group modules in ISIB-Group/inspur.sm@1fdd630 which seem like an improvement at first glance. Do you plan to deprecate the previous modules in favor of these new ones ?

Yes, we plan to deprecate the modules replaced by 'ad_group', 'ldap_group', 'user' and 'user_group', but we haven't done that yet.

[abadger]

Just a note. The deadline for new collections to be accepted was today, 27-01-2021, but we voted at today's community irc meeting to extend that to Friday:

• inspur.sm has until Friday to be ready for another review.
• Reviewers can approve or say not ready by 2-2.
• If it's not approved by 2-2, it has to go to ansible-4 instead.

[felixfontein]

I looked again at it, and I think I found one aspect which we overlooked before: the _info modules should have supports_check_mode=True, since they do not modify anything (see https://docs.ansible.com/ansible/devel/dev_guide/developing_modules_best_practices.html#following-ansible-conventions: "Implement check_mode in all *_info and *_facts modules.").

Since these modules do not modify anything (if they do, it's a bug!), setting supports_check_mode=True for them should be no problem.

[ISIB-Group]

We have changed all modules ending in _info from 'supports_check_mode=False' to 'supports_check_mode=True'.The _info modules never change anything, because they are all implementation fetches.

[felixfontein]

Awesome! Looks good now, thanks a lot! :)

[tadeboro]

Thank you all for either work on the collection itself or for reviewing the submission. I created a PR to have the inspur.sm Ansible Collection included in Ansible 3.0.0 (ansible-community/ansible-build-data#60).

[ISIB-Group]

Thank you all for your help and support for our collection. Thank you very much.

[abadger]

Thanks for all the hard work getting this ready for inclusion! We look forward to working with you into the future :-)