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

add pass through for customers #10

Conversation

Yaruis19
Copy link
Contributor

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?

  • Yes (please provide breaking change details below.)
  • No (please provide explanation how the change is non breaking below.)
    it add new columns to the customer model
    Is this PR in response to a previously created Issue
  • Yes, Issue [link issue number here]
  • No

How did you test the PR changes?

  • CircleCi
  • Other (please provide additional testing details below)

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 through
Select which warehouse(s) were used to test the PR

  • BigQuery
  • Redshift
  • Snowflake
  • Postgres
  • Databricks
  • Other (provide details below)

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.

@fivetran-joemarkiewicz
Copy link
Contributor

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.

@Yaruis19
Copy link
Contributor Author

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.

@fivetran-joemarkiewicz
Copy link
Contributor

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

@Yaruis19
Copy link
Contributor Author

dbt_netsuite

@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 :)

@fivetran-joemarkiewicz
Copy link
Contributor

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

@Yaruis19
Copy link
Contributor Author

Thank you! @fivetran-joemarkiewicz

@fivetran-joemarkiewicz fivetran-joemarkiewicz changed the base branch from master to feature/Yaruis19-passthrough-add October 28, 2021 19:55
Copy link
Contributor

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

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

@fivetran-joemarkiewicz fivetran-joemarkiewicz merged commit a8280b4 into fivetran:feature/Yaruis19-passthrough-add Oct 28, 2021
jmongerlyra added a commit to jmongerlyra/dbt_netsuite_source that referenced this pull request Feb 7, 2024
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.

2 participants