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.

Sign up for GitHub

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

Jump to bottom

bugfix/daylight-end-catch #128

Merged
merged 12 commits into from
Jan 4, 2024
Merged

bugfix/daylight-end-catch #128

merged 12 commits into from
Jan 4, 2024

Conversation

fivetran-joemarkiewicz
Copy link
Contributor

@fivetran-joemarkiewicz fivetran-joemarkiewicz commented Dec 5, 2023 •
edited
Loading

PR Overview

This PR will address the following Issue/Feature: #121

This PR will result in the following new package version: v0.13.1

This will not be a breaking change. Instead, this will update the int_zendesk_schedule_spine model so schedules that have daylight saving times that have ended are properly being reported into the future. As none of the downstream models are incremental, this will not be a breaking change.

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 –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:

Please inspect the corresponding Height ticket for detailed validations of these changes. The validations for this PR include the following checks:

  • Understanding of the schedule (in this example Hong Kong)
  • Understanding of how the Hong Kong schedule behaves when DST ends at a specified point in time.
  • Validation of how the changes in this PR fix the issue for a ticket using the Hong Kong schedule in the current day and age when DST is no longer used.

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

⌛

@fivetran-joemarkiewicz fivetran-joemarkiewicz self-assigned this Dec 5, 2023
@fivetran-joemarkiewicz fivetran-joemarkiewicz marked this pull request as ready for review December 20, 2023 20:57
fivetran-jamie
fivetran-jamie approved these changes Dec 26, 2023
View reviewed changes
Copy link
Contributor

@fivetran-jamie fivetran-jamie left a comment

Choose a reason for hiding this comment

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

approved with some doc-related suggestions! also reminder to regen docs

CHANGELOG.md Outdated Show resolved Hide resolved
README.md Outdated
@@ -59,7 +59,7 @@ Include the following zendesk package version in your `packages.yml` file:
```yml
packages:
- package: fivetran/zendesk
version: [">=0.13.0", "<0.14.0"]
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.

should revert if this isn't a breaking change

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I had initially marked this as a breaking change as I was planning to include this in the next upcoming breaking change. But if we are going to introduce this earlier as a patch then we should adjust the versioning throughout.

dbt_project.yml Outdated Show resolved Hide resolved
integration_tests/dbt_project.yml Outdated Show resolved Hide resolved
.github/workflows/auto-release.yml
call-workflow-passing-data:
if: github.event.pull_request.merged
uses: fivetran/dbt_package_automations/.github/workflows/auto-release.yml@main
secrets: inherit
Copy link
Contributor

Choose a reason for hiding this comment

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

just added the secret

CHANGELOG.md
@@ -1,3 +1,15 @@
# dbt_zendesk v0.14.0

## Bug Fixes
Copy link
Contributor

Choose a reason for hiding this comment

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

should we note that this only affects people who use schedules in zendesk? might be implied bust just to be uber clear

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah that's a good clarification to include.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated.

CHANGELOG.md Outdated
- For example, the Hong Kong timezone originally recognized DST, but them stopped in 1979. The previous versions of this package only recorded the schedule business hours until 1979. This update addresses this bug.

## Under the Hood
- Included auto-releaser GitHub Actions workflow to automate future releases. ([PR #128](https://github.com/fivetran/dbt_zendesk/pull/128))
Copy link
Contributor

Choose a reason for hiding this comment

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

should we include the updated maintainer PR template? or is it too under the hood...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that is a good think to include. Will update and thanks for calling out.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated.

@fivetran-joemarkiewicz @fivetran-jamie
Update CHANGELOG.md
163a01d 
Co-authored-by: Jamie Rodriguez <65564846+fivetran-jamie@users.noreply.github.com>
@fivetran-joemarkiewicz @fivetran-jamie
Update dbt_project.yml
fd89c07 
Co-authored-by: Jamie Rodriguez <65564846+fivetran-jamie@users.noreply.github.com>
@fivetran-joemarkiewicz @fivetran-jamie
Update integration_tests/dbt_project.yml
0384705 
Co-authored-by: Jamie Rodriguez <65564846+fivetran-jamie@users.noreply.github.com>
@fivetran-joemarkiewicz
review updates
f9b3f64
@fivetran-joemarkiewicz
docs regen
550df6f
fivetran-catfritz
fivetran-catfritz reviewed Dec 27, 2023
View reviewed changes
CHANGELOG.md Outdated Show resolved Hide resolved
@fivetran-joemarkiewicz @fivetran-catfritz
Update CHANGELOG.md
a9d2b1b 
Co-authored-by: fivetran-catfritz <111930712+fivetran-catfritz@users.noreply.github.com>
fivetran-catfritz
fivetran-catfritz approved these changes Dec 27, 2023
View reviewed changes
Copy link
Contributor

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

approved for release.

@fivetran-joemarkiewicz fivetran-joemarkiewicz merged commit d76e86f into main Jan 4, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fivetran-catfritz fivetran-catfritz fivetran-catfritz approved these changes

@fivetran-jamie fivetran-jamie fivetran-jamie approved these changes

Assignees

@fivetran-joemarkiewicz fivetran-joemarkiewicz

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@fivetran-joemarkiewicz @fivetran-jamie @fivetran-catfritz