-
Notifications
You must be signed in to change notification settings - Fork 2
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/remove-deprecated-fields #16
Conversation
@@ -761,8 +761,6 @@ sources: | |||
Defaults to the plan's total_billing_cycles. | |||
- name: shipping_address_id | |||
description: Unique id assigned to shipping address. | |||
- name: started_with_gift |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
converted_at
was not present in main
, so no need to remove from docs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can tell this is my first major project ugh 🤦🏽♂️ .
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
CHANGELOG.md
Outdated
- These fields were removed since they are planned for future deprecation in the Fivetran Connector and are not utilized in the downstream transformation package. | ||
|
||
|
||
[PR #14](https://github.com/fivetran/dbt_recurly_source/pull/14) includes the following updates: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this referring to the Under the Hood changes from a previous PR? Maybe we should add a link to this PR at the end of those sentences rather than have two sentences of this format in the same release notes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah it was a pr from a while ago. Updated!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fivetran-catfritz Looks mostly good! A few comments to address before approval.
Also, run_results.json
needs to be removed from the docs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @fivetran-avinash! I updated and good call on the run_results. I removed that too!
CHANGELOG.md
Outdated
- These fields were removed since they are planned for future deprecation in the Fivetran Connector and are not utilized in the downstream transformation package. | ||
|
||
|
||
[PR #14](https://github.com/fivetran/dbt_recurly_source/pull/14) includes the following updates: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah it was a pr from a while ago. Updated!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved!
PR Overview
This PR will address the following Issue/Feature:
converted_at
andhas_started_with_gift
fields from subscription history table #15This PR will result in the following new package version:
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 (if incremental models are present) && dbt testBefore marking this PR as "ready for review" the following have been applied:
Detailed Validation
Please share any and all of your validation steps:
If you had to summarize this PR in an emoji, which would it be?
💃