-
Notifications
You must be signed in to change notification settings - Fork 20
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
Magic bot 36c3c8115b #13
Magic bot 36c3c8115b #13
Conversation
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.
Hi @phillem15 thanks so much for opening this PR! The code changes you applied make sense and I appreciate you leveraging a past PR to guide you in this suggestion 😄 I have one quick question about your changes and two small requests for updates.
-
One quick question - do you intend for any of these passthrough fields to be included in any end models within the dbt_netsuite package? Or are you looking to just leverage these fields in the staging models?
-
Can you update the README section on describing the passthrough columns to include the two additional variables you are adding in this PR
-
Can you make a small update in the CHANGELOG to detail your update. Be sure to add yourself in the next release contributors section!
Update passthrough columns section.
Add subsidiaries and consolidated exchange rates pass through columns to the changelog.
Hi @fivetran-joemarkiewicz thanks for reaching out! I've made the changes you've requested (hoping I've done them correctly). As for your first question, I am only looking to leverage these fields in the staging models of my own project. |
Thanks so much for the clarification @phillem15! I will plan to give your changes a final review and possibly merge later this week 😄 I will let you know if I have any other questions! |
Hi @fivetran-joemarkiewicz I realized the tests I conducted were not looking at my official changes. I have retested and made some final adjustments to the models. I have re-run the tests and everything is now looking good on my end. Thanks! |
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.
@phillem15 thanks so much for this contribution 😄
I made one small edit to your branch to increase the version of the package to the next appropriate index. Aside from that, I tested on our own Netsuite data and saw the passthrough columns work as intended and also had no downstream impacts. Looks great to me!
We are extremely appreciative of your contribution to one of our packages and I happy you are able to leverage it in your own environment! I will merge these changes and cut a release shortly!
Are you a current Fivetran customer?
Evan Phillips, Lead BI Architect, Nordic Global
What change(s) does this PR introduce?
Adding pass through columns for Subsidiaries and Consolidated Exchange Rates staging models.
Does this PR introduce a breaking change?
Is this PR in response to a previously created Issue
How did you test the PR changes?
Followed the suggested approach in this post, forked and cloned the repository to my local machine, changed the code and made sure my project ran with the updated code!
Select which warehouse(s) were used to test the PR
Provide an emoji that best describes your current mood
😎
Feedback
We are so excited you decided to contribute to the Fivetran community dbt package! We continue to work to improve the packages and would greatly appreciate your feedback on our existing dbt packages or what you'd like to see next.