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

bugfix/revised-parent-id-datatype #122

Merged
merged 8 commits into from
Mar 25, 2024

Conversation

fivetran-joemarkiewicz
Copy link
Contributor

@fivetran-joemarkiewicz fivetran-joemarkiewicz commented Mar 21, 2024

PR Overview

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

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

The changing datatype of the revised_parent_issue_id field from int to string will result in a breaking change. There is no impact on incremental models, so no full refresh is required. But we should categorize this as a breaking change.

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

🚨 Breaking Changes: Bug Fixes 🚨

  • The following fields in the below mentioned models have been converted to a string datatype (previously integer) to ensure classic Jira projects may link issues to epics. If you are referencing these fields downstream, be sure to make any changes to account for the new datatype.
    • revised_parent_issue_id field within the int_jira__issue_type_parents model
    • parent_issue_id field within the jira__issue_enhanced model

Under the Hood

  • Included auto-releaser GitHub Actions workflow to automate future releases.
  • Updated the maintainer PR template to resemble the most up to date format.
  • Updated field and issue_field_history seed files to ensure we have an updated test case to capture the epic-link scenario for classic Jira environments.

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 see the corresponding Height ticket for a link to the Hex validation doc.

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

🧶

@fivetran-joemarkiewicz fivetran-joemarkiewicz marked this pull request as ready for review March 21, 2024 17:41
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.

@fivetran-joemarkiewicz This generally looks good! I was able to reproduce the error with the seeds from this PR and then resolve it with the new branch.

My remaining question is that in other packages, we preferred to cast in the source package rather than in the downstream joins, so we only had to cast once. Do we not do this in this case because the intermediate models get a bit complicated and this was more straightforward or another reason?

CHANGELOG.md Outdated
[PR #122](https://github.com/fivetran/dbt_jira/pull/122) contains the following updates:

## 🚨 Breaking Changes: Bug Fixes 🚨
- The following fields in the below mentioned models have been converted to a string datatype (previously integer) to ensure classic Jira projects may link issues to epics. If you are referencing these fields downstream, be sure to make any changes to account for the new datatype.
Copy link
Contributor

Choose a reason for hiding this comment

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

It might help to add a line explaining how the classic projects were formatted, or why this benefits the classic projects.

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 think that would be helpful. Just added a sentence explaining this a bit more.

Copy link
Contributor

Choose a reason for hiding this comment

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

Super helpful!

@fivetran-joemarkiewicz
Copy link
Contributor Author

My remaining question is that in other packages, we preferred to cast in the source package rather than in the downstream joins, so we only had to cast once. Do we not do this in this case because the intermediate models get a bit complicated and this was more straightforward or another reason?

Great question @fivetran-catfritz! You are correct that I would prefer we typically cast the fields as string in the staging models. However, I did not take the approach here as it was a more straightforward approach and when exploring casting the upstream fields as strings I found that this PR would have quite a larger fingerprint and cause significantly more breaking changes (with little to no value). Because of this I decided taking the direct approach was the more efficient way to go about addressing this issue for both us and the end user. In this case the end user only has to be concerned about the revised_parent_issue_id being changed to a string datatype. If we were to cast the fields (particularly the issue_id field) upstream it would involve more models and fields that would be breaking.

Let me know if you have any other questions!

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.

@fivetran-joemarkiewicz thanks for the explanation--makes perfect sense. Approved!

## 🚨 Breaking Changes: Bug Fixes 🚨
- The following fields in the below mentioned models have been converted to a string datatype (previously integer) to ensure classic Jira projects may link issues to epics. In classic Jira projects the epic reference is in a hyperlink form (ie. "https://ulr-here/epic-key") as opposed to an ID. As such, a string datatype is needed to successfully link issues to epics. If you are referencing these fields downstream, be sure to make any changes to account for the new datatype.
- `revised_parent_issue_id` field within the `int_jira__issue_type_parents` model
- `parent_issue_id` field within the `jira__issue_enhanced` model
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see parent_issue_id in the jira__issue_enhanced. Did you mean issue_id? Or did you mean to mention a different model, int_jira__issue_users?

Also, if you're mentioning int_jira__issue_type_parents, should you mention the changes in int_jira__issue_epic and int_jira__issue_users too?

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 don't see parent_issue_id in the jira__issue_enhanced. Did you mean issue_id? Or did you mean to mention a different model, int_jira__issue_users?

parent_issue_id is included in the jira__issue_enhanced model. It is not included in the code due to it being brought in via the *.

image

Also, if you're mentioning int_jira__issue_type_parents, should you mention the changes in int_jira__issue_epic and int_jira__issue_users too?

These are both ephemeral models which which will not result in any destination changes on the customers side. This is why they were not listed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Got it, thanks for clarifying, looks good!

Copy link
Contributor

@fivetran-reneeli fivetran-reneeli left a comment

Choose a reason for hiding this comment

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

Just had a question about the changelog!

Copy link
Contributor

@fivetran-reneeli fivetran-reneeli left a comment

Choose a reason for hiding this comment

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

looks good

@fivetran-joemarkiewicz fivetran-joemarkiewicz merged commit c1e6fc8 into main Mar 25, 2024
8 checks passed
@fivetran-joemarkiewicz fivetran-joemarkiewicz deleted the bugfix/revised-parent-id-datatype branch March 25, 2024 15:02
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