New collection: kaytus.ksmanage

[ieisystem]

This repo contains the kaytus. ksmanage Ansible Collection. KAYTUS Server supports ansible management device,Basic management of the server based on restful interface.

Ansible Galaxy: https://galaxy.ansible.com/ui/repo/published/kaytus/ksmanage/
GitHub: https://github.com/ieisystem/kaytus.ksmanage
Issues Tracker: https://github.com/ieisystem/kaytus.ksmanage/issues
GitHub Handles:
Is the collection part of Automation Hub: No
We meet Collection Requirements: Yes

[ieisystem]

@Andersson007 Could you please check whether this package meets the requirements? Thank you.

[Andersson007]

@ieisystem hello, i need to consult with the Steering Committee first

[ieisystem]

thanks

[Andersson007]

@ieisystem here's the first review, please take a look (please re-check your second collection you submitted).

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
• 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 git 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
  MUST FIX: doesn't follow, please the guides ^ carefully. Scrutinize the guides, create a list of things to fix (like all sentences must start with a capital later and end with a period, etc.) Also the language used should adhere English grammar and structure: the latter can be hard but try - the docs must look good if it's on docs.ansible.com.

For example i found the following with my script:

========== ~/ansible_collections/kaytus/ksmanage/plugins/modules/edit_alert_policy.py ============
opt destination: line 1 does not end with a dot
opt destination: line 2 does not start with a capital letter
opt destination: line 2 does not end with a dot
opt destination: line 3 does not start with a capital letter
return message: no sample
return state: no sample
return changed: no sample

You can check all things manually or it my script. There can be false positives because the script is not ideal: if you're sure things it reports are correct, ignore such warnings.

• follows development conventions; as well as these other requirements:
  • modules satisfy the concept of idempotency
    COMMENT: does the collection satisfy this rule? For example, does the user.py module check if the user is already present and its attributes and the module doesn't change anything if the state already satisfies the desired? Same questions for all the other modules.
  • 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 all Python versions supported by all ansible-core versions its supports. 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)
  SHOULD FIX: In the README, Ansible is used but it's actually ansible-core meant. Please fix it to avoid confusion.
• 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 (except cases when they were added in the very first release of the collection)
• 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. Temporary files should be added to .gitignore.

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 in the list of errors that must not be ignored
• 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

[ieisystem]

follows the Ansible documentation standards and the style guide

On the issue of document formatting, You can refer to this, which is generated based on the content in the script, and I think it meets the requirements.

modules satisfy the concept of idempotency

Similar modules to user.py are ad_group.py,ldap_group.py, and user_group.py, which all check to see if information already exists when they are added; they are all idempotent. thanks!

[Andersson007]

follows the Ansible documentation standards and the style guide

On the issue of document formatting, You can refer to this, which is generated based on the content in the script, and I think it meets the requirements.

modules satisfy the concept of idempotency

Similar modules to user.py are ad_group.py,ldap_group.py, and user_group.py, which all check to see if information already exists when they are added; they are all idempotent. thanks!

There are at least necessary white spaces missed in many places in this module you point to. Anyway, it was only one example. I spotted doc issues in other modules too

[ieisystem]

Thank you very much for your script. Through the script, I found many problems that I did not find before, and I have modified the discovered problems. Thank you!

[Andersson007]

@ieisystem cool, thanks, in both the collections?
I'll try to find time this Thursday to take another look

[Andersson007]

I've jut taken another look

@oraNode @samccann doc question: do you think if it's OK to have sentences in docs like:

 44             - Alert destination,when type is snmp,fill in IP.
 45             - When type is email,fill in user name.
 46             - When type is snmpdomain,fill in domain.

I'm talking about punctuation: no whitespaces after commas

- SNMP trap port(1-65535).

Also no whitespace

Things like the following are clearly incorrect from the language perspective and must be fixed:

29     uid:
 30         description:
 31             - User id,The range is 1 to 16.
 32         type: int
 33     uname:
 34         description:
 35             - User name,Required when uid is None.
 36         type: str

I also don't understand what's meant here:

 42         description:
 43             - User group.
 44             - Default user group 'Administrator', 'Operator', 'User'.
 45             - Use command C(user_group_info) can get all group information.
 46         type: str

@ansible-collections/steering-committee i understand that English can be hard as foreign language for some of our contributors. Do you think it's OK to have the doc like ^ ? I personally think it doesn't satisfy the clarity requirement but I'm not sure, it's subjective..

[oraNod]

@Andersson007 Maybe it's a bit hard to grok with the missing spaces and grammar issues but it seems to make sense to me overall. I don't know that it breaks anything in the clarity requirement. There's certainly room for improvement and I feel like it's better to make those changes prior to inclusion though.

How can I help with improving the docs? Should we send PRs directly to https://github.com/ieisystem/kaytus.ksmanage ?

On a side note it might be a good idea to have a linter to catch some of these things and suggest improvements. We could use vale. WDYT?

[ieisystem]

I agree with you that you can open a PR.

[oraNod]

@ieisystem OK, I can see there is a large number of modules available. I'll start by opening a PR with some changes that demonstrate the type of documentation fixes that need to be applied. However I do not intend to make the changes throughout every module in the repo.

[oraNod]

ieisystem/kaytus.ksmanage#10

[ieisystem]

@oraNod Thank you very much for your modification. Your modification is very helpful to me. I have not noticed these problems before, which let me know how to modify such problems. Thanks!

[ieisystem]

@Andersson007 We released the new version 1.1.2, could you please check if it meets the requirements, thanks!

[ieisystem]

follows the Ansible documentation standards and the style guide
Regarding this issue, we have finished the modification. Please check whether our modification is correct. Thanks!

[Andersson007]

@ieisystem i was on PTO, i'll check it this week, thanks

[Andersson007]

+1 from me for inclusion. We still need the second review and approval. I'll change the category that the second review is wanted and announce it. Anybody from @ansible-collections/steering-committee wants to make the second review?

[ieisystem]

Thank you very much.

[markuman]

Anybody from @ansible-collections/steering-committee wants to make the second review?

Should I get an email when using this syntax?
I will review the collection today and tomorror.

[markuman]

@ieisystem in the README.md you're refering to

[ieisystem support] inManage 1.0.0 or later

https://github.com/ieisystem/kaytus.ksmanage/blob/main/README.md?plain=1#L25

but in module_utils error manage, you name it

ksmanage_sdk must be installed to use this module

https://github.com/ieisystem/kaytus.ksmanage/blob/main/plugins/module_utils/ksmanage.py#L52

I recommend to use just one name. Preferably the name of the python package that needs to be installed as a dependency.

[markuman]

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
• 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 git 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 satisfy the concept of idempotency
  • 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 all Python versions supported by all ansible-core versions its supports. 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 (except cases when they were added in the very first release of the collection)
• 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. Temporary files should be added to .gitignore.

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 in the list of errors that must not be ignored
• 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

[markuman]

Technical everything is fine from my point of view.
Because the collection is very new (the time gap between creation of the collection and submit of inclusion) I personally would wait some time how well the maintenance of the collections works in the near future. But because this is my own opinion and not a inclusion criterium:
+1 from me for inclusion

[Andersson007]

@markuman thanks a lot!

[Andersson007]

https://forum.ansible.com/t/default-date-2024-04-25-collection-inclusion-include-kaytus-ksmanage-in-the-ansible-community-package/5016

[felixfontein]

https://github.com/ieisystem/kaytus.ksmanage/blob/main/docs/docsite/links.yml is the unmodified file from the template. It must be filled out with correct values, or deleted!

A smaller issue: I see a lot gather_facts: no in the examples. Please use gather_facts: false instead.

[Andersson007]

@ieisystem please let us know when what Felix mentioned is fixed, otherwise the inclusion will be postponed, thanks

[ieisystem]

I will finish it before tomorrow, thank you!

[ieisystem]

@Andersson007 We have finished fixing the problem and released version 1.2.1, please check if it meets the requirements. Thanks!

[Andersson007]

PR ansible-community/ansible-build-data#394 , once merge, the collection will appear in Ansible 9.Y next minor release, fyi

[felixfontein]

@ieisystem thanks for fixing this!

[Andersson007]

@ieisystem congrats https://forum.ansible.com/t/default-date-2024-04-25-collection-inclusion-include-kaytus-ksmanage-in-the-ansible-community-package/5016/5?u=andersson007 !

Thanks everyone!