Google Ads API Update #27
Conversation
fivetran-joemarkiewicz
requested review from
fivetran-jamie and
fivetran-reneeli
| @@ -11,7 +11,7 @@ with base as ( | |||
| 'Google Ads' as platform, | |||
| cast(date_day as date) as date_day, | |||
| account_name, | |||
| cast(external_customer_id as {{ dbt_utils.type_string() }}) as account_id, | |||
| {% if var('api_source','adwords') == 'google_ads' %} account_id {% else %} external_customer_id as account_id {% endif %} , | |||
There was a problem hiding this comment.
I'm not too familiar with variables, are they always lowercased? There's not going to be a chance of it being GOOGLE_ADS or something?
There was a problem hiding this comment.
Good call out @fivetran-reneeli! The variable is set by the user so they very well could in fact set the result to GOOGLE_ADS. However, we explicitly tell them that they should use google_ads in the README so I believe it is safe to strictly look just for the lowercase variable result.
There was a problem hiding this comment.
If we are using a variable however where we do not explicitly tell the user what to input (if they want to add passthrough columns) it is always a good idea to standardize the results so it can be caught if the users changes the casing.
There was a problem hiding this comment.
^ yah could be to add a | lower filter when comparing the variable value to google_ads
There was a problem hiding this comment.
or alternatively point out that it's case sensitive more
There was a problem hiding this comment.
Yeah I agree we should have added a | lower filter. However, this filter is not present in the other references of this variable within google_ads (which has already been released) for consistency I will just make a point to call out that this is case sensitive for now.
Thanks team!!
README.md
Outdated
| @@ -104,7 +104,18 @@ models: | |||
| snapchat_ads_source: | |||
| enabled: false | |||
| ``` | |||
| ### Google Ads API Configuration | |||
| If your connector is setup using the Google Ads API then you will need to configure your `dbt_project.yml` with the below variable: | |||
There was a problem hiding this comment.
will users know what we're talking about here? like should we explain what this difference is at all?
There was a problem hiding this comment.
Yeah good call, let me lift our wording from Google Ads and paste here to be more descriptive.
| @@ -11,7 +11,7 @@ with base as ( | |||
| 'Google Ads' as platform, | |||
| cast(date_day as date) as date_day, | |||
| account_name, | |||
| cast(external_customer_id as {{ dbt_utils.type_string() }}) as account_id, | |||
| {% if var('api_source','adwords') == 'google_ads' %} account_id {% else %} external_customer_id as account_id {% endif %} , | |||
There was a problem hiding this comment.
^ yah could be to add a | lower filter when comparing the variable value to google_ads
| @@ -11,7 +11,7 @@ with base as ( | |||
| 'Google Ads' as platform, | |||
| cast(date_day as date) as date_day, | |||
| account_name, | |||
| cast(external_customer_id as {{ dbt_utils.type_string() }}) as account_id, | |||
| {% if var('api_source','adwords') == 'google_ads' %} account_id {% else %} external_customer_id as account_id {% endif %} , | |||
There was a problem hiding this comment.
or alternatively point out that it's case sensitive more
Are you a current Fivetran customer?
Fivetran created PR
What change(s) does this PR introduce?
This PR introduces the functionality of the ad reporting package to leverage the Google Ads API version of the connector if the customer has configured it as such.
Does this PR introduce a breaking change?
As this change integrates the Google Ads breaking change we will want to list this as a breaking change as well.
Is this PR in response to a previously created Issue
How did you test the PR changes?
Select which warehouse(s) were used to test the PR
Provide an emoji that best describes your current mood
📟
Feedback
We are so excited you decided to contribute to the Fivetran community dbt package! We continue to work to improve the packages and would greatly appreciate your feedback on our existing dbt packages or what you'd like to see next.