-
Notifications
You must be signed in to change notification settings - Fork 2
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 var for CALL
+ update way we disable source freshness
#6
Conversation
Feature: add flag for call models
question for you @raphaelvarieras -- i'm curious about the have you found yourself or others to use Twilio data without any messages? |
No, not me or anybody I know. I added this in reaction of the commit from @fivetran-reneeli from last year where they added a meta |
@raphaelvarieras cool yeah I see, then I may revert the addition of |
CALL
and MESSAGE
+ update way we disable source freshnessCALL
+ update way we disable source freshness + add dbt_utils.star
CALL
+ update way we disable source freshness + add dbt_utils.starCALL
+ update way we disable source freshness
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work on this PR @fivetran-jamie! I just have a few small suggestions and one main one to consider making this a minor upgrade instead of a patch. Let me know if you have any questions.
Co-authored-by: Raphael Varieras <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One small request to update the CHANGELOG. Otherwise LGTM!
CHANGELOG.md
Outdated
@@ -1,3 +1,22 @@ | |||
# dbt_twilio_source v0.2.0 | |||
|
|||
## Breaking Changes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add the red lights here to bring attention to the breaking change
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few small notes before approving
## 🚨 Breaking Changes 🚨 | ||
- Under the hood, we've updated the `_tmp` models to use the `dbt_utils.star` macro instead of a basic `select *` ([PR #6](https://github.com/fivetran/dbt_twilio_source/pull/6)). | ||
- This means that you can no longer use `var(<table_name>)` to override the source tables we create staging models from. Instead, see the [README](https://github.com/fivetran/dbt_twilio_source?tab=readme-ov-file#change-the-source-table-references) for how to use our `_identifier` variables. | ||
- Removed the deprecated `twilio_using_message` variable. This is a breaking change because you could previously use this variable to disable freshness tests on the `MESSAGE` source table. To continue to do so, leverage dbt [overrides](https://docs.getdbt.com/reference/resource-properties/overrides#configure-your-own-source-freshness-for-a-source-table-in-a-package) to set `message`'s freshness to `null` ([PR #6](https://github.com/fivetran/dbt_twilio_source/pull/6)). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you know why we're capitalizing MESSAGE
here? The source table is message
in the dbt_project.yml
. Is it because the customer is using Snowflake?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just capitalizing it to highlight it as a source table (as opposed to a package model or a column)
- Removed the deprecated `twilio_using_message` variable. This is a breaking change because you could previously use this variable to disable freshness tests on the `MESSAGE` source table. To continue to do so, leverage dbt [overrides](https://docs.getdbt.com/reference/resource-properties/overrides#configure-your-own-source-freshness-for-a-source-table-in-a-package) to set `message`'s freshness to `null` ([PR #6](https://github.com/fivetran/dbt_twilio_source/pull/6)). | ||
|
||
## Features | ||
- Added the ability to disable models related to the `CALL` source table. Refer to the [README](https://github.com/fivetran/dbt_twilio_source?tab=readme-ov-file#step-4-enablingdisabling-models) for more details ([PR #5](https://github.com/fivetran/dbt_twilio_source/pull/5)). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar question regarding CALL
vs. call
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just a stylistic choice - i tend to capitalize raw source tables in documentation to highlight that they're not package models or columns
I just noticed another issue and I wonder if we want to try to address it in this release maybe? I noticed today that the |
@raphaelvarieras that is a good callout, but would you mind creating a new issue for that? I'd prefer to tackle in it another release to ensure we have ample time to figure out each field we'd want to (and logically should) cast as an integer, as I see that other tables have quite a few numeric fields cast as strings (ex: |
PR Overview
This PR will address the following Issue/Feature:
This PR will result in the following new package version:
v0.1.1
Please provide the finalized CHANGELOG entry which details the relevant changes included in this PR:
Features
CALL
andMESSAGE
source tables. Refer to the README for more details (PR #5).Under the Hood
src_twilio.yml
(PR #5).config.enabled
flag (PR #6).PR Checklist
Basic Validation
Please acknowledge that you have successfully performed the following commands locally:
Before marking this PR as "ready for review" the following have been applied:
Detailed Validation
Please share any and all of your validation steps:
@raphaelvarieras confirmed
dbt build -s package:twilio_source
was successful, but i'll also validate given i made additional updates:using_twilio_call Addition
By default, the
call
models still run:After setting
using_twilio_call: False
in mydbt_project.yml
,call
models are not executed:Freshness Update
On main (with functional but non-native code for disabling sources),
dbt source freshness
yields 3 runs (note that i am using stale data)adding
using_twilio_call: false
to my dbt_project.yml makes it so thatdbt source freshness
yields 3 runs, but now with thecall
test passing (which is expected from our custom freshness macro)On this branch,
dbt source freshness
without variable configs still gives us 3 failing testsAnd after setting
using_twilio_call: false
, we only see 2 source freshness tests being run now that dbt has natively disabled thecall
sourceIf you had to summarize this PR in an emoji, which would it be?
📞