fivetran / dbt_fivetran_log

Data models for Fivetran's internal log connector built using dbt.
https://fivetran.github.io/dbt_fivetran_log/
Apache License 2.0
30 stars 24 forks source link

Adds options to which models are included #52

Closed cmcau closed 2 years ago

cmcau commented 2 years ago

Pull Request Are you a current Fivetran customer? Partner Chris McClellan Director Visualisedata Pty Ltd AUSTRALIA

What change(s) does this PR introduce? Adds options to dbt_project.yml to optionally load some of the models. I wanted to use this package to show what Fivetran was doing (during the trial period), but some of the models are not available and that causes an error. With these changes I don't get any errors and I can report on the data because the run finishes without any errors

Did you update the CHANGELOG?

Does this PR introduce a breaking change?

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)

Is this PR in response to a previously created Bug or Feature Request

How did you test the PR changes?

I tested it with all the vars properly defined in the root dbt_project.yml and it passed all tests. I DID NOT test when the vars are not defined in the root dbt_project.yml

Select which warehouse(s) were used to test the PR

Provide an emoji that best describes your current mood

:grinning: hopefully, but a lot of :worried: at the same time **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](https://www.surveymonkey.com/r/DQ7K7WW) on our existing dbt packages or what you'd like to see next.
fivetran-joemarkiewicz commented 2 years ago

Hey @cmcau thanks so much for opening this PR!!

These changes make complete sense and I understand why you would want to remove these models from the run as they do not exist during the trial phase. My one question I have is regarding the value of the end models with these disabled. Are you still able to gain actionable insights in your opinion with these disabled?

cmcau commented 2 years ago

Well, I don't know what I'm missing right ? ;) I have created a visualisation that shows the data in the log_audit table (so I can see Deletes, Updated and Replaced or Inserted by Connector and Table Name.

Active Volume seems to be the most interesting one that I'm missing (but I'm only going on the name of the model)

fivetran-joemarkiewicz commented 2 years ago

@cmcau thanks again for opening this PR!

I am going to be pushing a few changes to the PR you have in place to fold into the main branch more seamlessly. One major change I made which I wanted to call out is that I removed the variables on the credits_used and the active_volume models. These models I feel are integral to the package and should not be removed with variables. If a user decides to want to remove these models then they can leverage the models config to disable them.

Further, I believe these models should be synced even when you are on the free trial. Please let me know if this was not your experience.

fivetran-joemarkiewicz commented 2 years ago

@cmcau in order to address the merge conflicts I had to create a new branch from yours. Please follow PR #55 for next steps on your commits being merged.