Respect system/user timezone in API requests by cristiand391 · Pull Request #2630 · cli/cli · GitHub
Skip to content

Respect system/user timezone in API requests#2630

Merged
vilmibm merged 2 commits into
cli:trunkfrom
cristiand391:api-respect-timezone
Jan 20, 2021
Merged

Respect system/user timezone in API requests#2630
vilmibm merged 2 commits into
cli:trunkfrom
cristiand391:api-respect-timezone

Conversation

@cristiand391

@cristiand391 cristiand391 commented Dec 15, 2020

Copy link
Copy Markdown
Contributor

Fixes #1504

When doing an action that requires a non-GET, non-HEAD request and TZ is set, add the Time-Zone header for that request.
If TZ isn't set, fall back to a known timezone name.

@mislav mislav left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for taking this on!

Comment thread pkg/cmd/factory/http.go Outdated
Comment thread pkg/cmd/factory/http.go Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It looks like that, on macOS, the TZ environment variable is not set by default, and therefore time.Local is always Local. However, even if Go does not know the detailed name of my time zone, my system clock does dictate a +1h offset from UTC, as evident by time.Now().Zone(). So when TZ isn't set, could we map that offset to a known time zone name (it doesn't matter which one, as long as it has the same offset) and send that as the header value?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It looks like that, on macOS, the TZ environment variable is not set by default, and therefore time.Local is always Local

The same happens on Linux, see golang/issues/37855. It only shows the timezone name when TZ is set and exported.

So when TZ isn't set, could we map that offset to a known time zone name

Is it safe to get the timezone from UTC offset for this use case?

From what I've read there are timezones with more than one offset from UTC (DST) so, if picked the wrong one, wouldn't github generate a wrong timestamp based on bad transition zone info?

@mislav mislav Dec 18, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

From what I've read there are timezones with more than one offset from UTC (DST) so, if picked the wrong one, wouldn't github generate a wrong timestamp based on bad transition zone info?

You have a good point that, at different times of the year, a single time zone can have varying offsets from UTC due to DST. However, the way I see it, the most important thing we're trying to do when reporting the user's time zone to the server is to have the creation timestamps recorded with the right offset. We might accidentally record the wrong time zone name (unless we have precise information about the time zone), but as long as we record the right offset as it is at the present, I feel that it is already an improvement over not specifying any time zone information at all. The UTC offset for a specific point in time in a year does not change even with DST applied.

BTW, I've just checked in GitHub's codebase for what happens if the Time-Zone header is not there or when it's malformed, and it turns out GitHub defaults to the time zone that it has stored for the authenticating user (based on their last web activity), and UTC if the user's last time zone is unknown.

@Kizzer415

This comment was marked as spam.

@cristiand391 cristiand391 requested a review from mislav December 21, 2020 12:20

@mislav mislav left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This looks great. Thanks for solving this!

@iamdiegoc8

This comment was marked as spam.

@vilmibm vilmibm merged commit 2086d13 into cli:trunk Jan 20, 2021
@cristiand391 cristiand391 deleted the api-respect-timezone branch January 20, 2021 18:58
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.

Respect system/user timezone in API requests

5 participants