Integrate upstream package variables into Quickstart#179
Conversation
fivetran-jamie
changed the title
fivetran-avinash
left a comment
fivetran-avinash
left a comment
There was a problem hiding this comment.
@fivetran-jamie Few comments before approval!
| pinterest__campaign_report_passthrough_metrics: | ||
| type: dictionary | ||
| description: "Additional `CAMPAIGN_REPORT` columns to include in `pinterest_ads__campaign_report`" | ||
| display_name: "Pinterest Ads: Additional Campaign Report Metrics" | ||
| pinterest__keyword_report_passthrough_metrics: | ||
| type: dictionary | ||
| description: "Additional `KEYWORD_REPORT` columns to include in `pinterest_ads__keyword_report`" | ||
| display_name: "Pinterest Ads: Additional Keyword Report Metrics" |
There was a problem hiding this comment.
For future reference (doesn't need to be fixed now): the quickstart.yml descriptions are correct here but incorrect in dbt_pinterest.
There was a problem hiding this comment.
Yeah I noticed a few issues -- will make a ticket after this to update the packages
Co-authored-by: Avinash Kunnath <108772760+fivetran-avinash@users.noreply.github.com>
fivetran-avinash
left a comment
fivetran-avinash
left a comment
There was a problem hiding this comment.
@fivetran-jamie approved for pre-release
fivetran-jamie
removed
docs:ready
fivetran-avinash
left a comment
fivetran-avinash
left a comment
There was a problem hiding this comment.
@fivetran-jamie few comments before approval
| - facebook_ads__url_report | ||
| - facebook_ads__url_tags | ||
| - facebook_ads__country_report | ||
| - facebook_ads__region_report |
There was a problem hiding this comment.
It looks we still haven't brought in facebook_ads__conversion_action_types for both facebook_ads and ad_reporting even though we have conversions brought in other packages, are there complexities here too?
There was a problem hiding this comment.
Yeah definitely too complex to have in here for now -- it involves some string parsing that can break easily
| description: "Which `*_sale_amount` fields should be included in calculating the total value of conversions" | ||
| display_name: "Twitter Ads: Conversion Value Fields" | ||
|
|
||
| table_variables: |
There was a problem hiding this comment.
looks like we have a few non-essential tables in the individual packages that feed into some end models. do we need to bring these in here?
amazon_ads: _fivetran_quickstart_product_ad_history for product_ad_history
facebook_ads: _fivetran_quickstart_basic_ad_actions for basic_ad_actions
facebook_ads: _fivetran_quickstart_basic_ad_action_values for basic_ad_action_values snapchat_ads: _fivetran_quickstart_creative_url_tag_history for creative_url_tag_history
There was a problem hiding this comment.
Ah yep lemme add
There was a problem hiding this comment.
Added but adjusted the variable names slightly to indicate which source they belong to (_fivetran_quickstart_<source name>_*)
…tran/dbt_ad_reporting into pre-release/quickstart-vars
fivetran-avinash
left a comment
fivetran-avinash
left a comment
There was a problem hiding this comment.
Approved for release once changes applied.
2 participants
PR Overview
Package version introduced in this PR:
This PR addresses the following Issue/Feature(s):
Summary of changes:
Submission Checklist
Changelog