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

update/get-ticket-property-history-columns #126

Merged
merged 10 commits into from
Jul 31, 2024

Conversation

fivetran-catfritz
Copy link
Contributor

@fivetran-catfritz fivetran-catfritz commented Jul 24, 2024

PR Overview

This PR will address the following Issue/Feature:

  • T-746924
  • _fivetran_end, which should be brought in through stg_hubspot__ticket_property_history, is used downstream in int_hubspot__daily_ticket_history. It only creates an issue if the field is not present, which for most users would never be the case. I only discovered this since I was using the ticket_property_history_data seed from the source, which is different from the seed in the transform, which has this field.
  • Since stg_hubspot__ticket_property_history relies on the macro for the missing fields, adding _fivetran_end resolves the issue.

This PR will result in the following new package version:

  • v0.15.0 since there is the possibility of a new column for users.

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

🚨 Breaking Changes 🚨

  • Added field _fivetran_end to macro get_ticket_property_history_columns() to ensure the column available for use downstream.
    • While this should not affect most users, this will add a new column _fivetran_end to stg_hubspot__ticket_property_history if you do not have _fivetran_end in your source TICKET_PROPERTY_HISTORY table.

Documentation

  • Update documentation to include _fivetran_end under model stg_hubspot__ticket_property_history.

Under the hood

  • Updated the maintainer PR template to the current format.
  • Included auto-releaser GitHub Actions workflow to automate future releases.

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:

  • The downstream issue is resolved when adding this.

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

💃

@fivetran-catfritz fivetran-catfritz self-assigned this Jul 24, 2024
Copy link
Contributor

@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-catfritz thanks for this quick turnaround! I just have a few small comments. Let me know if you want to sync on any of these comments.

.github/workflows/stale.yml Outdated Show resolved Hide resolved
.github/workflows/stale.yml Outdated Show resolved Hide resolved
CHANGELOG.md Show resolved Hide resolved
Copy link
Contributor Author

@fivetran-catfritz fivetran-catfritz 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-joemarkiewicz. I have made this breaking and removed the staleness workflow.

.github/workflows/stale.yml Outdated Show resolved Hide resolved
.github/workflows/stale.yml Outdated Show resolved Hide resolved
CHANGELOG.md Show resolved Hide resolved
Copy link
Contributor

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

Just a few small suggestions. Otherwise this looks great!

CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@@ -49,6 +49,8 @@ models:
description: '{{ doc("_fivetran_synced") }}'
- name: _fivetran_start
description: The (UTC DATETIME) to keep track of the time when a record is first created or modified in the source database.
- name: _fivetran_end
Copy link
Contributor

Choose a reason for hiding this comment

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

We should probably add this to the src_hubspot.yml as well. It looks as if _fivetran_start is not in the src_hubspot.yml either.

Copy link
Contributor

Choose a reason for hiding this comment

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

_fivetran_start and _fivetran_end are not present in the ticket_property_history seed file. Should we add those in with the proper data?

Copy link
Contributor

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

@fivetran-catfritz Looks mostly good! A few comments before approving.

(cc: @fivetran-joemarkiewicz if you are taking this on.)

- Update documentation to include `_fivetran_end` under model `stg_hubspot__ticket_property_history`.

## Under the hood
- Updated the maintainer PR template to the current format.
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's add a note that we are adding integration testing for Databricks SQL Warehouse.

Copy link
Contributor

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

@fivetran-joemarkiewicz One minor changelog note but approved!

@fivetran-catfritz fivetran-catfritz merged commit d1d6e9c into main Jul 31, 2024
8 checks passed
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.

3 participants