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

Feature #50: Enable teams to configure additional columns for staging entities models #51

Conversation

FrankTub
Copy link

@FrankTub FrankTub commented Jul 9, 2024

Please provide your name and company

Frank Tubbing - Beequip

Link the issue/feature request which this PR is meant to address

#50

Detail what changes this PR introduces and how this addresses the issue/feature request linked above.

Enables me to define custom entity fields without having to create my own base/staging model.

How did you validate the changes introduced within this PR?

Ran my forked repo branch in my own dbt project

Which warehouse did you use to develop these changes?

Postgres

Did you update the CHANGELOG?

  • Yes

Did you update the dbt_project.yml files with the version upgrade (please leverage standard semantic versioning)? (In both your main project and integration_tests)

  • Yes

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.

PR Template

@FrankTub
Copy link
Author

FrankTub commented Jul 9, 2024

Not really sure if I should bump the version here ?

@fivetran-avinash
Copy link
Contributor

fivetran-avinash commented Jul 12, 2024

Not really sure if I should bump the version here ?

Hi @FrankTub ! Yes, you'll want to bump the version up. Since I don't think any customers would be impacted, I'd recommend a minor update up to version 0.10.1. (I can also commit those changes directly as well, see the below comment)

@fivetran-avinash fivetran-avinash self-requested a review July 12, 2024 20:13
@fivetran-avinash
Copy link
Contributor

fivetran-avinash commented Jul 13, 2024

Thanks for opening this PR @FrankTub ! I think this mostly looks good--I've tested these changes and I feel mostly confident this is the correct configuration. There is one significant change I noticed, but otherwise we are close to releasing this change.

I was wondering if you had any issue with me making some commits directly to your branch? There are a few changes within our integration_tests folder and other general maintenance changes we include for every release of dbt_netsuite.

If it works for you, as I review your changes I can make these background changes to your branch. This way we may fold them into the main branch more smoothly for a release early next week.

If not, I can provide you with the required changes as well in my PR review.

Once you give me the approval either way, we can kickstart the process of this release! Let me know your questions and concerns.

@fivetran-avinash
Copy link
Contributor

@FrankTub Also one additional question: Is group one of the columns you are attempting to passthrough? There are some intricacies we will need to address on our end that we will add to the release if that's the case, as group is a reserved keyword and our current passthrough column logic isn't quite equipped to handle this. But we have workarounds in that case.

@FrankTub
Copy link
Author

@fivetran-avinash , the field group is not per se one of the fields we will need to pass through.

You are free to add commits to my branch without breaking any functionality at our side. We want to start using this, but don't use it yet in our production environment. I've bumped the version in my branch to 0.10.1.

Copy link
Contributor

@fivetran-avinash fivetran-avinash left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @FrankTub for letting us know about this feature and then opening this PR proactively! We really appreciate your collaboration and patience as we get this to a ready state.

I made a few of the changes I discussed to ensure this passthrough column feature works properly on the entity staging macro and model.

With that done, I've approved this PR and have begun kicking off the internal release process. Once this is release reviewed, I will merge this into main, and you should see this new release live on our hub hopefully sometime this week!

We will keep you in the loop.

macros/get_entity_columns.sql Outdated Show resolved Hide resolved
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.

@FrankTub @fivetran-avinash thanks for working through this feature update! This PR looks great, I just have two small suggestions to be applied before approval.

CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
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.

LGTM

@fivetran-avinash fivetran-avinash merged commit 9452156 into fivetran:main Jul 17, 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.

[Feature] Enable teams to configure additional columns for all source models
3 participants