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 snapshot compilation error #2791

Conversation

kingfink
Copy link
Contributor

@kingfink kingfink commented Sep 25, 2020

resolves #2787

Description

Implements strategy-specific validation for the timestamp and check strategies. As a result compilation errors are more relevant and clear to the user.

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.

@cla-bot
Copy link

cla-bot bot commented Sep 25, 2020

Thanks for your pull request, and welcome to our community! We require contributors to sign our Contributor License Agreement and we don't seem to have your signature on file. Check out this article for more information on why we have a CLA.

In order for us to review and merge your code, please submit the Individual Contributor License Agreement form attached above above. If you have questions about the CLA, or if you believe you've received this message in error, don't hesitate to ping @drewbanin.

CLA has not been signed by users: @kingfink

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.

This looks great!

We could add a test for the repro case in #2787, but I don't feel so strongly about it—this doesn't represent a functional change, it just raises a more relevant error.

I see you added the changelog entries under v0.18.1. I think it's perfectly reasonable to get this in sooner rather than later; I'll change the base branch to match.

Edit: Since you branched off dev/kiyoshi-kuromiya, you'd need to rebase onto dev/0.18.1. Let me know if you'd prefer to avoid the rebase and just have this ship in v0.19.0.

@jtcohen6 jtcohen6 changed the base branch from dev/kiyoshi-kuromiya to dev/0.18.1 September 29, 2020 19:44
@jtcohen6 jtcohen6 changed the base branch from dev/0.18.1 to dev/kiyoshi-kuromiya September 29, 2020 19:44
@kingfink
Copy link
Contributor Author

Thanks for the review @jtcohen6 ! I added a test and moved my notes in the changelog to v0.19.0.

@jtcohen6
Copy link
Contributor

Thanks for the contribution @kingfink!

@jtcohen6 jtcohen6 merged commit 89b6e52 into dbt-labs:dev/kiyoshi-kuromiya Sep 30, 2020
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.

Incorrect compilation error thrown for snapshots using check strategy and missing check_cols parameter
2 participants