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

bugfix/union-data-coverage #114

Conversation

fivetran-joemarkiewicz
Copy link
Contributor

What change does this PR introduce?

This PR adjusts the union_data macro to ensure a source connection may be established regardless of unioning schemas or not. This is a mirror PR to PR #110 with the addition of handling the error which resulted from v0.4.5 where the default_schema name did not always map to the name of the source.yml in the package. This caused compilation errors for customers and MUST be avoided and addressed.

Unfortunately, the only way to address this is to hard code the exceptions and performing a proper mapping exercise. As this behavior is only present in two packages, it is not too much of a risk, but this can be expanded in the future to include more as we roll out this feature.

A few other approaches I took include the following:

  • Adding a new argument to the macro that allows the package to provide the actual source name (regardless if it matches the default schema name). However, this did not prove to be viable as this would only be available in the latest versions of the impacted packages. Therefore, customers who have pinned versions of the packages would experience failures which is not ideal.
  • Using a different argument from the original macro to obtain the source name. This was inconclusive and default_schema name was consistently the best option
    • This did make me think that we should always make the source name the default schema name in our packages. At least for consistency.
  • Performing a trip or some manipulation procedure on the default_schema name. This proved to be more "hacky" than the exception mapping approach I took.

Very much open to other ideas if any have them.

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

This is not a new macro but rather an expansion of the existing union_data macro.

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?

This update mainly impacts packages using the union_data macro. As such, this version of fivetran_utils was tested on all of the following packages:

  • dbt_shopify_source
  • dbt_klaviyo_source
  • dbt_stripe_source
  • dbt_xero_source
  • dbt_quickbooks_source
  • dbt_linkedin_pages_source
  • dbt_instagram_business_source
  • dbt_twitter_organic_source
  • dbt_facebook_pages_source

Additional packages this was tested on include:

  • dbt_fivetran_log
  • dbt_zendesk
  • dbt_shopify_holistic_reporting

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

  • Yes
  • No (provide further explanation)
    The README updates were already applied in the previous PR linked above.

@fivetran-joemarkiewicz
Copy link
Contributor Author

Closing this as PR #118 covered this as well

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.

1 participant