Fix make_connection_configuration for MSSQL (adding a driver) #113
Conversation
| elif conn_type == "mysql": | ||
| odbc_connector = "mysql" | ||
| database_name = self.schema | ||
| else: |
There was a problem hiding this comment.
will conn_type = "mssql" or something similar be passed to this method? If so could we please try something like
elfi conn_type == "mssql":
...
else:
raise ValueError(f"conn_type {conn_type} is not supported")There was a problem hiding this comment.
Hi @Lee-W, thank you for your review!
Yes, conn_type == "mssql" will be passed in. I adjusted the code with your suggestion. I might actually add something about odbc connection types in a second PR, but I will have to test that first :)
…he driver w/o a db name specified
|
@Lee-W apologies, I tested the operator again and found out that the connection only accepts the driver when a database is specified in the URI. Added the MSSQL default database |
| odbc_connector = "mssql+pyodbc" | ||
| uri_string = f"{odbc_connector}://{self.conn.login}:{self.conn.password}@{self.conn.host}:{self.conn.port}/{self.schema}" # noqa | ||
| ms_driver = self.conn.extra_dejson.get("driver") or "ODBC Driver 17 for SQL Server" |
There was a problem hiding this comment.
nitpick: not yet tested myself, but I think the following should work 🤔
| ms_driver = self.conn.extra_dejson.get("driver") or "ODBC Driver 17 for SQL Server" | |
| ms_driver = self.conn.extra_dejson.get("driver", "ODBC Driver 17 for SQL Server") |
|
@phanikumv thank you! 😊 |
Hi :)
This came up in the GX Slack twice. Using the GXO with a MSSQL conn_id currently does not work because there is no driver being passed via the URI. If fails with:
There was also an issue with the connection needing a database argument separate from the schema passed through the operator.
This PR should fix that. I added the possibility to pass a driver via the connection extras, if none is passed
ODBC Driver 17 for SQL Serveris passed, which is what worked for me when testing with themcr.microsoft.com/mssql/server:2022-latestimage.I tested this change with Postgres and MSSQL. :)
@phanikumv I hope it is ok to tag you for review on this, Viraj told me you might be able to help :)