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

Feature/order to orders #12

Merged
merged 16 commits into from
Jun 20, 2024
Merged

Feature/order to orders #12

merged 16 commits into from
Jun 20, 2024

Conversation

fivetran-catfritz
Copy link
Contributor

@fivetran-catfritz fivetran-catfritz commented Jun 17, 2024

PR Overview

This PR will address the following Issue/Feature:

This PR will result in the following new package version:

  • I chose v0.2.0 since we're potentially changing schema behavior, but on 2nd thought, it would only apply to new connectors. Open to opinions here.

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:

  • dbt run –full-refresh && dbt test
  • 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)
    • will regen after approval
  • BuildKite integration tests are passing
  • Detailed validation steps have been provided below

Detailed Validation

Please share any and all of your validation steps:

Test 1

  • Seeded both tables ORDERS and ORDER

    • Screenshot 2024-06-18 at 11 27 22 AM
  • Ran updates with no variable set and checked that it correctly compiled to select ORDERS in this case.

    • Screenshot 2024-06-18 at 11 26 52 AM

Test 2

  • Seeded both tables ORDERS and ORDER

  • Set var recharge__using_orders: false and checked that it correctly compiled to select ORDER in this case.

    • Screenshot 2024-06-18 at 11 37 53 AM

Test 3

  • Seeded only table ORDER

    • Screenshot 2024-06-18 at 11 28 16 AM
  • Ran updates with no variable set and checked that it correctly compiled to select ORDER in this case.

    • Screenshot 2024-06-18 at 11 27 58 AM

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

🍔

@fivetran-catfritz fivetran-catfritz self-assigned this Jun 17, 2024
@fivetran-catfritz fivetran-catfritz linked an issue Jun 18, 2024 that may be closed by this pull request
4 tasks
Copy link
Collaborator

@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 this looks great! I just have a few small suggestions. Mainly I want to get your thoughts around if we should still test in our integration_test when the connector uses the order naming convention.

Other than that, this looks great! Let me know once you take a look and I can re-review.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
models/tmp/stg_recharge__order_tmp.sql Outdated Show resolved Hide resolved
integration_tests/dbt_project.yml Show resolved Hide resolved
Copy link
Contributor Author

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

Thanks @fivetran-joemarkiewicz for the review! I have updated based on your comments. I tried a few different ways for getting the source 'order' to cooperate with the integrations test, but I had to go back to the way I had originally set it up. The issue is that the source has quoting set to true instead of there being issues with the column names like we discussed earlier.

Let me know if this looks good to go now!

integration_tests/dbt_project.yml Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
models/tmp/stg_recharge__order_tmp.sql Outdated Show resolved Hide resolved
Copy link
Collaborator

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

LGTM after docs are regenerated!

Copy link
Contributor

@fivetran-jamie fivetran-jamie left a comment

Choose a reason for hiding this comment

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

just one comment on the src.yml! otherwise approved

@@ -425,6 +425,51 @@ sources:
- name: _fivetran_synced
description: "{{ doc('_fivetran_synced') }}"

- name: orders # applies to connectors created on or after 2024-06-18
Copy link
Contributor

Choose a reason for hiding this comment

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

should we disable this source based on recharge__using_orders?

Copy link
Contributor

Choose a reason for hiding this comment

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

potentially also using the recharge_does_table_exist macro if possible

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@fivetran-jamie I hand't considered that, so I'm flexible either way! Could you explain the benefits?

Copy link
Contributor

Choose a reason for hiding this comment

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

Great question! Not much since we don't have any freshness tests in this package haha

Only benefit I can think of is getting rid of a floating orders source entity in the DAG for people who don't have it yet. Not super high priority, especially given that there's not an easy way to get rid of a floating order source in the DAG for people with orders.

Honestly let's leave it as is. I feel like it's more beneficial to potentially call people's attention to the upcoming ORDERS transition

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.

[Feature] Accommodate change from ORDER to ORDERS
3 participants