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

dbt-core 1.5.x and 1.6.0 support #36

Merged
merged 4 commits into from
Aug 9, 2023

Conversation

ciklista
Copy link
Contributor

@ciklista ciklista commented Jul 19, 2023

Adding support for dbt-core 1.5.x

⚠️ Breaks support for older versions of dbt-core. For older versions of dbt-core, users would have to install prior dbt-invoke versions. ⚠️

Given the many changes in the dbt-core API, I'd argue that backwards compatibility increases code complexity of this project to the extend that it becomes to much of a burden to keep the code clean and simple. Also, this allows for a cleaner cut to switch to dbt-duckdb for testing.

Addresses #34

Changes:

  • migrating to dbt-duckdb for testing
    • running dbt project as part of the tests
    • fixing tests for new columns in seeds (dbt_scd_id and dbt_updated_at)
  • fixing issues with new json log formats
  • black replaced all single quotes with double quotes. I hope this is fine ;)

@ciklista ciklista marked this pull request as draft July 19, 2023 20:14
@ciklista ciklista force-pushed the dbt-core-1-5-support branch 2 times, most recently from c39fa3f to 9650a07 Compare July 20, 2023 06:28
@ciklista ciklista changed the title [wip] dbt-core 1.5.x support dbt-core 1.5.x support Jul 20, 2023
@ciklista ciklista marked this pull request as ready for review July 20, 2023 06:55
@ciklista
Copy link
Contributor Author

@robastel feel free to review.
Could you also remove the tests for dbt-core <1.5.x from the expected tests in case you agree to drop support for older dbt-core versions?

@robastel
Copy link
Member

Thanks for submitting this pull request @ciklista !

I need a bit of time to review. It looked like a lot of changes, but I see much of it is just changing single quotes to double quotes, as you mentioned.

Black has an option -S for skipping string normalization, as they call it: https://black.readthedocs.io/en/stable/usage_and_configuration/the_basics.html#s-skip-string-normalization

For example, I use that option in the lint job here: https://github.com/Dashlane/dbt-invoke/blob/c749c72698083e39f2d85659ca225c99235b8968/.github/workflows/test.yml#L26C8-L26C8

Are you able to use that option to get rid of the quote changes (or at least separate them into their own commit) so that it is easier to review? I want avoid overlooking an actual logic change in the sea of quote changes.

Thanks again for taking this on!

@ciklista
Copy link
Contributor Author

ciklista commented Jul 22, 2023

Thanks for the feedback @robastel, I reverted the quotes changes.

@robastel
Copy link
Member

Thanks for the feedback @robastel, I reverted the quotes changes.

Thanks again @ciklista, that definitely helped the readability for me. I've done a quick read through of the changes and plan to take a more thorough look in the coming days.

@ciklista
Copy link
Contributor Author

ciklista commented Aug 8, 2023

@robastel did you have the chance to look into the code? Are there any open questions?

Copy link
Member

@robastel robastel left a comment

Choose a reason for hiding this comment

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

Sorry for the review delay @ciklista and thanks again for this PR!

The general structure makes sense to me.

I have left some comments/suggestions for you to consider. Feel free to push back if there is anything you disagree with or feel should be considered separately from this PR.

.github/workflows/test.yml Outdated Show resolved Hide resolved
.github/workflows/test.yml Outdated Show resolved Hide resolved
tests/test.py Outdated Show resolved Hide resolved
tests/test.py Outdated Show resolved Hide resolved
tests/test.py Show resolved Hide resolved
tests/dbt.duckdb Outdated Show resolved Hide resolved
tests/data_files/customers.parquet Outdated Show resolved Hide resolved
dbt_invoke/properties.py Outdated Show resolved Hide resolved
dbt_invoke/internal/_utils.py Outdated Show resolved Hide resolved
@ciklista ciklista changed the title dbt-core 1.5.x support dbt-core 1.5.x and 1.6.0 support Aug 9, 2023
@ciklista ciklista requested a review from robastel August 9, 2023 07:21
@ciklista ciklista requested a review from robastel August 9, 2023 14:07
Copy link
Member

@robastel robastel left a comment

Choose a reason for hiding this comment

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

Thank you @ciklista !

@robastel robastel merged commit 1f75eeb into Dashlane:main Aug 9, 2023
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants