Feature/historical schedules #55
Conversation
| /* | ||
| The below macro is used to generate the correct SQL for package staging models. It takes a list of columns | ||
| that are expected/needed (staging_columns from dbt_zendesk_source/models/tmp/) and compares it with columns | ||
| in the source (source_columns from dbt_zendesk_source/macros/). | ||
| For more information refer to our dbt_fivetran_utils documentation (https://github.com/fivetran/dbt_fivetran_utils.git). | ||
| */ |
There was a problem hiding this comment.
I don't imagine this comment is entirely necessary. We can probably remove it.
There was a problem hiding this comment.
This was copied over since all the stg models in Zendesk have this language. Should I remove it from all of them?
There was a problem hiding this comment.
That's unnecessary for the scope of this task. We can leave this as is.
There was a problem hiding this comment.
@fivetran-joemarkiewicz I had one small question. I also added the changelog and regenned docs. Ready for re-review!
| /* | ||
| The below macro is used to generate the correct SQL for package staging models. It takes a list of columns | ||
| that are expected/needed (staging_columns from dbt_zendesk_source/models/tmp/) and compares it with columns | ||
| in the source (source_columns from dbt_zendesk_source/macros/). | ||
| For more information refer to our dbt_fivetran_utils documentation (https://github.com/fivetran/dbt_fivetran_utils.git). | ||
| */ |
There was a problem hiding this comment.
This was copied over since all the stg models in Zendesk have this language. Should I remove it from all of them?
There was a problem hiding this comment.
@fivetran-catfritz thanks for your work on this PR! After a more thorough review alongside the transform PR, I have a few change requests and questions. Let me know if you would like to sync on any of my comments.
| /* | ||
| The below macro is used to generate the correct SQL for package staging models. It takes a list of columns | ||
| that are expected/needed (staging_columns from dbt_zendesk_source/models/tmp/) and compares it with columns | ||
| in the source (source_columns from dbt_zendesk_source/macros/). | ||
| For more information refer to our dbt_fivetran_utils documentation (https://github.com/fivetran/dbt_fivetran_utils.git). | ||
| */ |
There was a problem hiding this comment.
That's unnecessary for the scope of this task. We can leave this as is.
There was a problem hiding this comment.
@fivetran-joemarkiewicz This is ready for re-review. Thank you!
There was a problem hiding this comment.
@fivetran-catfritz this looks great! Approved with just two small documentation notes then this is good to go!
README.md
Outdated
| ### Step 4: Disable models for non-existent sources | ||
| This package takes into consideration that not every Zendesk Support account utilizes the `schedule`, `domain_name`, `user_tag`, `organization_tag`, or `ticket_form_history` features, and allows you to disable the corresponding functionality. By default, all variables' values are assumed to be `true`. Add variables for only the tables you want to disable: | ||
| ### Step 4: Enable/Disable models for non-existent sources | ||
| This package takes into consideration that not every Zendesk Support account utilizes the `schedule`, `schedule_holiday`, `audit_log`, `domain_name`, `user_tag`, `organization_tag`, or `ticket_form_history` features, and allows you to disable the corresponding functionality. By default, all variables' values are assumed to be `true`, except for `using_schedule_histories`. Add variables for only the tables you want to enable/disable: |
There was a problem hiding this comment.
I noticed you removed the ticket_schedule, daylight_time, and time_zone. However, these are all dependent on the using_schedules variable (reference). Can we still include these sources and maybe it would help to call them out in the comment of the variable shown in the example below.
Co-authored-by: Joe Markiewicz <74217849+fivetran-joemarkiewicz@users.noreply.github.com>
|
I have addressed the comments, so this is ready for release review. |
Co-authored-by: Jamie Rodriguez <65564846+fivetran-jamie@users.noreply.github.com>
There was a problem hiding this comment.
@fivetran-jamie Thank you for reviewing so quickly! I'm going to leave run_models.sh as is for now, but let me know if you feel strongly otherwise.
PR Overview
This PR will address the following Issue/Feature:
This PR will result in the following new package version:
Please provide the finalized CHANGELOG entry which details the relevant changes included in this PR:
PR Checklist
Basic Validation
Please acknowledge that you have successfully performed the following commands locally:
dbt run (if incremental models are present) && dbt testBefore marking this PR as "ready for review" the following have been applied:
Detailed Validation
Please share any and all of your validation steps:
If you had to summarize this PR in an emoji, which would it be?
💃