search

dbt-labs / dbt-spark

dbt-spark contains all of the code enabling dbt to work with Apache Spark and Databricks
https://getdbt.com
Apache License 2.0
400 stars 227 forks source link

Allow hostname to be provided with or without a trailing slash #784

Closed tim-steinkuhler closed 1 year ago

tim-steinkuhler commented 1 year ago

resolves #302

Description

Allow hostname to be provided with or without trailing slash

When you copy in the hostname, it may or may not include trailing / We could avoid confusion by allowing users to include or exclude these elements.

Before this change, dbt debug would get you something like this:

dbt was unable to connect to the specified database.
The database returned the following error:

  >Runtime Error
  Database Error
    failed to connect

Output while using a trailing slash after change (given credentials are correct):

Connection test: [OK connection ok]

Checklist

cla-bot[bot] commented 1 year ago

Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Tim Steinkühler. This is most likely caused by a git client misconfiguration; please make sure to:

  1. check if your git client is configured with an email to sign commits git config --list | grep email
  2. If not, set it up using git config --global user.email email@example.com
  3. Make sure that the git commit email is configured in your GitHub account settings, see https://github.com/settings/emails
tim-steinkuhler commented 1 year ago

Hi @colin-rogers-dbt (I see you are assigned as reviewer), I'm finding it a bit difficult to relate the CI error message "at least one column must be specified for the table" to the changes I made. If I did mess this up, I would expect a lot more tests to fai, not just that one, and I would expect a different error message. Could you help me get a better understanding?

tim-steinkuhler commented 1 year ago

@Fokko, are you able to have a look at this? After a rebase on the latest version of main, the tests have been passed, although there are still 3 "expected" checks, how does this work? :-D

tim-steinkuhler commented 1 year ago

By the way, thank you @Fokko! Your feedback is much appreciated!

tim-steinkuhler commented 1 year ago

I don't think my change could lead to this error message in the integration-spark-session pipeline. @colin-rogers-dbt could you have a look?

JCZuurmond commented 1 year ago

@tim-steinkuhler : Could you force a new commit to restart the CI? I do not understand why the integration-spark-session CI fails; I would like to rerun the CI to be sure

JCZuurmond commented 1 year ago

@tim-steinkuhler : Ok, that did not do the trick unfortunately. Let's discuss an approach for this in person!

tim-steinkuhler commented 1 year ago

After a short discussion with @JCZuurmond , we decided to not remove the https:// prefix

JCZuurmond commented 1 year ago

@colin-rogers-dbt : Could you help with the CI again? Given the code changes and that the CI passed before, I expect that the CI fails because it is a bit flacky.

@fleid: This PR is ready to be merged. Could you merge it once the CI is fixed?

colin-rogers-dbt commented 1 year ago

Thanks for the contribution!