Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ci cd take2 #62

Merged
merged 37 commits into from
Dec 17, 2020
Merged

Ci cd take2 #62

merged 37 commits into from
Dec 17, 2020

Conversation

mikaelene
Copy link
Collaborator

Add the adapter tests

@mikaelene
Copy link
Collaborator Author

The new settings for connections breaks old way of setting up profile. Needs to be fixed before merge. Otherwise the tests are actually running quite good!

Thanks @swanderz

@@ -49,7 +49,8 @@ class SQLServerCredentials(Credentials):
# "sql", "ActiveDirectoryPassword" or "ActiveDirectoryInteractive", or
# "ServicePrincipal"
authentication: Optional[str] = "sql"
encrypt: Optional[str] = "yes"
encrypt: Optional[bool] = False
Copy link
Collaborator

@dataders dataders Nov 20, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mikaelene by "breaks old way of setting up profile" do you mean that I've toggled Encrypt to now be False by default instead of True?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something like that. I removed the cert settings stuff from my profile and it stopped working. Just have to make sure the pr doesn’t break the adapter for all users 😀

@mikaelene
Copy link
Collaborator Author

@swanderz I managed to isolate the problem with standard connections to on-prem SQL Server. Looks like when adding Authentcation=SqlPassword to the connections string, some settings around encryption and certificate are changed to a value that doesn't work out of the box. By removing that setting it all works. The question is if it still works with Azure sql with user pass?

@dataders
Copy link
Collaborator

dataders commented Dec 1, 2020

EUREKA! You're incredible @mikaelene for finding this. At first glace, I thought "there's no way that can be it", but you're totally right. Finally my lifelong dream to become an expert in Microsoft ODBC Connection String Keywords has been fulfilled. 😝 This docs page has some juicy snippets that I'll paste here for posterity.

Example Connection Strings

SQL Server Authentication - legacy syntax.
Server certificate is not validated, and encryption is used only if the server enforces it.
The username/password is passed in the connection string.
server=Server;database=Database;UID=UserName;PWD=Password;

SQL Authentication - new syntax.
The client requests encryption (the default value of Encrypt is true) and the server certificate gets validated, regardless of the encryption setting (unless TrustServerCertificate is set to true).
The username/password is passed in the connection string.
server=Server;database=Database;UID=UserName;PWD=Password;Authentication=SqlPassword;

also this farther down the page

Notes
To connect using a SQL Server account username and password, you may now use the new SqlPassword option, which is recommended especially for Azure SQL since this option enables more secure connection defaults.

@dataders
Copy link
Collaborator

dataders commented Dec 1, 2020

@mikaelene everything's good on our end! whenever you're ready

@dataders dataders linked an issue Dec 17, 2020 that may be closed by this pull request
@mikaelene
Copy link
Collaborator Author

This looks great! I imagine that snapshot-check did work before. Can it be that it do work but the test have some issues verifying it? Anyway, lets merge this this to master, do some test runs on actual dbt-projects and then publish to pip :-)

@mikaelene mikaelene merged commit 818409c into master Dec 17, 2020
@mikaelene mikaelene deleted the CI_CD_take2 branch January 20, 2021 08:41
schlich pushed a commit that referenced this pull request Jan 6, 2024
Releasing 1.5 for dbt cloud integration
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

New adapter testing framework
2 participants