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

Bug/databricks get url parameter #130

Merged
merged 8 commits into from
Nov 29, 2023

Conversation

fivetran-catfritz
Copy link
Contributor

@fivetran-catfritz fivetran-catfritz commented Nov 27, 2023

What change does this PR introduce?

  • Added macro extract_url_parameter to create special logic for Databricks instances not supported by dbt_utils.get_url_parameter(). The macro uses dbt_utils.get_url_parameter() for default, non-Databricks targets. See README for more details.

If this PR introduces a new macro, how did you test the new 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?

  • Does not modify existing fivetran_utils macro, but is meant to replace dbt_utils.get_url_parameter().
  • The affected packages tend use this macro at different stages, so the list is a result of a search for dbt_utils.get_url_parameter:
    • dbt_facebook_ads
    • dbt_google_ads_source
    • dbt_google_ads
    • dbt_linkedin_source
    • dbt_microsoft_ads
    • dbt_pinterest_source
    • dbt_reddit_ads
    • dbt_snapchat_ads
    • dbt_tiktok_ads_source
    • dbt_twitter_source
  • Copy-cat issues have been created in these packages.

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

  • Yes
  • No (provide further explanation)

@fivetran-catfritz fivetran-catfritz self-assigned this Nov 27, 2023
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-catfritz great work on this PR! I just have a few small comments and suggestions I would like you to look at and address before approving. Let me know if you have any questions!

integration_tests/packages.yml Show resolved Hide resolved
macros/extract_url_parameter.sql Outdated Show resolved Hide resolved
macros/extract_url_parameter.sql Show resolved Hide resolved
@fivetran-catfritz
Copy link
Contributor Author

Thanks @fivetran-joemarkiewicz! I made the changes and added comments, so it's ready for re-review.

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.

Thanks @fivetran-catfritz these changes look good to me! Let's hold off merging this to the live branch until we also incorporate the sql server updates as well.

If you have the time, would you be able to create a release branch for release/v0.4.9. We can then merge this branch into there, then the sql server updates into that branch as well before then rolling out the update to the live branch.

@fivetran-catfritz fivetran-catfritz changed the base branch from releases/v0.4.latest to release/v0.4.9 November 29, 2023 16:39
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.

2 participants