-
Notifications
You must be signed in to change notification settings - Fork 10
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
update/twitter-organic-source-change #11
Conversation
twitter_organic_source: | ||
+materialized: table |
There was a problem hiding this comment.
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
# - 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 |
There was a problem hiding this comment.
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.
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" |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
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. |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this 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.
Co-authored-by: Avinash Kunnath <[email protected]>
@fivetran-avinash all requested changes have been applied. Thanks for catching the version mismatch! |
There was a problem hiding this 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!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm for release!
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:
PR Checklist
Basic Validation
Please acknowledge that you have successfully performed the following commands locally:
Before marking this PR as "ready for review" the following have been applied:
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.
If you had to summarize this PR in an emoji, which would it be?
🐦