-
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
add pass through for customers #10
add pass through for customers #10
Conversation
Thanks so much for opening this PR @Yaruis19! ❤️ 🎉 At first glance your changes make sense and look good. I will take a deeper dive and let you know if I have any further questions before approving and merging to be included in the next release. |
Thank you! @fivetran-joemarkiewicz Do you have an idea about when we will do the next release? I have some pending model that replies to this being added. If it's going to be more than several weeks from now, I will do some workaround from my side in the meantime. |
@Yaruis19 I actually have some time available today and can do a more thorough review. Realistically, I will be able to cut a new release of the dbt_netsuite_source package by Monday. With that, I do have one quick question. Are you intending that these customer passthrough columns be used in any downstream models within the dbt_netsuite package? Or would you use these passthrough columns for your own modeling efforts? |
@fivetran-joemarkiewicz This customer passthrough are added for my own modeling efforts. I haven't gotten a chance to use the standard model since our NetSuite is configured a little differently. If you want, I'm happy to add pass through to rest of the models if needed, since I already have this PR open :) |
@Yaruis19 sounds good! Since there haven't been any other requests for passthroughs with the other models I think it would be fine to just stick with customers for the time being. Then we can create other PRs if necessary as I would like to keep the number of variables to a minimum if possible. I'll review today though and let you know. Thanks! |
Thank you! @fivetran-joemarkiewicz |
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.
@Yaruis19 this PR looks great! Thanks for answering my other questions.
There are a few minor updates I want to make on my end (upgrade the version number, etc.) before release. Therefore, I have changed the base branch to be feature/Yaruis19-passthrough-add
and will make some changes there before merging to master
and releasing. I will create a new PR soon and tag you in it for visibility.
a8280b4
into
fivetran:feature/Yaruis19-passthrough-add
Are you a current Fivetran customer?
Yes
What change(s) does this PR introduce?
Add pass-through columns variables for NetSuite customers
Does this PR introduce a breaking change?
it add new columns to the customer model
Is this PR in response to a previously created Issue
How did you test the PR changes?
I tested by adding the local repo as my dbt package and ran
dbt run -m netsuite_source
to make sure the new columns I added locally did pass throughSelect which warehouse(s) were used to test the PR
Provide an emoji that best describes your current mood
💃
Feedback
I would love this package can add variables for all models
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.