Skip to content
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

Feature/v2 updates #29

Merged
merged 36 commits into from
Sep 1, 2022
Merged

Feature/v2 updates #29

merged 36 commits into from
Sep 1, 2022

Conversation

fivetran-joemarkiewicz
Copy link
Contributor

Are you a current Fivetran customer?

Fivetran created PR

What change(s) does this PR introduce?

This PR applies the Ad Reporting V2 updates. In particular this PR does the following:

  • Adds the ad_group_criterion_history source and accompanying staging models
  • Adds the ad_group_stats source and accompanying staging models
  • Adds the campaign_stats source and accompanying staging models
  • Adds the account_stats source and accompanying staging models
  • Adds the keyword_stats source and accompanying staging models
  • Adds tests to each of the new and existing end models
  • Completely removes the Adwords versions of the package as it has been fully sunset
  • Applies README updates
  • Adds identifiers to all source models

Did you update the CHANGELOG?

  • Yes

Does this PR introduce a breaking change?

  • Yes (please provide breaking change details below.)
  • No (please provide an explanation as to how the change is non-breaking below.)

This will be a major update and will be breaking for existing users.

Did you update the dbt_project.yml files with the version upgrade (please leverage standard semantic versioning)? (In both your main project and integration_tests)

  • Yes

Is this PR in response to a previously created Bug or Feature Request

  • Yes, Issue/Feature [link bug/feature number here]
  • No

How did you test the PR changes?

  • CircleCi
  • Local (please provide additional testing details below)

Select which warehouse(s) were used to test the PR

  • BigQuery
  • Redshift
  • Snowflake
  • Postgres
  • Databricks
  • Other (provide details below)

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.

@fivetran-joemarkiewicz fivetran-joemarkiewicz changed the base branch from main to feature/google-ads-api-default June 22, 2022 19:40
@fivetran-joemarkiewicz
Copy link
Contributor Author

FYI the base branch is feature/google-ads-api-default as that branch will be merged prior to this one and I want to make sure I am not duplicating reviews from the changes made in PR #28

@fivetran-joemarkiewicz fivetran-joemarkiewicz marked this pull request as ready for review June 22, 2022 20:19
Copy link
Contributor

@fivetran-sheringuyen fivetran-sheringuyen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @fivetran-joemarkiewicz, great job on the deprecation and rest of the PR! I just had some notes I wanted us to keep track of and a couple in-line comments.

There may be some merge conflicts since the feature/google-ads-api-default updates, but I want to make sure certain things don't accidentally get lost when we do get around to releasing this package:

  • Documentation for final urls regex matching - specifically in the stg_google_ads__ad_history.source_final_urls tests.
  • Removing deprecation notes from README

It looks like in both google_ads and google_ads_source, run_results.json is missing. Is this intentional (and should be updated in the generate_docs macro)?

models/stg_google_ads__account_history.sql Outdated Show resolved Hide resolved
models/stg_google_ads__account_stats.sql Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Show resolved Hide resolved
models/src_google_ads.yml Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@fivetran-joemarkiewicz fivetran-joemarkiewicz changed the base branch from feature/google-ads-api-default to main June 28, 2022 17:54
@fivetran-joemarkiewicz
Copy link
Contributor Author

Thanks for the review @fivetran-sheringuyen! I addressed your inline comments and only had a question on the one I left unresolved regarding the passthrough logic. Let me know your thoughts! Additionally I addressed the following that was not captured inline:

  • Added the yml documentation for the test warning with the stg_google_ads__ad_hsitory final_urls field.
  • Included the run_results.json. I initially intentionally omitted it as it isn't necessarily needed. However, I noticed the datatype value is missing within the docs if you omit this file. Therefore I added it back in.
  • Addressed merge conflicts after switching the base branch to be main.
  • Regenerated the docs.

Copy link
Contributor

@fivetran-sheringuyen fivetran-sheringuyen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @fivetran-joemarkiewicz for making the updates, this package looks good to me for release when we roll out Ad Reporting V2!

@fivetran-joemarkiewicz
Copy link
Contributor Author

fivetran-joemarkiewicz commented Jul 20, 2022

Things to update before merging:

  • Add not null tests
  • Ensure passthrough metrics is correct in both CTEs and no duplicates in macro

@fivetran-sheringuyen fivetran-sheringuyen merged commit d091572 into main Sep 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants