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/default-schema-change #12

Merged
merged 8 commits into from
Aug 27, 2024

Conversation

fivetran-joemarkiewicz
Copy link
Collaborator

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

PR Overview

This PR will address the following Issue/Feature: Issue #10

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

This will be a breaking change since the default schema name is changing from twitter_organic to twitter. Likewise, the underlying source name is making the same change. This could break customers workflows if they were previously relying on the default schema being twitter_organic. Now those users will need to update the twitter_organic_schema variable accordingly.

Please detail what change(s) this PR introduces and any additional information that should be known during the review of this PR:

Breaking Change

PR #12 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 this package, 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:

  • All tmp models have been updated to reference the new source name twitter (previously twitter_organic). (#12)
  • Included auto-releaser GitHub Actions workflow to automate future releases. (#12)
  • Updated the maintainer PR, Issue, Feature Request, and Config templates to resemble the most up to date format. (#12)
  • Incorporated the new fivetran_utils.drop_schemas_automation macro into the end of each Buildkite integration test job. (#9)
  • Updated the pull request templates. (#9)

PR Checklist

Basic Validation

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

  • dbt run –full-refresh && dbt test
  • [n/a] 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:

I was able to validate these changes are working as expected by simply running the models. As there were no code changes applied to these models, I just needed to confirm that the models would compile and run successfully with the schema name change.

image

Additionally, we can see that the default schema name is now trying to reference twitter as opposed to twitter_organic when I do not define the schema name variable. Note the TWITTER schema being the callout.
image

If we look at the live version, we can see this references TWITTER_ORGANIC.

image

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

🐦

Comment on lines 7 to 10
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_account_history_identifer: "account_history"
twitter_organic_tweet_report_identifer: "organic_tweet_report"
twitter_tweet_identifer: "tweet"
twitter_twitter_user_history_identifer: "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.

@fivetran-joemarkiewicz fivetran-joemarkiewicz marked this pull request as ready for review August 20, 2024 15:21
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 Was able to successfully run and compile! A few notes before approval.

README.md Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

packages yml version range needs to be bumped up in Step 2

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updated


## Under the Hood:
- All tmp models have been updated to reference the new source name `twitter` (previously `twitter_organic`). ([#12](https://github.com/fivetran/dbt_twitter_organic_source/pull/12))
- Renamed the seed files to allow for more testing functionality. ([#12](https://github.com/fivetran/dbt_twitter_organic_source/pull/12))
- Included auto-releaser GitHub Actions workflow to automate future releases. ([#12](https://github.com/fivetran/dbt_twitter_organic_source/pull/12))
Copy link
Contributor

Choose a reason for hiding this comment

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

It doesn't look as if the secrets have been added, so the automation bot won't work upon release. Can you add the secret in?

Screenshot 2024-08-21 at 11 44 00 AM

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yup this will be added before merge for all repos. I just added here and will add to the remaining ones before merge.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added to all relevant repos

- 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`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Not quite 😅. Can you fix the spelling?
Screenshot 2024-08-21 at 11 46 05 AM

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ahh yeah this was done to test what happens when variables are misspelled again but never reverted in integration tests. Will update

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Pushed your suggestion

integration_tests/dbt_project.yml Outdated Show resolved Hide resolved
Co-authored-by: Avinash Kunnath <108772760+fivetran-avinash@users.noreply.github.com>
@fivetran-joemarkiewicz
Copy link
Collaborator Author

Thanks @fivetran-avinash! Changes addressed and ready for re-review.

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.

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 17c4607 into main Aug 27, 2024
7 checks passed
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