switch disable default config to enable default #43
Conversation
There was a problem hiding this comment.
@fivetran-reneeli great work! I have a few change requests included. Additionally, can you provide a screenshot of the validations passing after this update to make sure there are no unexpected changes we haven't accounted for in this update. Lastly, can you add consistency validation tests for the program and campaign models now that they're enabled by default.
| # - package: fivetran/marketo_source | ||
| # version: [">=0.13.0", "<0.14.0"] | ||
|
|
||
| - git: https://github.com/fivetran/dbt_marketo_source.git | ||
| revision: default_enable | ||
| warn-unpinned: false |
There was a problem hiding this comment.
Reminder to swap before merge
CHANGELOG.md
Outdated
| ```yml | ||
| vars: | ||
| marketo__enable_campaigns: False # Disable if Fivetran is not syncing the campaign table. This will disable the marketo__campaigns, marketo__programs, marketo__email_stats__by_campaign, marketo__email_stats__by_program models. | ||
| marketo__enable_programs: False # Disable if Fivetran is not syncing the program table. This will disable the marketo__programs and marketo__email_stats__by_program models. | ||
| ``` |
There was a problem hiding this comment.
I like how we're calling out here what will be disabled if these are turned off. However, would you be able to replace this section with a bit more clear direction as to what is being turned on by default, since that's a more relevant change from this update. It would be great as well to have those be included in maybe there own bullets and not hidden as comments within a code block.
There was a problem hiding this comment.
update-- let me know what you think
|
Thanks @fivetran-joemarkiewicz , re: validation tests; see internal ticket thread! |
There was a problem hiding this comment.
@fivetran-reneeli thanks for these updates! A few quick validation test requested updates before approving.
| ## Breaking Change | ||
| - Updates the default configuration for the `marketo__enable_campaigns` and `marketo__enable_programs` variables from disabled to enabled. | ||
|
|
||
| - Each variable will enable the following models by default: |
There was a problem hiding this comment.
This isn't capturing all the changes. For example, enabling marketo__enable_campaigns adds the campaign_type and program_id in the marketo__email_sends model. Also it would be good to add in here the upstream staging models these enable as well. Especially since Quickstart users will only see this changelog not and not the upstream one.
There was a problem hiding this comment.
Added a more in-depth changelog section!
| {{ config( | ||
| tags="fivetran_validations", | ||
| enabled=var('fivetran_validation_tests_enabled', false) | ||
| ) }} |
There was a problem hiding this comment.
Can you add to this model a variable to disable it if the dependent campaigns are disabled.
| {{ config( | ||
| tags="fivetran_validations", | ||
| enabled=var('fivetran_validation_tests_enabled', false) | ||
| ) }} |
There was a problem hiding this comment.
Can you add to this model a variable to disable it if the dependent programs are disabled.
There was a problem hiding this comment.
Thanks so much @fivetran-reneeli! This last iteration of changes look good to me with one comment in the integration_tests/dbt_project.yml to either remove or comment out some configs.
This is ready for release review.
integration_tests/dbt_project.yml
Outdated
| marketo__enable_campaigns: false ## Uncomment for testing to disable campaigns | ||
| # marketo__enable_programs: false ## Uncomment for testing to disable programs |
There was a problem hiding this comment.
These both need to either be commented out or removed.
There was a problem hiding this comment.
Ah forgot to comment this back out. Thanks!
PR Overview
This PR will address the following Issue/Feature: #41
This PR will result in the following new package version: v0.13.0
Breaking since this will enable these models for users who originally had these disabled by default. They will need to manually configure them to False is they wish to continue disabling them.Please provide the finalized CHANGELOG entry which details the relevant changes included in this PR:
Breaking Change
disabletoenablefor themarketo__enable_campaignsandmarketo__enable_programsvariables since the Quickstart product can dynamically enable/disable such components. If you are not a Quickstart user nor are syncing the respectivecampaignorprogramtables, you must configure the variables accordingly. Disable the related models or fields by adding the following to yourdbt_project.ymlfile:Refer to the README for more details.
PR Checklist
Basic Validation
Please acknowledge that you have successfully performed the following commands locally:
Before marking this PR as "ready for review" the following have been applied:
Detailed Validation
Please share any and all of your validation steps:
Run without the relevant variables configured, then run with them set to False.
If you had to summarize this PR in an emoji, which would it be?
💃