mssql_db: new module #189

Closed
wants to merge 5 commits into
from

Projects

None yet

6 participants

@vedit
Contributor
vedit commented Jan 8, 2015

Can create, delete databases and execute sql scripts. Currently neglected dump, but can add it.
This is basically the mssql equivalent of mysql_db module. I tried to keep all the usage of the module the same.

Waiting for questions and feedbacks

@bcoca
Member
bcoca commented Jan 9, 2015

One thing would be avoiding SQL injection

"SELECT name FROM master.sys.databases WHERE name = N'%s'" % db

is not safe, you want to pass the parameters in the cursor.execute method as it does sanitize input.

@bcoca bcoca added the needs_revision label Jan 9, 2015
@vedit
Contributor
vedit commented Jan 12, 2015

Thanks for the feedback @bcoca . I updated the pull request, but can't really seem to think of a safer way to execute scripts, else then executing every other line in cursor object, if I don't sanitize the inputs myself.

@bcoca
Member
bcoca commented Jan 12, 2015

cursor.execute santizes the input when it does the interpolation.

@vedit
Contributor
vedit commented Jan 12, 2015

True, but I was talking about the following code block. I fixed the other parts properly

def db_import(conn, cursor, module, db, target):
+    if os.path.isfile(target):
+        with open(target, 'r') as backup:
+            sqlQuery = "USE %s\n" % db
+            for line in backup:
+                if line.startswith('GO'):
+                    cursor.execute(sqlQuery)
+                    sqlQuery = "USE %s\n" % db
+                else:
+                    sqlQuery = sqlQuery + line
+            cursor.execute(sqlQuery)
+            conn.commit()
+        return 0, "Import Successful", ""
+    else:
+        module.fail_json(msg="cannot find target file")
@bcoca
Member
bcoca commented Jan 12, 2015

i really don't see the issue:

def db_import(conn, cursor, module, db, target):
+    if os.path.isfile(target):
+        with open(target, 'r') as backup:
+            sqlQuery = "USE %s\n"
+            for line in backup:
+                if line.startswith('GO'):
+                    cursor.execute(sqlQuery, db)
+                    sqlQuery = "USE %s\n"
+                else:
+                    sqlQuery = sqlQuery + line
+            cursor.execute(sqlQuery, db)
+            conn.commit()
+        return 0, "Import Successful", ""
+    else:
+        module.fail_json(msg="cannot find target file")
@vedit
Contributor
vedit commented Jan 12, 2015

Yeah, it is ok I guess. Just thought

sqlQuery = "USE %s\n" % db

part might have been problematic

@vedit
Contributor
vedit commented Jan 13, 2015

Pardon my reading skills @bcoca I didn't notice the difference. Here is your proposed fix

@gregdek
Contributor
gregdek commented Jun 17, 2015

@bcoca @vedit does the proposed fix make this module appropriate for inclusion?

@gregdek gregdek removed the P3 label Sep 25, 2015
@gregdek
Contributor
gregdek commented Sep 25, 2015

Adding new process. We will be evaluating all new module PRs according to this process, effective immediately.

Thanks for submitting this new module to Ansible Extras! This module is now in community review, a process that is open to all Ansible users. In order for this module to be approved, it must gain the following votes:

“works_for_me”: If you have tested the module thoroughly, including testing of all of the module’s options, and if the module works for you, please add “works_for_me” in the comments.

“passes_guidelines”: If you have gone through the module guidelines and the module meets all of the requirements, please add “passes_guidelines” in the comments. Guidelines are available here: http://docs.ansible.com/developing_modules.html#module-checklist

“needs_revision”: If the module fails to work for you, or if it doesn’t meet guidelines, please add “needs_revision” in the comments with details about what needs to be fixed.

When a module has both “works_for_me” and “passes_guidelines” tags, we will promote the module for inclusion in Ansible Extras. At this point, you will be expected to maintain the module by fixing bugs and evaluating pull requests in a timely manner.

Thanks again for submitting your Ansible module!

@dario-hd

@gregdek @bcoca any chance to include this new module? I was going to develop a similar one, but I found this. I'd like to see it included so that I can contribute to add more features. Or, should I develop it from scratch and open a new PR?

@bcoca
Member
bcoca commented Jan 30, 2016

@dario-hd reviewing and testing this module and providing feedback is fastest way to get it merged, right now extras modules depend more on the community than on the core team for acceptance.

@vedit
Contributor
vedit commented Jan 30, 2016

@bcoca @dario-hd I had some fixes for this module, apparently this PR was left behind. let me get it up to speed.

@h0nIg
Contributor
h0nIg commented Feb 19, 2016

pulled your pull request and exteded it / fixed some stuff: #1690

  • added kerberos support (username is empty)
  • fix syntax problems related to sql injection change
  • added autocommit setting to import stuff not allowed within transaction
  • fix automated tests
@gregdek
Contributor
gregdek commented Mar 29, 2016

Thanks @vedit for this PR. This PR requires revisions, either because it fails to build or by reviewer request. Please make the suggested revisions. When you are done, please comment with text 'ready_for_review' and we will put this PR back into review.

[This message brought to you by your friendly Ansibull-bot.]

@vedit
Contributor
vedit commented Mar 31, 2016

Closing in favor of #1690

@vedit vedit closed this Mar 31, 2016
@jimi-c jimi-c removed the needs_revision label Mar 31, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment