-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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
fix(internal): Fix time parsing for abbreviated timezones #13363
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.
I have questions about the behavior and what we are expecting from the change.
After a short discussion, we concluded to make the change in behavior more visible. @powersj does the warning and Changelog text work for you? |
internal/internal.go
Outdated
once.Do(func() { | ||
log.Print("W! You are using abbreviated timezones for which parsing was fixed in v1.27.0.") | ||
log.Print("W! Please remove any workarounds in place and check your resulting times carefully!") | ||
log.Print("W! In case you experience any problems, please file an issue!") |
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.
log.Print("W! In case you experience any problems, please file an issue!") | |
log.Print("W! If you experience any problems, please file an issue on GitHub") |
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.
IMHO this line can be removed, this counts for any problem experienced, not only for timezones.
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.
@Hipska I like to encourage people to report the stuff, so the line is well spend IMO. This worked quite well for the gnmi plugin...
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.
Okay, but there it is in a single log message, not spread over multiple independent lines.. Please keep the messages together!
It is now fine as the multiple log messages are now also combined into one.
Download PR build artifacts for linux_amd64.tar.gz, darwin_amd64.tar.gz, and windows_amd64.zip. 📦 Click here to get additional PR build artifactsArtifact URLs |
resolves #12960
upstream issues golang/go#56528, golang/go#9617
Golang does not handle ambiguous timezones which holds true for almost all abbreviated timezones. As a workaround, (see Golang discussion) we can parse times with abbreviated timezones first to get the timezone's name and then forcefully parse the time-string in the found timezone. By doing this we can handle many of the abbreviated timezones correctly assuming the "most probable" of the ambiguous timezone abbreviations, e.g. "Mountain Standard Time" for "MST".