Feature/ssl support dbtspark

[rahulgoyal2987]

resolves #

Description

Checklist

• I have signed the CLA
• I have run this code in development and it appears to resolve the stated issue
• This PR includes tests, or tests are not required/relevant for this PR
• I have updated the CHANGELOG.md and added information about my change to the "dbt next" section.

[jtcohen6]

@rahulgoyal2987 Thanks for the PR!

I'm not opposed to the changes you're proposing here; my main concern is with the new dependency (https://github.com/devinstevenson/pure-transport). I see it has a small number of stars/users/contributors. Is this a package you've relied on for other projects?

FWIW it does seem tailor-made to the our use case, given the reliance on PyHive for thrift + http connections.

[jtcohen6]

Could we put an upper bound on the version here, since it's version-0 software?

You'll need to add this to the extra setup requirements as well:
https://github.com/fishtown-analytics/dbt-spark/blob/dff1b613ddf87e4e72e8a47475bcfd1d55796a5c/setup.py#L41-L44

I'm inclined to bundle it in with the larger PyHive extra, unless there's a good reason to bundle it separately.

[rahulgoyal2987]

@jtcohen6 I have added the logic to build transport object rather than using pure-transport

[rahulgoyal2987]

@jtcohen6 Major use of pure-transport dependency (https://github.com/devinstevenson/pure-transport) It is providing support for TSSLSocket which pyhive is not providing. I have added the logic to build transport object and remove pure-transport dependency

[jtcohen6]

@rahulgoyal2987 This is really neat! Have you managed to test this with your own projects, and confirm that it works as expected?

[rahulgoyal2987]

@jtcohen6 I tested with my own project and setup and it works as expected.

[jtcohen6]

@rahulgoyal2987 Thanks for all the work here! This isn't functionality I'm able to test directly, so I appreciate the unit test, and your confirmation that this works for your use case.

My only comments are around package imports: making sure the dependencies are appropriately installed alongside PyHive extras, or handled if missing.

Could you also:

• Add use_ssl to the README as a supported connection parameter
• Add a Changelog entry (under dbt next), and add yourself to the list of contributors

[jtcohen6]

Let's include these up in the try import (line 11) for requirements that may or may not be installed.

ssl is a net-new dependency, right? We'll need to add it in setup.py, within the Pyhive extra

[rahulgoyal2987]

ssl is part of python library https://docs.python.org/3/library/ssl.html so i think it is not required to add

[jtcohen6]

Sorry, I said ssl here but I was thinking of thrift_sasl. That's a net-new dependency, right? Just kidding, I follow now, those are dependencies of PyHive[hive]. Thanks for the clarification around ssl.

[jtcohen6]

We have a different way of handling these optional imports. See the try logic at the top of the file

[rahulgoyal2987]

Done

[jtcohen6]

Thanks! Could you move these up alongside the other imports (lines 29-32)? I'd prefer not to have imports nested so far down in the fil.

[rahulgoyal2987]

@rahulgoyal2987 Thanks for all the work here! This isn't functionality I'm able to test directly, so I appreciate the unit test, and your confirmation that this works for your use case.

My only comments are around package imports: making sure the dependencies are appropriately installed alongside PyHive extras, or handled if missing.

Could you also:

• Add use_ssl to the README as a supported connection parameter
• Add a Changelog entry (under dbt next), and add yourself to the list of contributors

Done

[jtcohen6]

LGTM! Thanks for the contribution @rahulgoyal2987