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

Added sqlserver compatibility including CI #248

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

jgilfillan
Copy link

this PR relatest to #194

Backlog:
Making sure the adapter you're supporting implements the type helpers used in the source models.
Adding overwrites for the custom type helpers if required
If required, creating adapter-specific version of the upload_* macros.
Adding SQLServer CI support

I do not have access to a snowflake, databricks or bigquery server so relying on the CI tests to confirm all is working.

@jgilfillan jgilfillan temporarily deployed to Approve Integration Tests November 20, 2022 23:49 Inactive
@jgilfillan jgilfillan temporarily deployed to Approve Integration Tests November 20, 2022 23:49 Inactive
@jgilfillan jgilfillan temporarily deployed to Approve Integration Tests November 20, 2022 23:49 Inactive
@jgilfillan jgilfillan temporarily deployed to Approve Integration Tests November 20, 2022 23:49 Inactive
@jgilfillan
Copy link
Author

I've reviewed the linting errors and I can't figure out why the code is failing for most of the exceptions. For example, there's nothing obviously wrong with the two items in the attached pic. Any help would be appreciated.
image

@jgilfillan
Copy link
Author

I've reviewed the linting errors and I can't figure out why the code is failing for most of the exceptions. For example, there's nothing obviously wrong with the two items in the attached pic. Any help would be appreciated. image

@NiallRees hoping you can provide some assistance - I'm at a loss trying to figure out why most of the linting failed.

@NiallRees
Copy link
Contributor

I've reviewed the linting errors and I can't figure out why the code is failing for most of the exceptions. For example, there's nothing obviously wrong with the two items in the attached pic. Any help would be appreciated. image

@NiallRees hoping you can provide some assistance - I'm at a loss trying to figure out why most of the linting failed.

Hi @jgilfillan - don't worry about those. I'll try and review the PR this week!

DBT_CLOUD_RUN_REASON: "String with 'quotes' !"
TEST_ENV_VAR_1: TEST_VALUE
DBT_ENV_CUSTOM_ENV_FAVOURITE_DBT_PACKAGE: dbt_artifacts
# These are configured in GitHub secrets
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't these need to be under the env key?

Copy link
Author

Choose a reason for hiding this comment

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

Right you are, I've updated my branch with this change.

@@ -10,10 +10,10 @@ models:
- unique

exposures:
- name: "ceo's imaginary dashboard"
- name: "ceos_imaginary_dashboard"
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the thinking here? :)

Copy link
Author

Choose a reason for hiding this comment

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

dbt 1.3 gave a warning that spaces in the name will not be permitted in a future version. Also with sql server the spaces were failing the ci tests, so killed two birds with one stone.

@jgilfillan jgilfillan temporarily deployed to Approve Integration Tests December 1, 2022 09:42 Inactive
@jgilfillan jgilfillan temporarily deployed to Approve Integration Tests December 1, 2022 09:42 Inactive
@jgilfillan jgilfillan temporarily deployed to Approve Integration Tests December 1, 2022 09:42 Inactive
@jgilfillan jgilfillan temporarily deployed to Approve Integration Tests December 1, 2022 09:42 Inactive
@jgilfillan
Copy link
Author

@NiallRees is there anything else you're waiting on from me to accept this pr?

@glsdown
Copy link
Contributor

glsdown commented Mar 31, 2023

Hi @jgilfillan thank you for contributing with this, and I'm really sorry that our response has been delayed. We are going to work through the issue backlog now, and get to reviewing this again as soon as possible.

@glsdown glsdown added the enhancement New feature or request label Mar 31, 2023
@sql-chris-albert
Copy link

Hi @jgilfillan thank you for contributing with this, and I'm really sorry that our response has been delayed. We are going to work through the issue backlog now, and get to reviewing this again as soon as possible.

Any progress on approving this PR?

@jgilfillan
Copy link
Author

jgilfillan commented Oct 25, 2023

Hi @jgilfillan thank you for contributing with this, and I'm really sorry that our response has been delayed. We are going to work through the issue backlog now, and get to reviewing this again as soon as possible.

@glsdown It would be good to get an indication if there are any plans to merge this code. The SQL Server compatibility was added on the advice from this project that it would be accepted. So if this has changed it would be good to communicate this so alternative plans can be made (for SQL Server, Synapse, Fabric).

@l-t-k
Copy link

l-t-k commented Feb 28, 2024

@glsdown can you please give us an update on this one?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants