-
Notifications
You must be signed in to change notification settings - Fork 5
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
updates and rm run results in docs #39
Conversation
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-reneeli this looks good to me, but I have one documentation request to add before approval. Let me know if you have questions. Thanks!
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.
Just one comment to update the CHANGELOG. Otherwise looks good!
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.
LGTM just a few final requests to update the CHANGELOG
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.
This PR looks good, just a few final change requests before opening for release review.
CHANGELOG.md
Outdated
## Contributors | ||
- [@justin-fundrise](https://github.com/justin-fundrise) |
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.
If there is no PR linked in the source for the contribution then we shouldn't include this section in the CHANGELOG. We should only include a contributing section when there is a PR linked within the package update.
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.
Ah ok! That makes a lot of sense
Co-authored-by: Joe Markiewicz <74217849+fivetran-joemarkiewicz@users.noreply.github.com>
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.
LGTM just one CHANGELOG edit
Co-authored-by: Joe Markiewicz <74217849+fivetran-joemarkiewicz@users.noreply.github.com>
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-reneeli Great work! A few comments, but should be good to merge after that.
Co-authored-by: Avinash Kunnath <[email protected]>
Co-authored-by: Avinash Kunnath <[email protected]>
Co-authored-by: Avinash Kunnath <[email protected]>
PR Overview
This PR will address the following Issue/Feature:
#36
#37
This PR will result in the following new package version:
v0.10.0
breaking change since this could affect runs and will result in downstream changes as well
Please provide the finalized CHANGELOG entry which details the relevant changes included in this PR:
Breaking Change (
--full-refresh
required after upgrading)stg_iterable__user_history
from table to view in order to improve performance. A--full-refresh
is required after upgrading to ensure there are no issues.Bug Fix
src_iterable.yml
. This was originally causing run errors for users where tables weren't present in their schema but listed in thesrc_iterable.yml
.created_at
from the uniqueness test forstg_iterable__event
. Uniqueness is now tested solely onunique_event_id
, a surrogate key made up ofevent_id
(_fivetran_id
in the raw table, which is a Fivetran-created unique identifier derived from hashing campaign_id, created_at, and event_name) and_fivetran_user_id
(a Fivetran-created column derived from a hash ofuser_id
and/oremail
).Documentation
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:
If you had to summarize this PR in an emoji, which would it be?
💃