search

dbt-msft / dbt-sqlserver

dbt adapter for SQL Server and Azure SQL
MIT License
205 stars 96 forks source link

DBT 1.8 and test coverage updates #513

Closed cody-scott closed 2 weeks ago

cody-scott commented 3 weeks ago

Updates the adapter to support 1.8 and implements most of the dbt-test-adapters tests to fix the coverage issues.

Additionally fixes some regressions created from the 1.4 => 1.7 move.

This merge will likely collide with other PRs including https://github.com/dbt-msft/dbt-sqlserver/pull/502, https://github.com/dbt-msft/dbt-sqlserver/pull/504, https://github.com/dbt-msft/dbt-sqlserver/pull/474, https://github.com/dbt-msft/dbt-sqlserver/pull/487

Tests

The test coverage has intentionally not inherited from any fabric tests; only importing from the core dbt-test-adapter.

The key idea is if the fabric adapter makes a breaking change, this adapter should fail loudly with respect to their changes, instead of being masked by their own coverage passing.

Coverage is passing, so outstanding issues such as https://github.com/dbt-msft/dbt-sqlserver/discussions/481 should be able to be better addressed, if they are still failing post update.

cody-scott commented 3 weeks ago

@schlich Can you take a look at this PR please?

This should implement much better coverage of tests and bring the adapter to 1.8

schlich commented 3 weeks ago

@cody-scott went ahead and ran CI, let me know if there's anything you want eyes on in particular otherwise i trust your judgement!

schlich commented 3 weeks ago

hey @cody-scott i suggest importing the futures annotations instead of reverting to the old typing syntax... just pushed to my branch a few minutes ago and it's running in CI, i'll leave it up to you to merge my changes w yours

cody-scott commented 3 weeks ago

@schlich I'd be OK with keeping the old type syntax for now, or at least until DBT formally drops support for 3.8.

Its passing in the current state, so i'd be open to releasing this version as a pre-release for feedback over the next week, then once that is good adopting other changes. Thoughts?

Its still looking like that | causes failures in the test jobs on the #514 checks.

schlich commented 3 weeks ago

Sounds fine by me!

cody-scott commented 2 weeks ago

For a pre-release do you merge the changes then push a release as pre with this off the main/master branch? What is the normal flow for this one?

schlich commented 2 weeks ago

Yeah you've pretty much got it, it's been a minute and it was a bit of trial and error with the 2 releases i did but i think this one was relatively clean: #483

IIRC i just merged all the changes then opened a release PR w/its own release commit that updated the version number and I think CI/CD took care of the rest.