-
Notifications
You must be signed in to change notification settings - Fork 39
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/add union data #133
base: main
Are you sure you want to change the base?
Conversation
Bumps [tj-actions/changed-files](https://github.com/tj-actions/changed-files) from 29.0.4 to 41.0.0. - [Release notes](https://github.com/tj-actions/changed-files/releases) - [Changelog](https://github.com/tj-actions/changed-files/blob/main/HISTORY.md) - [Commits](tj-actions/changed-files@v29.0.4...v41.0.0) --- updated-dependencies: - dependency-name: tj-actions/changed-files dependency-type: direct:production ... Signed-off-by: dependabot[bot] <[email protected]>
…ithub/workflows/tj-actions/changed-files-41.0.0 Bump tj-actions/changed-files from 29.0.4 to 41.0.0 in /.github/workflows
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-jamie the results of this PR look great! I just have a few questions and suggestions in the comments below that I would like your eyes on before we approve this. Let me know if you have any questions or want to chat about these further.
.github/workflows/check_docs.yml
Outdated
@@ -18,7 +18,7 @@ jobs: | |||
|
|||
- name: Get changed files | |||
id: changed-files | |||
uses: tj-actions/changed-files@v29.0.4 | |||
uses: tj-actions/changed-files@v41.0.0 |
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.
Let's remove this as we will use a different docs check this year
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.
removed and added note about removal to the changelog
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_hubspot_source?tab=readme-ov-file#step-4-disable-models-for-non-existent-sources)), you may still include them here, 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment about if there is a more concise and manageable option for sharing this information with the users.
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.
updated
README.md
Outdated
@@ -150,7 +1444,12 @@ vars: | |||
hubspot_service_enabled: true # Enables all service/ticket models. Default = false | |||
hubspot_ticket_deal_enabled: true # Default = false | |||
``` | |||
|
|||
### Dbt-core Version Requirement for disabling freshness tests |
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.
We need to include the proper reference to dbt when mentioning it within our docs.
### Dbt-core Version Requirement for disabling freshness tests | |
### dbt Core™ Version Requirement for disabling freshness tests |
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.
removed it wholly
# dbt_hubspot v0.16.0 | ||
|
||
## 🎉 Feature Update 🎉 | ||
- This release supports running the package on multiple Hubspot sources at once! See the [README](https://github.com/fivetran/dbt_hubspot?tab=readme-ov-file#step-3-define-database-and-schema-variables) for details on how to leverage this feature ([PR #133](https://github.com/fivetran/dbt_hubspot/pull/133)). |
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 think it is also very relevant to include here that customers will not also see a new source_relation
field in their end models.
Additionally, we should flag that customers using the ticket models will need to run a full refresh to capture the new schema 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.
added
@@ -9,13 +9,13 @@ | |||
file_format = 'parquet' | |||
) | |||
}} | |||
|
|||
-- depends_on: {{ ref('stg_hubspot__ticket') }} |
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 worry this will now cause compilation errors for customers who try to run dbt compile
when first trying to use the package.
What is the limitation of ensuring this model still references the source?
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'll add the same check i just added to zendesk so that non-unioning users will still use the source
# - package: fivetran/hubspot_source | ||
# version: [">=0.15.0", "<0.16.0"] | ||
- git: https://github.com/fivetran/dbt_hubspot_source.git | ||
revision: feature/add-union-data | ||
warn-unpinned: false |
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.
Reminder to update before merge.
README.md
Outdated
@@ -150,7 +1444,12 @@ vars: | |||
hubspot_service_enabled: true # Enables all service/ticket models. Default = false | |||
hubspot_ticket_deal_enabled: true # Default = false | |||
``` | |||
|
|||
### Dbt-core Version Requirement for disabling freshness tests | |||
If you are not using a source table that involves freshness tests, please be aware that the feature to disable freshness was only introduced in dbt-core 1.1.0. Therefore ensure the dbt version you're using is v1.1.0 or greater for this config to work. |
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.
Actually, I just realized this package requires the use of dbt v1.3.0 or higher. So I think we may be able to remove this messaging in both the source and transform.
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.
removed
PR Overview
This PR will address the following Issue/Feature:
#130
This PR will result in the following new package version:
v0.16.0
Please detail what change(s) this PR introduces and any additional information that should be known during the review of this PR:
source_relation
in every:dbt_hubspot v0.16.0
🎉 Feature Update 🎉
📝 Documentation 📝
hubspot__deal_changes
to better reflect the grain of the model (PR #132).🛠️ Under the Hood 🛠️
PR Checklist
Basic Validation
Please acknowledge that you have successfully performed the following commands locally:
hubspot__pass_through_all_columns
values and specific passthrough columns withadd_property_label: true
Before marking this PR as "ready for review" the following have been applied:
Detailed Validation
Please acknowledge that the following validation checks have been performed prior to marking this PR as "ready for review":
See Hex notebook linked in Height
Standard Updates
Please acknowledge that your PR contains the following standard updates:
dbt Docs
Please acknowledge that after the above were all completed the below were applied to your branch:
If you had to summarize this PR in an emoji, which would it be?
🍨