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

fix unit tests related to new serialization api #150

Merged
merged 2 commits into from
Feb 23, 2021

Conversation

kwigley
Copy link

@kwigley kwigley commented Feb 22, 2021

Description

This PR updates dbt-spark to use new serialization API that is the result of using Mashumaro in dbt-core. It also addresses failing unit tests as a result of updated the dbt-core version to 0.19.1b2

Checklist

  • I have signed the CLA
  • I have run this code in development and it appears to resolve the stated issue
  • This PR includes tests, or tests are not required/relevant for this PR
  • I have updated the CHANGELOG.md and added information about my change to the "dbt next" section.

@kwigley kwigley requested review from gshank and jtcohen6 February 22, 2021 15:47
@kwigley kwigley self-assigned this Feb 22, 2021
@cla-bot cla-bot bot added the cla:yes label Feb 22, 2021
@kwigley kwigley force-pushed the fix/serialization-unit-tests branch from 9b55f42 to 0f8fbab Compare February 22, 2021 15:49
Copy link
Contributor

@jtcohen6 jtcohen6 left a comment

Choose a reason for hiding this comment

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

Nice one! I hope this wasn't too too tricky to debug.

Today, I guess it's fair game that breaking changes to dbt internals relevant to some adapter plugins can happen in patch releases, as long as it's not breaking for any end users. After v1.0, we should think about how plausible it is to keep plugin-facing changes within minor versions.

@gshank
Copy link
Contributor

gshank commented Feb 22, 2021

This looks good, but unfortunately the Mashumaro likely solution for handling omit_none columns is to go back to using 'omit_none', something which I'm going to switch to in dbt core this week. We might want to wait for a few days for that.

@kwigley
Copy link
Author

kwigley commented Feb 22, 2021

@jtcohen6 100% agree, I'm interested in how we create and enforce this contract in core for different plugins. Will keep this in mind

@kwigley
Copy link
Author

kwigley commented Feb 22, 2021

@gshank will the changes involve just renaming keep_none to omit_none? If so, I'm happy to merge this as is so we can release dbt-spark 0.19.1b2 and update the code with the mashumaro changes once they are merged. Let me know if that is not the case, I will hold off from merging

@kwigley kwigley merged commit 46b2881 into master Feb 23, 2021
@kwigley kwigley deleted the fix/serialization-unit-tests branch February 23, 2021 13:55
@kwigley
Copy link
Author

kwigley commented Feb 23, 2021

Decided to merge this, I'm happy to update with any 0.19.1 changes

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

Successfully merging this pull request may close these issues.

3 participants