-
Notifications
You must be signed in to change notification settings - Fork 22
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
Update Quickstart.yml with proper source tables #57
Conversation
- local: ../ |
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.
odd, not sure why this is logging this as a change
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-avinash thanks for this update! I have two callouts that I'd like to be applied before approval. Let me know if you have any questions. Thanks!
CHANGELOG.md
Outdated
@@ -1,3 +1,9 @@ | |||
# dbt_xero v0.7.1 |
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.
Because this may ultimately change the number of models generated due to the variables working as expected, I would prefer we make this a breaking change. I would rather users are aware of the bugfix and realize that this will increase their model counts if these tables are selected in their connection schema tab.
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.
Updated.
CHANGELOG.md
Outdated
# dbt_xero v0.7.1 | ||
[PR #54](https://github.com/fivetran/dbt_xero/pull/54) includes the following updates: | ||
|
||
## Under the Hood |
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.
I would actually phrase this as a bugfix since this will have an impact on the model count if the tables are selected in the schema tab and present in their destination. As such, let's define this as a bugfix and reword this to ensure the users are aware of the change this may have on their model count.
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.
Rephrased.
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-joemarkiewicz Requested changes look good to me, ready for re-review.
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
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!
PR Overview
This PR will address the following Issue/Feature: [#56]
This PR will result in the following new package version: v0.8.0
quickstart.yml
updates that will raise the model count by default, so breaking changePlease provide the finalized CHANGELOG entry which details the relevant changes included in this PR:
Fixed the
_credit_note
and_bank_transaction
table variable naming inquickstart.yml
to ensure their respective models are enabled and disabled appropriately.Submission Checklist
Submitter:
quickstart.yml
matches connector schema (checked multiple Xero connections to make sure)Testing Instructions:
[NA] Focus Areas: Highlight any complex logic or queries needing special attention
Reviewer:
Changelog
PR Checklist
Basic Validation
Please acknowledge that you have successfully performed the following commands locally:
Before marking this PR as "ready for review" the following have been applied:
If you had to summarize this PR in an emoji, which would it be?
🖌️