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] Brand and organization variable configs #181

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

Conversation

fivetran-avinash
Copy link
Contributor

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

PR Overview

This PR will address the following Issue/Feature: [#176]

This PR will result in the following new package version: 0.19.2

This adds variable configs that are set to true by default so it doesn't impact the nature of the downstream models unless explicitly set.

Please provide the finalized CHANGELOG entry which details the relevant changes included in this PR:

New Features

  • Introduced new config variables using_brands and using_organizations to allow the customers to enable and disable brand and organization source data.
  • End models that could be impacted by these variables include:
    • zendesk__ticket_backlog: using_brands and/or using_organizations can be set to false to disable brand and/or organization fields.
    • zendesk__ticket_enriched: using_organizations can be set to false to remove organization fields from the final model.
  • Intermediate models that could be impacted by these variables include:
    • int_zendesk__organization_aggregates: using_organizations can be utilized to completely disable the model.
    • int_zendesk__ticket_aggregates: using_brands can be set to false to remove brand fields from the final model.
    • int_zendesk__updater_information: using_organizations can be set to false to remove organization fields from the final model.

Under the Hood

  • Updated table_variables in the quickstart.yml with the new brand and organization tables.
  • Updated our Buildkite model run script to ensure we test for when using_brands and using_organizations is set to either true or false.

Documentation Updates

  • Updated README with instructions on how to disable brand and organization sources.

PR Checklist

Basic Validation

Please acknowledge that you have successfully performed the following commands locally:

  • dbt run –full-refresh && dbt test
  • 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:

Various versions of variable configs with the new fields were introduced, run and tested to ensure the variable configs worked for various permutations of organization/brand enablement or disablement. Validation testing on our Zendesk environment also successfully passed (with an additional filter provided for a new ticket that did not pass but was not introduced for the purposes of this issue)

Screenshot 2024-12-26 at 5 27 31 AM

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

🦷

@fivetran-avinash fivetran-avinash self-assigned this Dec 26, 2024
Copy link
Contributor Author

@fivetran-avinash fivetran-avinash left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reminder to commit before merge.

Comment on lines +2 to +4
- git: https://github.com/fivetran/dbt_zendesk_source.git
revision: feature/organization-brand-enable-disable
warn-unpinned: false
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- git: https://github.com/fivetran/dbt_zendesk_source.git
revision: feature/organization-brand-enable-disable
warn-unpinned: false
- package: fivetran/zendesk_source
version: [">=0.14.0", "<0.15.0"]

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we have the using_organizations config at the top, I don't think it would be needed in the rest of the model since in order for the model to run, it must be true.

requester_org.organization_tags as requester_organization_tags,
{% endif %}
--If you use organizations this will be included, if not it will be ignored.
{% if var('using_organizations', True) %}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps we can combine with line 89 into just 1 config?

requester_org.name as requester_organization_name,

@@ -147,6 +147,8 @@ vars:
using_domain_names: False #Disable if you are not using domain names
using_user_tags: False #Disable if you are not using user tags
using_ticket_form_history: False #Disable if you are not using ticket form history
using_brands: False #Disable if you are not using brands
using_organizations: False #Disable if you are not using organizations
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same callout as in source about also adding a note for organization_tag here

using_brands:
- brand
using_organizations:
- organization
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Believe we also need to add organization_tag here

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reminder to up the version

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wondering what this change was for and if we need to document this in the changelog?

[PR #181](https://github.com/fivetran/dbt_zendesk/pull/181) includes the following updates:

## New Features
- Introduced new config variables `using_brands` and `using_organizations` to allow the customers to enable and disable `brand` and `organization` source data.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- Introduced new config variables `using_brands` and `using_organizations` to allow the customers to enable and disable `brand` and `organization` source data.
- Introduced new config variables `using_brands` and `using_organizations` to allow customers to enable and disable `brand` and `organization` source data, resulting in potential downstream impacts:

Copy link
Contributor

@fivetran-reneeli fivetran-reneeli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @fivetran-avinash ! Have some suggestions in addition to a reminder to update the run script to test for using_brands and using_organizations being set to false.

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