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

exploring integration test schema cleanup #106

Merged

Conversation

fivetran-jamie
Copy link
Contributor

@fivetran-jamie fivetran-jamie commented Mar 23, 2023

What change does this PR introduce?

  • adds drop_schemas macro to be run in integration testing
  • adds quote macro that i feel like we would use all the time

If this PR introduces a new macro, how did you test the new macro?

on shopify source

If this PR introduces a modification to an existing macro, which packages is the macro currently present in and what steps were taken to test compatibility across packages?

Did you update the README to reflect the macro addition/modifications?

  • Yes
  • No (provide further explanation) --> still need to

Copy link

@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.

Awesome PR! Love the cross team collab here, hoping to see more of this in the future!

Additionally, something that may be interesting to consider is setting expiration dates for tables like we currently do for analytics here.

README.md Outdated Show resolved Hide resolved
@@ -0,0 +1,31 @@
{% macro drop_schemas(drop_target_schema=true) %}

Choose a reason for hiding this comment

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

i wonder if you can make this more flexible by adding a fuzzy_schema_match condition?

Specifically for this use case. If a fuzz_schema_match = 'LIKE '%integration_test%' could be a param, this can allow us to leverage the fivetran utils package for this instead of having a custom macro in the integrations package.

Choose a reason for hiding this comment

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

I guess it doesn't have to be fuzzy lol it could just be something like schema_where_like . but probably depends on the use cases.

Copy link
Contributor Author

@fivetran-jamie fivetran-jamie Apr 17, 2023

Choose a reason for hiding this comment

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

oooh okay so twoish ideas:

  • maybe a schema_patterns argument that by default is [target.schema] so you can drop multiple schema patterns. so in this case you would provide ['integration_tests', 'integrations_tests']. i realize this isn't what you were focusing on above but realized that's also a thing 😅 or you'd have to run the macro twice with different targets
  • i opted for only adding the % wildcard as a suffix since that's in line with how dbt does custom schemas (ie appends _schema_suffx to the target). but yeah maybe that's too limiting.. i wonder if we add the schema_patterns variable, should the user have to add the %'s to the patterns? so like in your example, you could get it extra fuzzy and provide schema_patterns=['%integration_test%', '%integrations_test%']
    • however, the default behavior could still align with dbt and be schema_pattern=[target.schema ~ '%']

we could also have the user provide a whole where clause like you mentioned, which would offer the most flexibility but fewer guardrails

cc @fivetran-joemarkiewicz

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah I think for the current use case for this macro the way @fivetran-jamie opted to build it out works great! I could see a future where we provide more flexibility.

However, for the current implementation I would prefer we keep it as is so we don't run into any fuzzy matching issues within our own warehouses. Especially as this will be running constantly as we work on updates across our packages. If we see interest in people wanting to leverage this outside of the current use case, we should definitely consider expanding the flexibility of the macro.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

cool i'll make a FR to include fuzzy matching after we merge this in!

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-jamie I have a few comments and suggestions. Additionally, would you be able to increase the package index and add a log to the CHANGELOG. Let me know if you have any questions.

macros/drop_schemas.sql Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
macros/drop_schemas.sql 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-jamie thanks for making the requested updates! When doing my final review I had some additional questions and suggestions around the previous changes and the new ones you applied. That being said, the code looks to function without issue! I was able do some detailed testing on my end and feel confident that following this round of updates we should be good to approve and release!

Let me know if you want to sync to discuss in more detail.

README.md Outdated Show resolved Hide resolved
macros/drop_schemas_automation.sql Outdated Show resolved Hide resolved
macros/quote_something.sql Outdated Show resolved Hide resolved
macros/quote_something.sql Outdated Show resolved Hide resolved
macros/drop_schemas_automation.sql Show resolved Hide resolved
fivetran-jamie and others added 5 commits April 18, 2023 11:59
Co-authored-by: Joe Markiewicz <74217849+fivetran-joemarkiewicz@users.noreply.github.com>
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-jamie thanks for making these updates! All the changes look good to me!

@fivetran-jamie fivetran-jamie merged commit cc710ca into releases/v0.4.latest Apr 18, 2023
@fivetran-jamie fivetran-jamie deleted the explore/drop-integration-test-schemas branch December 15, 2023 22:05
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