-
Notifications
You must be signed in to change notification settings - Fork 14
Consolidate source + remove combo tests for linkedin #50
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
Conversation
fivetran-jamie
left a comment
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.
Looks awesome!!!! Left suggestions/comments for the changelog generation
| [PR #50](https://github.com/fivetran/dbt_linkedin/pull/50) includes the following updates: | ||
|
|
||
| ## Breaking Changes | ||
| > A `--full-refresh` is required after upgrading to this release. |
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.
Will this actually be necessary for incremental models?
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.
True--probably not necessary.
| ### Source Package Consolidation | ||
| - Consolidated `dbt_linkedin_source` into this package. | ||
| - All functionality from the source package has been merged into this transformation package. Be sure to: |
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.
People will also need to adjust any linkedin_source-scoped configs in their dbt_project.yml, either to be scoped to just linkedin or to the staging subfolder
| - Consolidated `dbt_linkedin_source` into this package. | ||
| - All functionality from the source package has been merged into this transformation package. Be sure to: | ||
| - Remove `fivetran/linkedin_source` from `packages.yml` | ||
| - Delete any `source:` overrides that reference the source package |
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.
If they are still on a dbt version that supports overrides, they can update the overrides: config to be overrides: linkedin instead of overrides: linkedin_source
| - Removed `dbt_utils.unique_combination_of_columns` tests | ||
| - Removed `accepted_values` tests |
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.
Should we say that we'll likely add these back in in the future once more users are upgraded?
| version: [">=1.0.0", "<1.1.0"] | ||
| ``` | ||
| Do **NOT** include the `linkedin_source` package in this file. The transformation package itself has a dependency on it and will install the source package as well. | ||
| > All required sources are now bundled into this transformation package. Do not include `fivetran/linkedin_source` in your `packages.yml`. |
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.
| > All required sources are now bundled into this transformation package. Do not include `fivetran/linkedin_source` in your `packages.yml`. | |
| > All required sources and staging models are now bundled into this transformation package. Do not include `fivetran/linkedin_source` in your `packages.yml`, as this package has been deprecated. |
Maybe
| ad_analytics_by_creative: "{{ source('linkedin_ads', 'ad_analytics_by_creative') }}" | ||
| creative_history: "{{ source('linkedin_ads', 'creative_history') }}" | ||
| campaign_history: "{{ source('linkedin_ads', 'campaign_history') }}" | ||
| campaign_group_history: "{{ source('linkedin_ads', 'campaign_group_history') }}" | ||
| account_history: "{{ source('linkedin_ads', 'account_history') }}" | ||
| ad_analytics_by_campaign: "{{ source('linkedin_ads', 'ad_analytics_by_campaign') }}" | ||
| geo: "{{ source('linkedin_ads', 'geo') }}" | ||
| monthly_ad_analytics_by_member_country: "{{ source('linkedin_ads', 'monthly_ad_analytics_by_country') }}" | ||
| monthly_ad_analytics_by_member_region: "{{ source('linkedin_ads', 'monthly_ad_analytics_by_region') }}" |
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.
Should we call out this change in the CHANGELOG in case people were utilizing the variables?
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.
yes
| +materialized: table | ||
| +schema: linkedin_ads | ||
| staging: | ||
| +schema: linkedin_source |
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.
| +schema: linkedin_source | |
| +schema: linkedin_ads_source |
* Q2 FY26: Apply automated update. * Q2 FY26: Update auto-release workflow only. * Update CHANGELOG.md --------- Co-authored-by: fivetran-catfritz <111930712+fivetran-catfritz@users.noreply.github.com> Co-authored-by: Joe Markiewicz <74217849+fivetran-joemarkiewicz@users.noreply.github.com>
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.
Not sure if we need to add a staging sub-folder for macros since they're all functions being applied across models. They could all live in the macros folder I think. Also avoids us having to move it in the future in case we were to apply a macro like date_from_month_string beyond the staging layer.
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.
No it's not necessary, but I opted to do this to keep them separated for now since the staging macros can get a bit much. Being in a sub folder should not restrict them to be used by only the staging models. Kind of like how we put the dbt date macros in their own folder.
|
|
||
| ## What does this dbt package do? | ||
| - Produces modeled tables that leverage Linkedin Ad Analytics data from [Fivetran's connector](https://fivetran.com/docs/applications/linkedin-ads) in the format described by [this ERD](https://fivetran.com/docs/applications/linkedin-ads#schemainformation) and builds off the output of our [Linkedin Ads source package](https://github.com/fivetran/dbt_linkedin_source). | ||
| - Produces modeled tables that leverage Linkedin Ad Analytics data from [Fivetran's connector](https://fivetran.com/docs/applications/linkedin-ads) in the format described by [this ERD](https://fivetran.com/docs/applications/linkedin-ads#schemainformation). |
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.
You could leverage this sentence from the Workday package (already consolidated) around how the staging tables are materialized via the ERD.
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.
We will want to update the Changing the Build Schema section to account for the new staging configuration. Example from the Workday package.
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.
Ahh yes good catch. It turns out Linkedin just got messed up because of the linkedin vs linkedin_ads issue, but the other packages will get the update!
|
Thanks everyone for the comments! i'm going to close this PR so we can start fresh with the script. |
Summary
This PR is to combine the source and transform packages and remove tests not compatible with dbt Fusion 1.10.6+.
Please check the below automated updates were successful:
Source + Transform Consolidation
dbt Fusion Compatibility
dbt_utils.unique_combination_of_columnstestYAML & Config Updates
+schemaand+materializationfor the staging models indbt_project.yml.README