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

Add bigquery hooks (#779) #836

Merged
merged 20 commits into from
Jul 16, 2018
Merged

Add bigquery hooks (#779) #836

merged 20 commits into from
Jul 16, 2018

Conversation

beckjake
Copy link
Contributor

Add pre/post hook support for bigquery (fixes #779).

@beckjake beckjake requested review from drewbanin and removed request for drewbanin July 10, 2018 17:17
@drewbanin drewbanin self-requested a review July 11, 2018 01:56
Copy link
Contributor

@drewbanin drewbanin left a comment

Choose a reason for hiding this comment

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

One comment here about hooks on seeds. I don't totally understand how these tests can be passing given that the one hook contains invalid SQL for BQ.

Really nice work on these tests though -- the changes to the bq base integration test code are terrific.

'models': {},
'seeds': {
'post-hook': [
'alter table {{ this }} add column new_col int',
Copy link
Contributor

Choose a reason for hiding this comment

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

does this work? alter table ... add column is not supported on BigQuery! I think a better test might just update a column or insert a record

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It turns out that we weren't loading the seed file on Postgres or BigQuery because the extension is .sql instead of .csv, so these hooks never run and the tests always pass.

Good stuff.

@beckjake
Copy link
Contributor Author

@drewbanin Per our in-person/slack discussion, fixed the hook + seed issues you found and added tests. Let me know when you've tested this out and if it's good to go I'll merge it

@drewbanin
Copy link
Contributor

@beckjake just gave this a spin locally and it works great! Feel free to merge when the tests pass

@beckjake beckjake merged commit e86bbe0 into development Jul 16, 2018
@beckjake beckjake deleted the bigquery-hooks branch July 16, 2018 13:13
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.

3 participants