Feature/add union data #44
Conversation
There was a problem hiding this comment.
These changes look great to me! I have a few small comments below, but otherwise this is good to generate the docs and be approved!
As discussed during standup, we will hold off merging and releasing this until all other union data updates are completed this quarter.
| # - package: fivetran/fivetran_utils | ||
| # version: [">=0.4.0", "<0.5.0"] | ||
| # - local: ../../dbt_fivetran_utils | ||
| - git: https://github.com/fivetran/dbt_fivetran_utils.git | ||
| revision: feature/enhance-union-data | ||
| warn-unpinned: false |
There was a problem hiding this comment.
Reminder to swap before release
There was a problem hiding this comment.
@fivetran-jamie thanks for working through this PR! I do have a few comments and questions below that I would like for you to take a look at before approving. Let me know if you would like to discuss any of these in more detail. Thanks!
README.md
Outdated
|
|
||
| <details><summary><i>Expand for source configuration template</i></summary><p> | ||
|
|
||
| > **Note**: If there are source tables you do not have (see [Step 4](https://github.com/fivetran/dbt_zendesk_source?tab=readme-ov-file#step-4-disable-models-for-non-existent-sources)), you may still include them, as long as you have set the right variables to `False`. Otherwise, you may remove them from your source definition. |
There was a problem hiding this comment.
Contradicting my previous comment, I am worried this addition will become unmanageable to maintain over time. What are some possible alternatives?
There was a problem hiding this comment.
are you referring to the source template or the note on line 80 specifically?
if it's the source template, we could just link to the src_zendesk.yml file and instruct users to copy/paste and replace certain attributes (like source name, database, schema)
| tests: | ||
| - dbt_utils.unique_combination_of_columns: | ||
| combination_of_columns: | ||
| - time_zone | ||
| - source_relation |
There was a problem hiding this comment.
This test is resulting in a failure when running on the zendesk_yashwanth schema. Do you have any understanding why this is failing?
There was a problem hiding this comment.
It looks like this is the result of the time_zone name creating a struct when we don't want it to. I recall you had a fix for this. Do you know if that may have been missed for some reason?
There was a problem hiding this comment.
Could it be that we need to add the alias to the union_data macro here.
So it would be:
{%- if table_exists -%}
select
{{ dbt_utils.star(from=relation.value) }}
from {{ relation.value }} as {{ table_identifier }}_tableThere was a problem hiding this comment.
ah yes i missed adding this to the single-connector use-case inside union_data
There was a problem hiding this comment.
that did the trick!
PR Overview
This PR will address the following Issue/Feature:
fivetran/dbt_zendesk#124
This PR will result in the following new package version:
v0.14.0
Please provide the finalized CHANGELOG entry which details the relevant changes included in this PR:
for now, i have a very very brief entry that points to the README
dbt_zendesk_source v0.11.0
🎉 Feature Update 🎉
This release supports running the package on multiple Zendesk sources at once! See the README for details on how to leverage this feature (PR #44).
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:
See hex notebook linked in Height