Add Script File Path Support with Automatic BOM Removal to mssql_script Module #10203

zollo · 2025-06-05T16:15:44Z

SUMMARY

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_path param.

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.

ansibullbot · 2025-06-05T16:16:33Z

cc @kbudde
click here for bot help

russoz · 2025-06-07T03:04:23Z

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? 😁 ), what do you think of this?

That being said, the handling of BOM markers is definitely something we could benefit from.

Andersson007 · 2025-06-10T09:02:46Z

@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 failed directive

my opinion

felixfontein · 2025-06-14T12:50:42Z

plugins/modules/mssql_script.py

+script_source:
+  description: Source of the executed script.
+  type: str
+  returned: always


This isn't returned if the module fails. Also all new return values need a version_added entry.

Suggested change

returned: always

returned: success

version_added: 11.0.0

felixfontein · 2025-06-14T12:50:51Z

plugins/modules/mssql_script.py

+  choices: ["file", "parameter"]
+script_path:
+  description: Path to the script file that was executed.
+  type: str


Suggested change

type: str

type: path

felixfontein · 2025-06-14T12:51:12Z

plugins/modules/mssql_script.py

+script_path:
+  description: Path to the script file that was executed.
+  type: str
+  returned: when script_path parameter is used


Same below:

Suggested change

returned: when script_path parameter is used

returned: success and O(script_path) is provided

felixfontein · 2025-06-14T12:51:52Z

plugins/modules/mssql_script.py

+bom_removed:
+  description: Whether a Byte Order Mark was detected and removed from the script file.
+  type: bool
+  returned: when script_path parameter is used and BOM was found


Why not always provide this if script_path is used? It's not very helpful if this is sometimes true and sometimes not there.

felixfontein · 2025-06-14T12:52:25Z

plugins/modules/mssql_script.py

+  description: Type of Byte Order Mark that was detected and removed.
+  type: str
+  returned: when script_path parameter is used and BOM was found
+  sample: "UTF-8"


How about returning none/null (the Jinja/YAML equivalent for Python's None) here if script_path is used, but no BOM was found?

felixfontein · 2025-06-14T12:53:34Z

plugins/modules/mssql_script.py

+      - If the file contains a Byte Order Mark (BOM), it will be automatically detected and removed.
+      - Mutually exclusive with O(script).
+    type: path
+    version_added: 10.8.0


Suggested change

version_added: 10.8.0

version_added: 11.0.0

felixfontein · 2025-06-14T12:54:35Z

changelogs/fragments/10201-mssql_script-script-path-and-bom-removal.yml

+  - mssql_script - add ``script_path`` parameter to allow executing SQL scripts from files as an alternative to inline scripts.
+  - mssql_script - add automatic Byte Order Mark (BOM) detection and removal for script files to prevent SQL parsing errors.


The second fragment isn't needed, since this is part of the feature mentioned in the first fragment. Also the URLs are missing.

Suggested change

- mssql_script - add ``script_path`` parameter to allow executing SQL scripts from files as an alternative to inline scripts.

- mssql_script - add automatic Byte Order Mark (BOM) detection and removal for script files to prevent SQL parsing errors.

- mssql_script - add ``script_path`` parameter to allow executing SQL scripts from files as an alternative to inline scripts (https://github.com/ansible-collections/community.general/issues/10201, https://github.com/ansible-collections/community.general/pull/10203).

felixfontein · 2025-06-14T12:54:57Z

plugins/modules/mssql_script.py

+  script_path:
+    description:
+      - Path to a file containing the SQL script to be executed.
+      - Script can contain multiple SQL statements. Multiple Batches can be separated by V(GO) command.


Suggested change

- Script can contain multiple SQL statements. Multiple Batches can be separated by V(GO) command.

- Script can contain multiple SQL statements. Multiple batches can be separated by V(GO) command.

felixfontein · 2025-06-14T12:55:38Z

plugins/modules/mssql_script.py

+      - Path to a file containing the SQL script to be executed.
+      - Script can contain multiple SQL statements. Multiple Batches can be separated by V(GO) command.
+      - Each batch must return at least one result set.
+      - If the file contains a Byte Order Mark (BOM), it will be automatically detected and removed.


Suggested change

- If the file contains a Byte Order Mark (BOM), it will be automatically detected and removed.

- If the file contains a Byte Order Mark (BOM), it will be automatically detected and removed.

The return values RV(bom_removed) and RV(bom_type) will be set accordingly.

felixfontein · 2025-06-14T12:57:10Z

plugins/modules/mssql_script.py

+                script_content = raw_content.decode('utf-8', errors='replace')
+
+        # Detect and remove BOM
+        cleaned_content, bom_removed, bom_type = detect_and_remove_bom(


Shouldn't the BOM detection/removal happen before any decoding is done? The BOM basically tells you the encoding you have to use to decode the script.

felixfontein · 2025-06-14T12:59:43Z

plugins/modules/mssql_script.py

+    Returns tuple (script_content, bom_info)
+    """
+    if not os.path.exists(script_path):
+        raise IOError(f"Script file not found: {script_path}")


f-strings are not compatible with Python 2.7, you cannot use them currently in modules.

zollo · 2025-06-14T14:29:11Z

Thank you for the great feedback @felixfontein - I will address these ASAP.

felixfontein · 2025-07-13T19:41:46Z

Ping @zollo

needs_info

ansibullbot · 2025-08-14T20:10:36Z

@zollo This pullrequest is waiting for your response. Please respond or the pullrequest will be closed.

click here for bot help

add new functionality to module, update changelog fragment

0d55883

ansibullbot added feature This issue/PR relates to a feature request module module new_contributor Help guide this first time contributor plugins plugin (any type) labels Jun 5, 2025

This comment was marked as outdated.

Sign in to view

ansibullbot added ci_verified Push fixes to PR branch to re-run CI needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR labels Jun 5, 2025

linting errors

199d7cd

ansibullbot removed ci_verified Push fixes to PR branch to re-run CI needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR labels Jun 5, 2025

felixfontein added the check-before-release PR will be looked at again shortly before release and merged if possible. label Jun 5, 2025

refactored BOM detection and removal code to be more robust

8e95aee

zollo changed the title ~~Add Alternate Login Credential & Script File Path Support to mssql_script Module~~ Add Script File Path Support with Automatic BOM Removal to mssql_script Module Jun 13, 2025

This comment was marked as outdated.

Sign in to view

ansibullbot added ci_verified Push fixes to PR branch to re-run CI needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR labels Jun 13, 2025

linting

ad0444f

ansibullbot removed ci_verified Push fixes to PR branch to re-run CI needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR labels Jun 13, 2025

felixfontein reviewed Jun 14, 2025

View reviewed changes

felixfontein added the backport-11 Automatically create a backport for the stable-10 branch label Jun 16, 2025

ansibullbot added the stale_ci CI is older than 7 days, rerun before merging label Jun 24, 2025

ansibullbot added the needs_info This issue requires further information. Please answer any outstanding questions label Jul 13, 2025

	returned: when script_path parameter is used
	returned: success and O(script_path) is provided

		- mssql_script - add ``script_path`` parameter to allow executing SQL scripts from files as an alternative to inline scripts.
		- mssql_script - add automatic Byte Order Mark (BOM) detection and removal for script files to prevent SQL parsing errors.

	- Script can contain multiple SQL statements. Multiple Batches can be separated by V(GO) command.
	- Script can contain multiple SQL statements. Multiple batches can be separated by V(GO) command.

	- If the file contains a Byte Order Mark (BOM), it will be automatically detected and removed.
	- If the file contains a Byte Order Mark (BOM), it will be automatically detected and removed.
	The return values RV(bom_removed) and RV(bom_type) will be set accordingly.

Add Script File Path Support with Automatic BOM Removal to mssql_script Module #10203

Are you sure you want to change the base?

Add Script File Path Support with Automatic BOM Removal to mssql_script Module #10203

Uh oh!

Conversation

zollo commented Jun 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

SUMMARY

ISSUE TYPE

COMPONENT NAME

ADDITIONAL INFORMATION

Uh oh!

ansibullbot commented Jun 5, 2025

Uh oh!

This comment was marked as outdated.

russoz commented Jun 7, 2025

Uh oh!

Andersson007 commented Jun 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment was marked as outdated.

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zollo commented Jun 14, 2025

Uh oh!

felixfontein commented Jul 13, 2025

Uh oh!

ansibullbot commented Aug 14, 2025

Uh oh!

Uh oh!

zollo commented Jun 5, 2025 •

edited

Loading

Andersson007 commented Jun 10, 2025 •

edited

Loading