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

Adds options to which models are included #52

Closed
wants to merge 11 commits into from
Closed

Conversation

cmcau
Copy link
Contributor

@cmcau cmcau commented Apr 25, 2022

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?

  • Yes

Does this PR introduce a breaking change?

  • Yes (please provide breaking change details below.)
  • No (please provide an explanation as to how the change is non-breaking below.)
    I'm going to say YES to be safe. I only thought of that now - if users don't have the vars defined then there will be an error I guess. I haven't tested this, but I'm happy to do the work if this is an issue that needs to be fixed and this PR is going to benefit the package.

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

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

  • Yes, Issue/Feature [link bug/feature number here]
  • No

How did you test the PR changes?

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

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

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

Provide an emoji that best describes your current mood

😀 hopefully, but a lot of 😟 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 on our existing dbt packages or what you'd like to see next.

cmcau added 11 commits April 23, 2022 16:28
bumped the version number to 0.5.5
added flag to disable this model build if required
Added option to disable this model
Added option to disable this model
Added option to disable this model
Added option to disable this model
Added option to disable model
Updated description for 0.5.5
@fivetran-joemarkiewicz
Copy link
Contributor

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
Copy link
Contributor Author

cmcau commented Apr 26, 2022

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 fivetran-joemarkiewicz changed the base branch from main to feature/cmcau-model-flexibility June 2, 2022 14:05
@fivetran-joemarkiewicz fivetran-joemarkiewicz changed the base branch from feature/cmcau-model-flexibility to main June 2, 2022 14:46
@fivetran-joemarkiewicz
Copy link
Contributor

@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 fivetran-joemarkiewicz mentioned this pull request Jun 2, 2022
14 tasks
@fivetran-joemarkiewicz
Copy link
Contributor

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

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