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/amazon ads #75

Merged
merged 14 commits into from
Jan 23, 2023
Merged

Feature/amazon ads #75

merged 14 commits into from
Jan 23, 2023

Conversation

fivetran-catfritz
Copy link
Contributor

@fivetran-catfritz fivetran-catfritz commented Jan 10, 2023

Are you a current Fivetran customer?

Fivetran created PR

What change(s) does this PR introduce?

Amazon Ads has officially been released and added to Ad Reporting.

Amazon Ad data can now be rolled into the below models:

  • ad_reporting__account_report
  • ad_reporting__campaign_report
  • ad_reporting__ad_group_report
  • ad_reporting__ad_report
  • ad_reporting__search_report
  • ad_reporting__keyword_report

Documentation has been updated to include Amazon Ads information.

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.)
    May need to disable amazon ads models if not using.

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?

  • Buildkite
  • 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 added type:enhancement New functionality or enhancement status:in_review Currently in review update_type:models Primary focus requires model updates labels Jan 12, 2023
Copy link
Contributor

@fivetran-joemarkiewicz fivetran-joemarkiewicz left a comment

Choose a reason for hiding this comment

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

@fivetran-catfritz this is looking great and all tests are passing on my end! I have just a few small change requests and comments below. Once those updates are applied and address, let me know and I can re-review!

Additionally, this is not from your PR, but I noticed the .DS_STORE file has been erroneously included in the root directory. This file is generated when creating the docs on a macOS computer. This is not needed and should be removed. Do you mind deleting this file in your PR and adding it to the .gitignore file. Thanks!

Let me know if you have any questions 😄

.buildkite/scripts/run_models.sh Outdated Show resolved Hide resolved
CHANGELOG.md Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
macros/get_enabled_packages.sql Outdated Show resolved Hide resolved
models/intermediate/int_ad_reporting__keyword_report.sql Outdated Show resolved Hide resolved
models/intermediate/int_ad_reporting__search_report.sql Outdated Show resolved Hide resolved
models/intermediate/int_ad_reporting__url_report.sql Outdated Show resolved Hide resolved
packages.yml Outdated
Comment on lines 29 to 33
# - package: fivetran/amazon_ads
# version: [">=0.1.0", "<0.2.0"]
- git: https://github.com/fivetran/dbt_amazon_ads.git
revision: main
warn-unpinned: false
Copy link
Contributor

Choose a reason for hiding this comment

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

Friendly reminder to switch before release 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

README.md Outdated Show resolved Hide resolved
Copy link
Contributor

@fivetran-joemarkiewicz fivetran-joemarkiewicz left a comment

Choose a reason for hiding this comment

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

@fivetran-catfritz thanks so much for applying the changes I requested. The PR looks great and I only have one final suggestion to the README. Once that is applied we can go ahead and regen the docs (using our postgres integration tests).

Let's hold off on merging this until the Amazon Ads packages are officially released. Great job!!

README.md Outdated Show resolved Hide resolved
@@ -13,7 +13,8 @@ dispatch:
vars:

apple_search_ads__using_search_terms: True
twitter_ads__using_keywords: False
twitter_ads__using_keywords: True
Copy link
Contributor

Choose a reason for hiding this comment

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

@fivetran-catfritz should this be switched?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@fivetran-joemarkiewicz I did it so the docs would generate with the twitter ads keyword reports, but if they shouldn't be I'll put it back.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have changed this back!

@fivetran-catfritz fivetran-catfritz removed the update_type:models Primary focus requires model updates label Jan 23, 2023
@fivetran-catfritz fivetran-catfritz merged commit 98d45ac into main Jan 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status:in_review Currently in review type:enhancement New functionality or enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants