-
Notifications
You must be signed in to change notification settings - Fork 1
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] Enable User models by default #17
base: main
Are you sure you want to change the base?
Conversation
@@ -27,8 +27,6 @@ vars: | |||
servicenow_sys_user_has_role_identifier: "sys_user_has_role_data" | |||
servicenow_sys_user_grmember_identifier: "sys_user_grmember_data" | |||
|
|||
# servicenow__using_roles: true # to be turned on when generating docs so that can be generated via the manifest and docs as it is turned off by default. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no longer needed as this is now true by default
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fivetran-avinash thanks for this update! A few change requests before this will be ready for approval.
|
||
## Breaking Changes | ||
- Enabled `servicenow__user_aggregated` and `servicenow__user_enhanced` by default by changing the default `servicenow__using_roles` value to true. | ||
- By setting the `servicenow__using_roles` variable to true, we now also enable the upstream `stg_servicenow_*` models that flow into these user tables, which derive from the `sys_user_grmember`, `sys_user_has_role`, and `sys_user_role` source tables. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I want to clearly callout what new models will be materialized in the warehouse by default. Can you add onto this entry sub-bullets which list out all the new models which will be enabled by default.
Additionally, we should mention that Quickstart will dynamically handle the enable/disabling based on the existence of the source tables or via the Quickstart data model selection tab. However, non Quickstart users will be required to define the variable themselves in their dbt_project.yml if they do not have the required source tables.
## Breaking Changes | ||
- Enabled `servicenow__user_aggregated` and `servicenow__user_enhanced` by default by changing the default `servicenow__using_roles` value to true. | ||
- By setting the `servicenow__using_roles` variable to true, we now also enable the upstream `stg_servicenow_*` models that flow into these user tables, which derive from the `sys_user_grmember`, `sys_user_has_role`, and `sys_user_role` source tables. | ||
- Because this change will introduce new end model tables to users because they were initially disabled by default, we've classified this as a breaking change. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is also a breaking change because we are changing the default behavior of a variable. Please also include here that users who do not have these source tables will need to define the variable in their dbt_project.yml
@@ -1,4 +1,4 @@ | |||
{{ config(enabled=var('servicenow__using_roles', False)) }} | |||
{{ config(enabled=var('servicenow__using_roles', True)) }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you also add the same enabled config to the relevant validation tests (example). This way when running validation tests we avoid false failures if the models are disabled when testing between dev and prod.
PR Overview
This PR will address the following Issue/Feature: [#14] and [#16]
This PR will result in the following new package version: 0.4.0
New models are now enabled by default so this will create new end models for customers, particularly in Quickstart.
Please provide the finalized CHANGELOG entry which details the relevant changes included in this PR:
Breaking Changes
servicenow__user_aggregated
andservicenow__user_enhanced
by default by changing the defaultservicenow__using_roles
value to true.servicenow__using_roles
variable to true, we now also enable the upstreamstg_servicenow_*
models that flow into these user tables, which derive from thesys_user_grmember
,sys_user_has_role
, andsys_user_role
source tables.Documentation Update
servicenow__using_roles
is now set to true by default.Under the Hood
servicenow__using_roles
is set to False (since it's now True by default on all models).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:
Ran and validated that the tables compiled when
servicenow__using_roles
was True or False, then ensured validation tests passed on our development data.If you had to summarize this PR in an emoji, which would it be?
🍾