community.general
community.general copied to clipboard
ansible-collections
Add Script File Path Support with Automatic BOM Removal to mssql_script Module
SUMMARY
Fixes #10201
This feature branch updates the mssql_script module with a new parameter allowing users to supply a file path containing their SQL script. To avoid potential execution issues, the file is checked for the presence of any common BOM patterns - if found, they are removed.
Notes:
- When a BOM is detected and removed, details are added to the module results.
- This module will never modify the provided script, we are only modifying the file contents as they are loaded into memory.
- All existing functionality remains unchanged, BOM detection only applies to files using the
script_pathparam.
ISSUE TYPE
- Feature Pull Request
COMPONENT NAME
mssql_script
ADDITIONAL INFORMATION
See changelog fragment for more information.
This code contribution is made on behalf of the Omnissa UEM Cloud Services Team (formerly VMware EUC), internal reference Jira AWSA-47744.
The test ansible-test sanity --test line-endings [explain] failed with 1 error:
changelogs/fragments/10201-mssql_script-alternate-credentials-and-script-path.yml:0:0: use "\n" for line endings instead of "\r\n"
The test ansible-test sanity --test pep8 [explain] failed with 2 errors:
plugins/modules/mssql_script.py:352:1: W293: blank line contains whitespace
plugins/modules/mssql_script.py:371:161: E501: line too long (169 > 160 characters)
The test ansible-test sanity --test pylint [explain] failed with 1 error:
plugins/modules/mssql_script.py:352:0: trailing-whitespace: Trailing whitespace
The test ansible-test sanity --test line-endings [explain] failed with 1 error:
changelogs/fragments/10201-mssql_script-alternate-credentials-and-script-path.yml:0:0: use "\n" for line endings instead of "\r\n"
The test ansible-test sanity --test pep8 [explain] failed with 2 errors:
plugins/modules/mssql_script.py:352:1: W293: blank line contains whitespace
plugins/modules/mssql_script.py:371:161: E501: line too long (169 > 160 characters)
The test ansible-test sanity --test line-endings [explain] failed with 1 error:
changelogs/fragments/10201-mssql_script-alternate-credentials-and-script-path.yml:0:0: use "\n" for line endings instead of "\r\n"
The test ansible-test sanity --test pep8 [explain] failed with 2 errors:
plugins/modules/mssql_script.py:352:1: W293: blank line contains whitespace
plugins/modules/mssql_script.py:371:161: E501: line too long (169 > 160 characters)
The test ansible-test sanity --test pylint [explain] failed with 1 error:
plugins/modules/mssql_script.py:352:0: trailing-whitespace: Trailing whitespace
The test ansible-test sanity --test pylint [explain] failed with 1 error:
plugins/modules/mssql_script.py:352:0: trailing-whitespace: Trailing whitespace
Hi @zollo thanks for your contribution.
I must say, I am a bit uneasy about this PR - I mean, the "alternate credentials" part of it. That does not seem to be anything particular to MSSQL, in fact the code literally tries with one set of credentials and if that does not work, it tries to login with the other one.
IMHO, this feature:
- is adding unnecessary complexity to the module, just to save few lines of YAML for the Ansible user. Do you have any particular use case in mind for this feature? I am finding it hard to visualize a playbook in which we need to access a database and we use credentials that do not work (hence the need for an alternate one), but I might be missing something.
- opens a dangerous precedent - why not add that feature to mysql modules? to postgresql modules? To any other module that required credentials (not just database ones)?
- raises another question: why only one set of alternate credentials? Why not two, or three? How many is too many in that case? You may think that one set of alternates is enough, I think 1 is already too much, but we might bump into someone else in the community who thinks they must have two.
I think those questions are very subjective, and as such, we should try and avoid bringing them up in the context of the module. The maintenance of common/shared code based on individual's subjective views is probably going to much more painful than it needs to.
Sorry, I don't think we should add that feature to the module, but I might be wrong in that. It would be nice if more folks could chip in this one. @Andersson007 you are maintaining the PGSQL modules (right? :grin: ), what do you think of this?
That being said, the handling of BOM markers is definitely something we could benefit from.
@russoz thanks for pinging! I'm also not sure if we should add these "in-case-of-failure" features (cause a lot of things can fail). It's solvable on Ansible level by using:
ignore_errors: true+register: result- another task containing the other creds with the
when: result is faileddirective
my opinion
The test ansible-test sanity --test pep8 [explain] failed with 1 error:
plugins/modules/mssql_script.py:381:1: E302: expected 2 blank lines, found 1
The test ansible-test sanity --test pep8 [explain] failed with 1 error:
plugins/modules/mssql_script.py:381:1: E302: expected 2 blank lines, found 1
The test ansible-test sanity --test pep8 [explain] failed with 1 error:
plugins/modules/mssql_script.py:381:1: E302: expected 2 blank lines, found 1
@zollo This pullrequest is waiting for your response. Please respond or the pullrequest will be closed.
@zollo You have not responded to information requests in this pullrequest so we will assume it no longer affects you. If you are still interested in this, please create a new pullrequest with the requested information.