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

Use pytz to calculate timezones #47788

Merged
merged 9 commits into from May 31, 2018

Conversation

Projects
None yet
4 participants
@twangboy
Contributor

twangboy commented May 23, 2018

What does this PR do?

Fixes issue where erroneous Timezones were being returned. Uses pytz module to get timzone information. Fixes timezone.get_offset so it actually returns an offset.

What issues does this PR fix or reference?

#47559

Previous Behavior

Everything was in a giant dictionary so the get_timezone just returned the first item it found in the dictionary. get_offset always returned false.

New Behavior

The dictionary only has one possible option for the timezone it finds, so get_zone and get_zonecode will be mostly right.
The get_offset function actually works.

Tests written?

No

Commits signed with GPG?

Yes

@salt-jenkins salt-jenkins requested a review from saltstack/team-windows May 23, 2018

@dwoz

dwoz approved these changes May 23, 2018

@rallytime

This comment has been minimized.

Contributor

rallytime commented May 23, 2018

@twangboy The test_win_timezone tests are failing with this change.

https://jenkins.saltstack.com/job/PR/job/salt-pr-linode-ubuntu14-n/23102/

twangboy added some commits May 23, 2018

@rallytime rallytime requested a review from terminalmage May 24, 2018

@@ -6,590 +6,387 @@
# Import Python libs
import logging
import re
import pytz

This comment has been minimized.

@terminalmage

terminalmage May 25, 2018

Member

This is not a hard dep for Salt, is it? If not, then this will cause a lot of tracebacks on non-Windows boxes which don't have pytz installed, unless we gate this in a try/except to catch the ImportError.

Also, we'd need to make sure that we add pytz to the freezer_includes or whatever we use now when making the Windows release.

What about the gist I posted in this comment? It seems like there was a way to get this to work using a different flag for tzutil. That would be the optimal solution as it would not require us to dep on another module.

This comment has been minimized.

@twangboy

twangboy May 25, 2018

Contributor

Looks like it gets installed in windows as a dependency to something we put in the requirements file. I'll put it in a try/except and __virtual__

I'll add to freezer includes. That's in setup.py, right?

The gist you referred to looked like it was referring to a linux box. The tzutil on Windows has no additional switches for offsets and such:

C:\dev\salt>tzutil /?
Windows Time Zone Utility

Usage:
TZUTIL </? | /g | /s TimeZoneID[_dstoff] | /l>

Parameters:
    /? Displays usage information.

    /g Displays the current time zone ID.

    /s TimeZoneID[_dstoff]
       Sets the current time zone using the specified time zone ID.
       The _dstoff suffix disables Daylight Saving Time adjustments
       for the time zone (where applicable).

    /l Lists all valid time zone IDs and display names. The output will
       be:
           <display name>
           <time zone ID>

twangboy added some commits May 25, 2018

Move pytz to 3rd party import, add to __virtual__
Add pytz to freezer includes

@rallytime rallytime requested a review from terminalmage May 25, 2018

@terminalmage

A couple changes need to be made to the release notes you added.

@twangboy To respond to this comment, if you read the gist closely it talks about having gotten the CLDR time zone information (that's the format with the offsets) from tzutil on Windows. This can apparently be done with the /l flag. Did you try that?

Having said all this though, pytz is better suited to this sort of thing, and as long as it is A) only used on Windows, and B) already included in the embedded Python we distribute with Windows, then we should use pytz instead of trying to parse tzutil.

@@ -0,0 +1,16 @@
===========================
In Progress: Salt 2018.3.2 Release Notes
===========================

This comment has been minimized.

@terminalmage

terminalmage May 30, 2018

Member

The overline and underline must match the length of the title line to avoid Sphinx warnings.

This comment has been minimized.

@twangboy

twangboy May 30, 2018

Contributor

Sorry, just copied the file from 2018.3.1 and changed the 1 to a 2. Shall I go through and fix all of them?

This comment has been minimized.

@twangboy

twangboy May 30, 2018

Contributor

I think it's because the In Progress: eventually gets removed when they're ready for release.

This release is still in progress and has not been released yet.
Changes to win_timezone
-----------------------

This comment has been minimized.

@terminalmage

terminalmage May 30, 2018

Member

Please change this underline to use ==== instead of ----, to match the pending changes to the changelog generator. The order of use of different underline chars affects the section names headings generated by Sphinx.

@twangboy

This comment has been minimized.

Contributor

twangboy commented May 30, 2018

@terminalmage I did look at the gist. The /l option returns a list of all Windows Timezones with their offset. The /g option returns the name of the timezone without the offset. The gist you're referring to just creates a dictionary you can use to marry the Windows timezone to the iana standard in a 1 to 1 relationship... which is what we're doing in this PR. Though I got the dictionary from somewhere else.

@terminalmage

This comment has been minimized.

Member

terminalmage commented May 31, 2018

Yeah, I know what the gist did... My original point was that we could have perhaps used tzutil to get a more accurate timezone mapping than we were already using. But given what I mentioned in my last comment, pytz is definitely a better solution.

@rallytime rallytime merged commit b095969 into saltstack:2018.3 May 31, 2018

6 of 9 checks passed

jenkins/PR/salt-pr-linode-cent7-py3 Pull Requests » Salt PR - Linode CentOS 7 - PY3 #5396 — ABORTED
Details
jenkins/PR/salt-pr-rs-cent7-n Pull Requests » Salt PR - RS CentOS 7 #19453 — ABORTED
Details
default Build started sha1 is merged.
Details
WIP ready for review
Details
jenkins/PR/salt-pr-clone Pull Requests » Salt PR - Clone #25593 — SUCCESS
Details
jenkins/PR/salt-pr-docs-n Pull Requests » Salt PR - Docs #17665 — SUCCESS
Details
jenkins/PR/salt-pr-linode-ubuntu14-n Pull Requests » Salt PR - Linode Ubuntu14.04 #23331 — SUCCESS
Details
jenkins/PR/salt-pr-linode-ubuntu16-py3 Pull Requests » Salt PR - Linode Ubuntu16.04 - PY3 #10369 — SUCCESS
Details
jenkins/PR/salt-pr-lint-n Pull Requests » Salt PR - Code Lint #22295 — SUCCESS
Details

@twangboy twangboy deleted the twangboy:fix_47559 branch May 31, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment