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

update/twitter-organic-source-change #11

Merged
merged 5 commits into from
Aug 27, 2024

Conversation

fivetran-joemarkiewicz
Copy link
Collaborator

@fivetran-joemarkiewicz fivetran-joemarkiewicz commented Aug 20, 2024

PR Overview

This PR will address the following Issue/Feature: Upstream Twitter Organic Source Issue #10

This PR will result in the following new package version: v0.5.0

This will be breaking because the upstream Twitter Organic source name has changed and there have been some identifier variable changes. Please see the CHANGELOG entry below for more details.

Please provide the finalized CHANGELOG entry which details the relevant changes included in this PR:

Upstream Breaking Changes (Twitter Organic)

PR #12 from the upstream dbt_twitter_organic_source package includes the following breaking change updates:

  • The source defined in the src_twitter_organic.yml file has been renamed from twitter_organic to twitter to align with the default schema name used by the upstream Fivetran connector.
    • If you're referencing sources from the upstream Twitter Organic packages, please update your source references as needed. See below for the full scope of source changes.
New Source Reference Old Source Reference
"{{ source('twitter','account_history') }}" "{{ source('twitter_organic','account_history') }}"
"{{ source('twitter','organic_tweet_report') }}" "{{ source('twitter_organic','organic_tweet_report') }}"
"{{ source('twitter','tweet') }}" "{{ source('twitter_organic','tweet') }}"
"{{ source('twitter','twitter_user_history') }}" "{{ source('twitter_organic','twitter_user_history') }}"
  • The default schema name has been modified from twitter_organic to now be twitter to more closely align with the default schema name generated by the Fivetran connector. Please be aware if you were leveraging the previous default schema then you will need to update the twitter_organic_schema variable accordingly.
  • All identifier variables in the src_twitter_organic.yml file have been renamed. If you’re using any of these in your project, please update them accordingly. The changes include:
    • Prepending twitter_organic_* has been updated to twitter_* to align with the schema change.
    • The spelling of *_identifer has been corrected to *_identifier.
New Identifier Variable Name Old Identifier Variable Name
twitter_account_history_identifier twitter_organic_account_history_identifer
twitter_organic_tweet_report_identifier twitter_organic_organic_tweet_report_identifer
twitter_tweet_identifier twitter_organic_tweet_identifer
twitter_twitter_user_history_identifier twitter_organic_twitter_user_history_identifer

Under the Hood

  • Consistency validation for integration tests has been added for the social_media_reporting__rollup_report model.
  • Updated the maintainer PR, Issue, Feature Request, and Config templates to resemble the most up to date format.
  • Renamed the Twitter Organic seed files to allow for more testing functionality.
  • Addition of a section tag within the README so the model descriptions may be accessible within the Fivetran UI for Quickstart.

PR Checklist

Basic Validation

Please acknowledge that you have successfully performed the following commands locally:

  • dbt run –full-refresh && dbt test
  • dbt run (if incremental models are present) && dbt test

Before marking this PR as "ready for review" the following have been applied:

  • The appropriate issue has been linked, tagged, and properly assigned
  • All necessary documentation and version upgrades have been applied
  • docs were regenerated (unless this PR does not include any code or yml updates)
  • BuildKite integration tests are passing
  • Detailed validation steps have been provided below

Detailed Validation

Please share any and all of your validation steps:

Please see Twitter Organic Source PR #12 and Twitter Organic PR #11 for upstream validation notes.

Additionally, the validation test written to ensure comparison and aggregate metrics match between dev and prod are passing as expected.

image

If you had to summarize this PR in an emoji, which would it be?

🐦

Comment on lines -40 to -41
twitter_organic_source:
+materialized: table
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Same note from the transform PR. This was masking the window function issues on Redshift. I have not turned this off on the other packages because the fix has not yet been applied. Ideally these table materialization configs can all be removed in the future.

packages.yml Outdated
Comment on lines 6 to 10
# - package: fivetran/twitter_organic
# version: [">=0.3.0", "<0.4.0"]
- git: https://github.com/fivetran/dbt_twitter_organic.git
revision: update/default-schema-change
warn-unpinned: false
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

To be swapped before merge.

Comment on lines -8 to +12
twitter_organic_schema: social_media_rollup_integration_tests
twitter_organic_account_history_identifer: "twitter_organic_account_history_data"
twitter_organic_organic_tweet_report_identifer: "twitter_organic_organic_tweet_report_data"
twitter_organic_tweet_identifer: "twitter_organic_tweet_data"
twitter_organic_twitter_user_history_identifer: "twitter_organic_twitter_user_history_data"
twitter_organic_schema: social_media_rollup_integration_tests_1
twitter_account_history_identifier: "account_history"
twitter_organic_tweet_report_identifier: "organic_tweet_report"
twitter_tweet_identifier: "tweet"
twitter_twitter_user_history_identifier: "twitter_user_history"
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

These changes were made to ensure the union_data macro was working as expected. We cannot effectively test the union_data macro when the seed files are named different from the expected source names.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The other seeds weren't changed to maintain consistency with their upstream counterparts.

@fivetran-joemarkiewicz fivetran-joemarkiewicz marked this pull request as ready for review August 20, 2024 15:21
CHANGELOG.md Outdated

- Consistency validation for integration tests has been added for the `social_media_reporting__rollup_report` model.
- Updated the maintainer PR, Issue, Feature Request, and Config templates to resemble the most up to date format.
- Renamed the Twitter Organic seed files to allow for more testing functionality.
- Addition of a section tag within the README so the model descriptions may be accessible within the Fivetran UI for Quickstart.
Copy link
Contributor

Choose a reason for hiding this comment

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

"Addition of a section tag within the README" I'm not seeing this change in this PR. Was it part of a past PR?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It looks like it was part of PR #10. I can add the specific link here for this line.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@fivetran-avinash, I just reworked the CHANGELOG to have this section be explicit for the PR reference.

Copy link
Contributor

@fivetran-avinash fivetran-avinash left a comment

Choose a reason for hiding this comment

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

@fivetran-joemarkiewicz Tests validated for me!

A few comments before release.

README.md Outdated Show resolved Hide resolved
dbt_project.yml Outdated Show resolved Hide resolved
integration_tests/dbt_project.yml Outdated Show resolved Hide resolved
@fivetran-joemarkiewicz
Copy link
Collaborator Author

@fivetran-avinash all requested changes have been applied. Thanks for catching the version mismatch!

Copy link
Contributor

@fivetran-avinash fivetran-avinash left a comment

Choose a reason for hiding this comment

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

@fivetran-joemarkiewicz Thanks for applying, lgtm!

Copy link
Contributor

@fivetran-catfritz fivetran-catfritz left a comment

Choose a reason for hiding this comment

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

lgtm for release!

@fivetran-joemarkiewicz fivetran-joemarkiewicz merged commit 1bd3262 into main Aug 27, 2024
7 checks passed
@fivetran-joemarkiewicz fivetran-joemarkiewicz deleted the update/twitter-organic-source-change branch August 27, 2024 17:55
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.

3 participants