-
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,13 @@ | ||
| name: 'generate dbt docs' | ||
| on: | ||
| pull_request: | ||
| types: | ||
| - labeled | ||
|
|
||
| jobs: | ||
| generate-docs: | ||
| if: github.event.label.name == 'docs:ready' | ||
| uses: fivetran/dbt_package_automations/.github/workflows/generate-docs.yml@main | ||
| secrets: inherit | ||
| with: | ||
| schema_var_name: linkedin_schema |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,7 +1,73 @@ | ||
| target/ | ||
| env/ | ||
| # dbt | ||
| **/package-lock.yml | ||
| package-lock.yml | ||
| .dbt/ | ||
| dbt_modules/ | ||
| dbt_packages/ | ||
| logs/ | ||
| profiles.yml | ||
| target/ | ||
| *.log | ||
|
|
||
| # IDE files | ||
| .idea/ | ||
| .vscode/ | ||
| *~ | ||
| *.swp | ||
| *.swo | ||
|
|
||
| # Jupyter Notebook | ||
| .ipynb_checkpoints | ||
|
|
||
| # OS generated files | ||
| **/.DS_Store | ||
| .DS_Store | ||
| dbt_packages/ | ||
| package-lock.yml | ||
| .Spotlight-V100 | ||
| .Trashes | ||
| ._* | ||
| Thumbs.db | ||
| ehthumbs.db | ||
|
|
||
| # Python | ||
| *.egg | ||
| *.egg-info/ | ||
| *.py[cod] | ||
| *.so | ||
| *$py.class | ||
| .Python | ||
| __pycache__/ | ||
| build/ | ||
| develop-eggs/ | ||
| dist/ | ||
| downloads/ | ||
| eggs/ | ||
| .env | ||
| .installed.cfg | ||
| lib/ | ||
| lib64/ | ||
| MANIFEST | ||
| parts/ | ||
| sdist/ | ||
| var/ | ||
| wheels/ | ||
|
|
||
| # Secrets and credentials | ||
| .env.* | ||
| .secrets | ||
| credentials.json | ||
| service-account.json | ||
|
|
||
| # Temporary files | ||
| .cache/ | ||
| *.temp | ||
| *.tmp | ||
|
|
||
| # Virtual environments | ||
| .conda/ | ||
| .env | ||
| .venv | ||
| ENV/ | ||
| env/ | ||
| env.bak/ | ||
| venv/ | ||
| venv.bak/ |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,3 +1,29 @@ | ||
| # dbt_linkedin v1.0.0 | ||
|
|
||
| [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. | ||
|
|
||
| ### 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: | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. People will also need to adjust any |
||
| - Remove `fivetran/linkedin_source` from `packages.yml` | ||
| - Delete any `source:` overrides that reference the source package | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
|
|
||
| ### dbt Fusion Compatibility Updates | ||
| - For compatibility with dbt Fusion versions >= 1.10.6: | ||
| - Removed `dbt_utils.unique_combination_of_columns` tests | ||
| - Removed `accepted_values` tests | ||
|
Comment on lines
+16
to
+17
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? |
||
| - In src_linkedin.yml, moved `loaded_at_field: _fivetran_synced` under the `config:` block. | ||
|
|
||
| ### Schema Configuration Updates | ||
| - Some customizations have changed, refer to the [README](https://github.com/fivetran/dbt_linkedin/blob/main/README.md) for details. | ||
|
|
||
| ## Under the Hood | ||
| - Updated conditions in `.github/workflows/auto-release.yml`. | ||
| - Added `.github/workflows/generate-docs.yml`. | ||
|
|
||
| # dbt_linkedin v0.12.0 | ||
|
|
||
| [PR #45](https://github.com/fivetran/dbt_linkedin/pull/45) includes the following updates: | ||
|
|
||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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! |
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -16,7 +16,7 @@ | |||||
| </p> | ||||||
|
|
||||||
| ## 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). | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||||||
| - Enables you to better understand the performance of your ads across varying grains: | ||||||
| - Providing an account, campaign (ad groups in other ad platforms), campaign group (campaigns in other ad platforms), creative, and utm/url level reports. | ||||||
| - Materializes output models designed to work simultaneously with our [multi-platform Ad Reporting package](https://github.com/fivetran/dbt_ad_reporting). | ||||||
|
|
@@ -61,9 +61,9 @@ Include the following Linkedin Ads package version in your `packages.yml` file: | |||||
| # packages.yml | ||||||
| packages: | ||||||
| - package: fivetran/linkedin | ||||||
| version: [">=0.12.0", "<0.13.0"] | ||||||
| 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`. | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Maybe |
||||||
|
|
||||||
| ### Step 3: Define database and schema variables | ||||||
| By default, this package runs using your destination and the `linkedin_ads` schema. If this is not where your Linkedin Ad Analytics data is (for example, if your Linkedin schema is named `linkedin_ads_fivetran`), add the following configuration to your root `dbt_project.yml` file: | ||||||
|
|
@@ -169,7 +169,7 @@ models: | |||||
| If an individual source table has a different name than the package expects, add the table name as it appears in your destination to the respective variable. This is not available when running the package on multiple unioned connections. | ||||||
|
|
||||||
| > IMPORTANT: See this project's [`dbt_project.yml`](https://github.com/fivetran/dbt_linkedin/blob/main/dbt_project.yml) variable declarations to see the expected names. | ||||||
|
|
||||||
| ```yml | ||||||
| # dbt_project.yml | ||||||
| vars: | ||||||
|
|
@@ -188,8 +188,6 @@ This dbt package is dependent on the following dbt packages. These dependencies | |||||
| > IMPORTANT: If you have any of these dependent packages in your own `packages.yml` file, we highly recommend that you remove them from your root `packages.yml` to avoid package version conflicts. | ||||||
| ```yml | ||||||
| packages: | ||||||
| - package: fivetran/linkedin_source | ||||||
| version: [">=0.12.0", "<0.13.0"] | ||||||
| - package: fivetran/fivetran_utils | ||||||
| version: [">=0.4.0", "<0.5.0"] | ||||||
| - package: dbt-labs/dbt_utils | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -1,18 +1,18 @@ | ||||||
| name: 'linkedin' | ||||||
| version: '0.12.0' | ||||||
| version: '1.0.0' | ||||||
| config-version: 2 | ||||||
| require-dbt-version: [">=1.3.0", "<2.0.0"] | ||||||
| vars: | ||||||
| linkedin: | ||||||
| ad_analytics_by_creative: "{{ ref('stg_linkedin_ads__ad_analytics_by_creative') }}" | ||||||
| creative_history: "{{ ref('stg_linkedin_ads__creative_history') }}" | ||||||
| campaign_history: "{{ ref('stg_linkedin_ads__campaign_history') }}" | ||||||
| campaign_group_history: "{{ ref('stg_linkedin_ads__campaign_group_history') }}" | ||||||
| account_history: "{{ ref('stg_linkedin_ads__account_history') }}" | ||||||
| ad_analytics_by_campaign: "{{ ref('stg_linkedin_ads__ad_analytics_by_campaign') }}" | ||||||
| geo: "{{ ref('stg_linkedin_ads__geo') }}" | ||||||
| monthly_ad_analytics_by_member_country: "{{ ref('stg_linkedin_ads__monthly_ad_analytics_by_country') }}" | ||||||
| monthly_ad_analytics_by_member_region: "{{ ref('stg_linkedin_ads__monthly_ad_analytics_by_region') }}" | ||||||
| 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') }}" | ||||||
|
Comment on lines
+7
to
+15
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes |
||||||
| linkedin_ads__creative_passthrough_metrics: [] | ||||||
| linkedin_ads__campaign_passthrough_metrics: [] | ||||||
| linkedin_ads__monthly_ad_analytics_by_member_country_passthrough_metrics: [] | ||||||
|
|
@@ -23,3 +23,6 @@ models: | |||||
| linkedin: | ||||||
| +materialized: table | ||||||
| +schema: linkedin_ads | ||||||
| staging: | ||||||
| +schema: linkedin_source | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
| +materialized: view | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,20 @@ | ||
| {% macro date_from_month_string(month_str) %} | ||
| {{ return(adapter.dispatch('date_from_month_string', 'linkedin')(month_str)) }} | ||
| {% endmacro %} | ||
|
|
||
| {% macro default__date_from_month_string(month_str) %} | ||
| to_date( | ||
| split_part({{ month_str }}, '-', 1) || '-' || lpad(split_part({{ month_str }}, '-', 2), 2, '0') || '-01', | ||
| 'YYYY-MM-DD' | ||
| ) | ||
| {% endmacro %} | ||
|
|
||
| {% macro bigquery__date_from_month_string(month_str) %} | ||
| parse_date('%Y-%m-%d', concat(split({{ month_str }}, '-')[offset(0)], '-', lpad(split({{ month_str }}, '-')[offset(1)], 2, '0'), '-01')) | ||
| {% endmacro %} | ||
|
|
||
| {% macro databricks__date_from_month_string(month_str) %} | ||
| to_date( | ||
| concat(split({{ month_str }}, '-')[0], '-', lpad(split({{ month_str }}, '-')[1], 2, '0'), '-01') | ||
| ) | ||
| {% endmacro %} |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,16 @@ | ||
| {% macro get_account_history_columns() %} | ||
|
|
||
| {% set columns = [ | ||
| {"name": "created_time", "datatype": dbt.type_timestamp()}, | ||
| {"name": "currency", "datatype": dbt.type_string()}, | ||
| {"name": "id", "datatype": dbt.type_int()}, | ||
| {"name": "last_modified_time", "datatype": dbt.type_timestamp()}, | ||
| {"name": "name", "datatype": dbt.type_string()}, | ||
| {"name": "status", "datatype": dbt.type_string()}, | ||
| {"name": "type", "datatype": dbt.type_string()}, | ||
| {"name": "version_tag", "datatype": dbt.type_string()} | ||
| ] %} | ||
|
|
||
| {{ return(columns) }} | ||
|
|
||
| {% endmacro %} |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,20 @@ | ||
| {% macro get_ad_analytics_by_campaign_columns() %} | ||
|
|
||
| {% set columns = [ | ||
| {"name": "campaign_id", "datatype": dbt.type_int()}, | ||
| {"name": "clicks", "datatype": dbt.type_int()}, | ||
| {"name": "cost_in_local_currency", "datatype": dbt.type_numeric()}, | ||
| {"name": "cost_in_usd", "datatype": dbt.type_numeric()}, | ||
| {"name": "day", "datatype": dbt.type_timestamp()}, | ||
| {"name": "impressions", "datatype": dbt.type_int()}, | ||
| {"name": "conversion_value_in_local_currency", "datatype": dbt.type_numeric()} | ||
| ] %} | ||
|
|
||
| {{ fivetran_utils.add_pass_through_columns(columns, var('linkedin_ads__conversion_fields')) }} | ||
|
|
||
| {# Doing it this way in case users were bringing in conversion metrics via passthrough columns prior to us adding them by default #} | ||
| {{ linkedin_ads_add_pass_through_columns(base_columns=columns, pass_through_fields=var('linkedin_ads__campaign_passthrough_metrics'), except_fields=(var('linkedin_ads__conversion_fields') + ['conversion_value_in_local_currency'])) }} | ||
|
|
||
| {{ return(columns) }} | ||
|
|
||
| {% endmacro %} |
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.