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

Add timzeone windows integration tests and fix get_zone #48788

Merged
merged 4 commits into from Jul 27, 2018

Conversation

Projects
None yet
4 participants
@Ch3LL
Contributor

Ch3LL commented Jul 26, 2018

What does this PR do?

Adds timezone integration tests for windows and fixes an issue found while writing the tests.

Here is the issue:

PS C:\testing> salt-call --local timezone.set_zone 'America/Denver' -lerror
local:
    True
PS C:\testing> salt-call --local timezone.get_zone
[INFO    ] Executing command ['tzutil', '/g'] in directory 'C:\Users\Administrator'
local:
    America/Inuvik
PS C:\testing>

I believe this occurs because the mapping here: https://github.com/saltstack/salt/blob/v2017.7.7/salt/modules/win_timezone.py#L114-L121

has multiple 'Mountain Standard Times'

I changed it to use tzlocal but i'm not certain this is the best way to grab the windows timezone accurately and I am definitely up for other ideas on how to approach this on the windows side of things and was hoping to get feedback from @dwoz and @twangboy

What issues does this PR fix or reference?

#45007

@salt-jenkins salt-jenkins requested a review from saltstack/team-windows Jul 26, 2018

@rallytime

This comment has been minimized.

Contributor

rallytime commented Jul 26, 2018

@Ch3LL You've got a lint error here: https://jenkinsci.saltstack.com/job/pr-lint/job/PR-48788/1/warnings52Result/file.-580256476/

Don't worry about the one in the file you didn't change. That's fixed in another PR.

@@ -9,6 +9,12 @@
import logging
import re
try:
import tzlocal

This comment has been minimized.

@rares-pop

rares-pop Jul 27, 2018

Contributor

we seem to align on doing this like:
try:
import tzlocal
except ImportError:
tzlocal = None

And then instead of HAS_TZLOCAL just do - if tzlocal is not None:

This comment has been minimized.

@Ch3LL

Ch3LL Jul 27, 2018

Contributor

i see many instances in the code of using HAS_* in the same manner. Do you see an advantage to using None instead?

This comment has been minimized.

@Ch3LL

Ch3LL Jul 27, 2018

Contributor

well it seems to get rid of the pylint error. @rallytime do we have a standard for this? should i be using what @rares-pop recommended or just adding a pylint exception: # pylint: disable=import-error

This comment has been minimized.

@rares-pop

rares-pop Jul 27, 2018

Contributor

I had to impression that this is the new way of doing it.
I have seen @isbm recommending that in some of the PRs he reviewed.

The only advantage I've seen is one less variable. Maybe @isbm can give his insight on this?

This comment has been minimized.

@rallytime

rallytime Jul 27, 2018

Contributor

We do not have a standard way of doing it, and I am fine with either way. It's really just a preference at this point.

Ch3LL added some commits Jul 27, 2018

@rallytime rallytime merged commit 2e00939 into saltstack:2017.7 Jul 27, 2018

4 of 8 checks passed

jenkins/pr/py2-centos-7 running py2-centos-7...
Details
jenkins/pr/py2-ubuntu-1604 running py2-ubuntu-1604...
Details
jenkins/pr/py3-centos-7 running py3-centos-7...
Details
jenkins/pr/py3-ubuntu-1604 running py3-ubuntu-1604...
Details
WIP ready for review
Details
continuous-integration/jenkins/pr-merge This commit looks good
Details
jenkins/pr/docs The docs job has passed
Details
jenkins/pr/lint The lint job has passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment