New collection: lowlydba.sqlserver

[lowlydba]

A cross-platform Ansible collection using PowerShell to configure and maintain SQL Server. This differs from the Microsoft SQL Server collection (https://galaxy.ansible.com/microsoft/sql) in that it is mostly PowerShell Core compatible and works on versions of SQL Server < 2017

Ansible Galaxy: https://galaxy.ansible.com/lowlydba/sqlserver
GitHub: https://github.com/lowlydba/lowlydba.sqlserver
Issues Tracker: https://github.com/lowlydba/lowlydba.sqlserver/issues
GitHub Handles: @lowlydba
Is the collection part of Automation Hub: No
We meet Collection Requirements: Yes

[briantist]

🤚 I will review this

[briantist]

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
  MUST FIX:
  • See the last bullet point in this section.
  • Examples include this issue in community.general, or this README section in vmware.vmware_rest
  • Your README mentions: "We announce releases and important changes through Ansible's The Bullhorn newsletter." but only the first collection release was announced. I don't think it's mandatory to announce in The Bullhorn but if it's mentioned in the README it would be good to make an announcement for new releases :)
• 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
  SHOULD FIX: the module_util _SqlServerUtils.psm1, while private, is still part of the collection and covered under the license. While the stated GPL-3 license for the repository does apply to it, it should still have copyright and license headers. You may also consider the BSD-2-Clause license if you wish.

• follows the Ansible documentation standards and the style guide
  SHOULD FIX:

  • Most modules: examples are very lean, typically only showing one use, with minimal options. Consider more complete examples that show real-world uses, and cover a variety of combinations of options.
  • Many modules (nit): description lines are misaligned (too many spaces, odd number of spaces)
  • Several modules (nit/suggestion): consider using C(value) formatting in place of plain quoting 'value', especially in option descriptions, also consider for formats, like C(yyyyMMdd), SP names like C(sp_WhoisActive). logins like C(sa), SQL values like C(NULL).
  • install_script (nit): description for deployment_method could use C(...) for values, and use a separate list item for each line of the description

  MUST FIX:

  • Most modules: short_description must not end in a period .
  • traceflag: short_description has SQL Server with two spaces
  • sa: description for password_must_change must refer to other options with I(...) formatting, not C(...)
  • restore:
    • many options refer to what appear to be other option names, but with PascalCase instead of snake_case. These should use the correct case, and use I(...) formatting if they are other module options, or otherwise be clarified. Some are referred to as "parameters" but should use the term "options"
    • description for restore_time and some other options refers to a "DateTime object" but takes a string without describing the format, not shown in examples
    • destination_file_suffix description uses "ALL" (caps for emphasis). Should be changed to B(all) for bold emphasis or I(all) for italics (but since I(...) is often used for options, that could be confusing).
    • azure_credential descriptions references an AzureBaseUrl but it isn't clear what that's referring to as there's no option with that name
  • maintenance_solution: description for solution should not include the values that are already specified in choices:
  • login.py: description for password_must_change must use I(..) for referring to other options
  • backup.py: description for azure_credential references azure_base_url option with PascalCase instead of snake_case and without I(...)
  • agent_job_step:
    • description for database option should not state the default value (already specified in default:)
    • description for on_success_step_id refers to option on_success_action in PascalCase instead of snake_case and without I(...)
    • ditto for on_fail_step_id's description and on_fail_action
  • agent_job_schedule:
    • line in force option's description: If I(force=true) the default will be 'Once'. should be under the frequency_type option instead
    • frequency_interval description's references to the frequency_type option should use I(...), and values should use C(...)
    • start_date and end_date should specify the date formats accepted (though there are some examples, so this is maybe optional)
  • agent_job_category: the category_type option should not repeat its choices in the description

• 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
  MUST FIX: (modules whose examples don't use FQCNs)

  • ag_listener
  • install_script (also example is for the wrong module)
  • spn

• 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.

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
  MUST FIX: meta/runtime.yml says ansible-core >=2.10 is supported but tests only run against 2.11+. Many collections have dropped 2.10 so it's ok to update the supported version to match the tests rather than the other way around.
• 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

[briantist]

All outstanding issues I raised have been addressed in lowlydba/lowlydba.sqlserver#119 (merged as lowlydba/lowlydba.sqlserver@f2d6730)

This is ready for second review!

[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.

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 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
  MUST FIX: please re-check -name: in examples contains a sentence describing tasks. It's missed in instance_info.py, for example
  MUST FIX: A description of agent_job_category.py would imo look a bit strange on the docsite. I would start the description with the sentence from short_description. Please re-check other modules.
• 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.
  MUST FIX: remove the .vscode directory and add it to .gitignore
  COMMENT: I would suggest moving some files from the .github directory at the repo's root, like CoC, CONTRIBUTING.md, etc. I think it's a bit tricky to find them where they are at the moment

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
  COMMENT: stable-2.10 can be removed from the matrix (the integration block)
• 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

[Andersson007]

@lowlydba i finished my review, could you please take a look and let me know when you fix the issues?
If there are any question, feel free to ask.
Thanks!

[lowlydba]

follows the Ansible documentation standards and the style guide

✅ - Fixed in v1.0.3 / lowlydba/lowlydba.sqlserver#122

collection repository should not contain any unnecessary files like, for example, temporary files created by development tools. Temporary files MUST be added to .gitignore.

✅ - Assuming the discussion on this allows the .vscode/ files as a take-away, this can be considered resolved in v1.0.3 / PR

[Andersson007]

@lowlydba thanks for fixing all the things we mentioned!
+1 from me for inclusion, we can proceed i guess

Could you please:

• Subscribe to the news-for-maintainers repo and track announcements there. The ansible-core folks publish changes related to CI (and not only). So you'll be informed in advance about the changes. Otherwise, your CI will get red one day in PRs for unclear reasons.
• Optionally but highly recommended, subscribe to Bullhorn.
• We have the Maintainer guidelines. Might be useful to take a look.
• Share the above with future maintainers.

[Andersson007]

ansible-community/community-topics#149

[Andersson007]

included ansible-community/ansible-build-data#172, will appear in the next minor release of 6 (if there's one) or max in 7.
@lowlydba thanks for the new collection! @briantist thanks for reviewing!