Release/0.4.0 #10

fivetran-reneeli · 2024-11-18T22:00:15Z

PR Overview

This PR will address the following Issue/Feature:
#10
This PR will result in the following new package version: v0.4.0

Made breaking change since disabling usage_record could impact the fields in the downstream models.

Please provide the finalized CHANGELOG entry which details the relevant changes included in this PR:

Features

Added the ability to disable models related to the USAGE_RECORD source table. Refer to the README for more details.

Under the hood

Removes references to unused models in twilio__message_enhanced.

PR Checklist

Basic Validation

Please acknowledge that you have successfully performed the following commands locally:

dbt run –full-refresh && dbt test
[na] dbt run (if incremental models are present) && dbt test

Before marking this PR as "ready for review" the following have been applied:

The appropriate issue has been linked, tagged, and properly assigned
All necessary documentation and version upgrades have been applied
docs were regenerated (unless this PR does not include any code or yml updates)
BuildKite integration tests are passing
Detailed validation steps have been provided below

Detailed Validation

Please share any and all of your validation steps:

Not much validation since these models were not used in the first place, but just ensuring that the models are removed in the documentation and in any mentions downstream.
And that disabling the usage_record source table via using_twilio_usage_record will still allow downstream models to run, without the usage_record fields.

If you had to summarize this PR in an emoji, which would it be?

💃

fivetran-reneeli · 2024-11-19T15:24:14Z

README.md

@@ -72,6 +72,8 @@ By default, all variables are assumed to be `true`.
 vars:
  using_twilio_call: False # Disable this if not using call
  using_twilio_messaging_service: False # Disable this if not using messaging_service
+  using_twilio_usage_record: False # Disable this if not using usage_record


our standard way would be twilio__using_usage_record, but then I should also update using_twilio_call and using_twilio_messaging_service, which would require users who already have those variables configured to change it, which would be more of a hassle customer-experience wise.

fivetran-joemarkiewicz

LGTM with just a few comments to be addressed before moving to release review. These changes aren't a cause for blocking initial approval, but please be sure to address before opening up for release review and regenerate the docs.

fivetran-joemarkiewicz · 2024-11-20T22:17:55Z

models/twilio__account_overview.sql

@@ -3,20 +3,23 @@
 with messages as (

    select *
-    from {{ ref('int_twilio__messages')}}
+    from {{ ref('int_twilio__messages') }}


Thanks for making these formatting changes to ensure we have consistent formatting!

fivetran-joemarkiewicz · 2024-11-20T22:24:08Z

models/twilio__message_enhanced.sql

-incoming_phone_number as (
-
-    select *
-    from {{ var('incoming_phone_number')}}
-),


Wow I didn't realize incoming phone number was included here as well even with it not being referenced. Thanks for removing!

fivetran-joemarkiewicz · 2024-11-22T19:12:36Z

CHANGELOG.md

+# dbt_twilio v0.4.0
+
+## Features
+- Added the ability to disable models related to the `USAGE_RECORD` source table. Refer to the [README](https://github.com/fivetran/dbt_twilio?tab=readme-ov-file#step-4-enablingdisabling-models) for more details.


Can we include the variable here in the CHANGELOG so customers are immediately aware of it. We can/should still link to the README, but would be good to share the variable as well.

fivetran-joemarkiewicz · 2024-11-22T19:14:32Z

packages.yml

+# - package: fivetran/twilio_source
+#   version: [">=0.3.0", "<0.4.0"]
+
+  - git: https://github.com/fivetran/dbt_twilio_source.git
+    revision: usage_record_enable_config
+    warn-unpinned: false


Reminder to update before merge.

fivetran-joemarkiewicz · 2024-11-22T19:16:16Z

models/twilio__account_overview.sql

+    round( cast(sum(messages.price) as {{ dbt.type_numeric() }}), 2) * -1 as total_messages_spend
+
+    {% if var('using_twilio_usage_record', True) %}
+    , round( cast(sum(usage_record.price) as {{ dbt.type_numeric() }}), 2) as total_account_spend


Can you update the documentation for this field here to add a disclaimer that this will only be present if the using_twilio_usage_record variable is defined as true (default being true).

fivetran-joemarkiewicz · 2024-11-22T19:39:20Z

integration_tests/dbt_project.yml

For this change there is no need for a set of validation tests since this is not directly making any changes to the underlying model logic. However, would you be able to add the following model config so we can use it for review testing and future validation testing once we add it to this package.

models: +schema: "twilio_{{ var('directed_schema','dev') }}"

that's a good point, thanks for dropping that config! i added

CHANGELOG.md

fivetran-avinash · 2024-11-26T16:42:16Z

CHANGELOG.md

+# dbt_twilio v0.4.0
+
+## Features
+- Added the ability to disable the `USAGE_RECORD` source table and its downstream fields via variable `using_twilio_usage_record` (default `true`). Refer to the [README](https://github.com/fivetran/dbt_twilio?tab=readme-ov-file#step-4-enablingdisabling-models) for more details.


Should we call out that this variable can be disabled in twilio__account_overview and total_account_spend will now be removed if this variable is disabled?

Good point, added that language!

fivetran-avinash · 2024-11-26T16:46:11Z

integration_tests/dbt_project.yml

 dispatch:
  - macro_namespace: dbt_utils
    search_order: ['spark_utils', 'dbt_utils']

+models:


More of a question: There weren't any validation tests added, so was this just for future use? Did we think it wouldn't have been useful to add a row comparison test given the changes weren't significant enough to the code?

Yeah! Yup just for future use. No validation tests for this PR since this isn't really changing any model.

fivetran-avinash

@fivetran-reneeli A few comments before approving!

Co-authored-by: Avinash Kunnath <[email protected]>

fivetran-reneeli · 2024-11-26T22:18:08Z

also added updates to address #11

fivetran-reneeli added 2 commits November 18, 2024 16:56

updates

60cbeb4

updates

acae74d

fivetran-reneeli self-assigned this Nov 18, 2024

fivetran-reneeli added 2 commits November 18, 2024 17:13

docs

1c16418

add using_twilio_usage_record variable and update docs

1ba61da

fivetran-reneeli commented Nov 19, 2024

View reviewed changes

fivetran-reneeli added 2 commits November 19, 2024 12:01

edit run script and correct the case for false using usage_record

0a16fdf

add to quickstart

e8e8cfb

fivetran-reneeli requested a review from fivetran-joemarkiewicz November 19, 2024 20:38

fivetran-reneeli added 3 commits November 21, 2024 15:39

add back the previous deleted seeds

8b0a3d7

docs

e56743d

changelog updates

d131790

fivetran-joemarkiewicz approved these changes Nov 22, 2024

View reviewed changes

updates

cd72d7a

fivetran-avinash reviewed Nov 26, 2024

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

fivetran-avinash reviewed Nov 26, 2024

View reviewed changes

fivetran-reneeli and others added 2 commits November 26, 2024 14:10

Update CHANGELOG.md

12457d4

Co-authored-by: Avinash Kunnath <[email protected]>

update

5b008da

fivetran-avinash approved these changes Nov 26, 2024

View reviewed changes

update readme format

53570f1

fivetran-reneeli linked an issue Nov 26, 2024 that may be closed by this pull request

[Documentation] Move badges below H1 heading #11

Open

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Release/0.4.0 #10

Release/0.4.0 #10

fivetran-reneeli commented Nov 18, 2024 •

edited

Loading

fivetran-reneeli Nov 19, 2024

fivetran-joemarkiewicz left a comment

fivetran-joemarkiewicz Nov 20, 2024

fivetran-joemarkiewicz Nov 20, 2024

fivetran-joemarkiewicz Nov 22, 2024

fivetran-reneeli Nov 25, 2024

fivetran-joemarkiewicz Nov 22, 2024

fivetran-joemarkiewicz Nov 22, 2024

fivetran-reneeli Nov 25, 2024

fivetran-joemarkiewicz Nov 22, 2024

fivetran-reneeli Nov 25, 2024

fivetran-avinash Nov 26, 2024

fivetran-reneeli Nov 26, 2024

fivetran-avinash Nov 26, 2024

fivetran-reneeli Nov 26, 2024

fivetran-avinash left a comment

fivetran-reneeli commented Nov 26, 2024

Release/0.4.0 #10

Are you sure you want to change the base?

Release/0.4.0 #10

Conversation

fivetran-reneeli commented Nov 18, 2024 • edited Loading

PR Overview

Features

Under the hood

PR Checklist

Basic Validation

Detailed Validation

If you had to summarize this PR in an emoji, which would it be?

Choose a reason for hiding this comment

fivetran-joemarkiewicz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fivetran-avinash left a comment

Choose a reason for hiding this comment

fivetran-reneeli commented Nov 26, 2024

fivetran-reneeli commented Nov 18, 2024 •

edited

Loading