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] Enable User models by default #17

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

fivetran-avinash
Copy link

@fivetran-avinash fivetran-avinash commented Dec 27, 2024

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

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

Documentation Update

  • Updated the variable configuration section of the README since servicenow__using_roles is now set to true by default.
  • Moved badges at top of the README below the H1 header to be consistent with popular README formats.

Under the Hood

  • Changed Buildkite scripts to run models when 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:

  • dbt run –full-refresh && dbt test
  • [NA] dbt run (if incremental models are present) && dbt test

Before marking this PR as "ready for review" the following have been applied:

  • The appropriate issue has been linked, tagged, and properly assigned
  • All necessary documentation and version upgrades have been applied
  • docs were regenerated (unless this PR does not include any code or yml updates)
  • BuildKite integration tests are passing
  • Detailed validation steps have been provided below

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.

Screenshot 2024-12-27 at 3 01 36 AM

If you had to summarize this PR in an emoji, which would it be?

🍾

@fivetran-avinash fivetran-avinash self-assigned this Dec 27, 2024
@@ -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.
Copy link
Author

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

Copy link
Collaborator

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

@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.
Copy link
Collaborator

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.
Copy link
Collaborator

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)) }}
Copy link
Collaborator

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.

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