-
Notifications
You must be signed in to change notification settings - Fork 41
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/variable_calendar_start #84
Conversation
will wait to regen docs til approved |
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 thanks for working on this PR and getting this new variable included! Generally this PR is looking good and I just have a few documentation related changes I would like you to make before approving.
Additionally, please add more details to your validation section of the PR template when not relying on our validation tests. I don't believe validation tests are needed for this change, but a quick screenshot will really help to provide more confidence to the claim that the variable works as expected.
For example a screenshot like the following highlights that the variable is working as expected and the default config is matching the expected results from before the changes.
Co-authored-by: Joe Markiewicz <[email protected]>
Thanks @fivetran-joemarkiewicz ! And yeah for validations, I left the variable I had tested with in our internal notes but definitely could've screenshotted what I was looking at since there's no PII, noted for future reference. |
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, be sure to regenerate the docs before moving to release review. Thanks @fivetran-reneeli!
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 A small edit but otherwise lgtm!
Co-authored-by: fivetran-catfritz <[email protected]>
PR Overview
This PR will address the following Issue/Feature:
#64
This PR will result in the following new package version:
v0.12.2
Please provide the finalized CHANGELOG entry which details the relevant changes included in this PR:
Feature: Customizable date start
shopify__calendar_start_date
. This can be set in your dbt_project.yml. If not used, the default will start at2019-01-01
. See the README for more details.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:
Tested by configuring the variable and running against internal data
If you had to summarize this PR in an emoji, which would it be?
💃