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

Docs/decisionlog timezones #131

Merged
merged 7 commits into from
Jan 15, 2025
Merged

Conversation

fivetran-catfritz
Copy link
Contributor

@fivetran-catfritz fivetran-catfritz commented Jan 14, 2025

PR Overview

This PR will address the following Issue/Feature:

@fivetran-catfritz fivetran-catfritz self-assigned this Jan 14, 2025
@fivetran-catfritz fivetran-catfritz linked an issue Jan 14, 2025 that may be closed by this pull request
4 tasks
DECISIONLOG.md Outdated Show resolved Hide resolved
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.

Great writeup, couple of comments around the timezone table

DECISIONLOG.md Outdated Show resolved Hide resolved
DECISIONLOG.md Outdated
Although this presents challenges within this package, customers can achieve standardization by configuring all Ad accounts and connectors to UTC whenever possible.

| Platform | Grain of Data | Timezone | Hourly Custom Reports? | Can Configure Time Zones for Custom Reports? |
Copy link
Contributor

Choose a reason for hiding this comment

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

Kinda wondering if this is too much information...thoughts on limiting this table to
Platform, Grain of Source Data, and Timezone?

I worry that talking about custom reports, given that they're not used in the package, may add some confusion here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed--updated!

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-jamie! I made the updates, so this is ready for re-review.

DECISIONLOG.md Outdated
Although this presents challenges within this package, customers can achieve standardization by configuring all Ad accounts and connectors to UTC whenever possible.

| Platform | Grain of Data | Timezone | Hourly Custom Reports? | Can Configure Time Zones for Custom Reports? |
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed--updated!

DECISIONLOG.md Outdated Show resolved Hide resolved
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.

Left some more comments!

DECISIONLOG.md Outdated Show resolved Hide resolved
DECISIONLOG.md Outdated Show resolved Hide resolved
DECISIONLOG.md Outdated 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-jamie. Made the suggested comments!

DECISIONLOG.md Outdated Show resolved Hide resolved
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.

lgtm!

@fivetran-catfritz fivetran-catfritz merged commit a82f473 into main Jan 15, 2025
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.

[Feature] Document platform timezone differences
2 participants