-
Notifications
You must be signed in to change notification settings - Fork 227
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
Make the https:// optional #165
Conversation
5c6e1f0
to
3a41bf6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this one @Fokko! I'm all about it
dbt/adapters/spark/connections.py
Outdated
@@ -321,7 +321,7 @@ def open(cls, connection): | |||
'cluster', 'organization']) | |||
|
|||
conn_url = cls.SPARK_CONNECTION_URL.format( | |||
host=creds.host, | |||
host=creds.host if creds.host.startswith('https://') else 'https://' + creds.host, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like flake8 is giving you a hard time for this line being too long:
dbt/adapters/spark/connections.py:324:80: E501 line too long (106 > 79 characters)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dang! Thanks for pointing it out. Just pushed a fix
3a41bf6
to
e88f0f7
Compare
e88f0f7
to
c5cc151
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thanks for this @Fokko!
Just ran into this issue after tracing down why I couldn't set HTTP vs HTTPS on a local testing installation. Is it appropriate to assume the connection is HTTPS? Particularly with no way to configure it otherwise. |
@joebeeson That's a fair thought. I guess I'd hope that folks are connecting to their Spark clusters via HTTPS, but you never know. Perhaps we could switch the logic added by this PR: To instead be:
That way, Is that a switch you'd be interested in contributing? |
resolves #153
Description
This allows you to both do:
and:
Checklist
CHANGELOG.md
and added information about my change to the "dbt next" section.