Use UTC timezone #569

charlesvdv · 2016-12-30T09:36:51Z

Fix the issue #303 and follow up of #439 PR.

I'm not sure about the spot to put the sls.common tests.

This change is

highfive · 2016-12-30T09:36:54Z

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @aneeshusa (or someone else) soon.

charlesvdv · 2016-12-30T09:56:19Z

Looks like timezone is not supported for macOS : saltstack/salt#31345

aneeshusa

Looks pretty good! Another change - in .travis.yml, each builder that invokes the test suite should get language: python and python: 3.5.

aneeshusa · 2016-12-31T22:09:33Z

.travis/dispatch.sh

@@ -72,4 +72,5 @@ else
    if [[ "${SALT_NODE_ID}" == "servo-master1" ]]; then
        ./test.py sls.buildbot.master sls.homu sls.nginx
    fi
+    ./test.py sls.common


We should only run this on the non-macOS machines for now, so we can check the ${SALT_NODE_ID} isn't a macOS node; easiest way is probably using the =~ bash operator.

aneeshusa · 2016-12-31T22:09:33Z

common/init.sls

@@ -45,6 +45,9 @@ servo:
    - shell: /bin/bash
    - home: {{ common.servo_home }}

+UTC:


Use Jinja to render this state conditionally, only on non-macOS, using an {% if ... %} block.

aneeshusa · 2016-12-31T22:09:33Z

tests/sls/common/timezone.py

+
+def run():
+    command = 'date'
+    ret = subprocess.run(command,


Inline the command and pass as an array so we control word-splitting, etc.

subprocess.run(['date'], ...)

Also, prefer to use block indent:

ret = subprocess.run( ['date'], stdout=subprocess.PIPE, stderr=subprocess.PIPE )

aneeshusa

One last nit, everything else LGTM, so go ahead and squash while addressing this last comment.

aneeshusa · 2017-01-01T23:11:59Z

.travis/dispatch.sh

+
+    # Salt doesn't support timezone.system on OSX
+    # See https://github.com/saltstack/salt/issues/31345
+    if [[ "${TRAVIS_OS_NAME}" != "osx" ]]; then


In the future the list of tests to run will be auto-generated from the Salt top file, so I'd prefer to check ${SALT_NODE_ID} instead of ${TRAVIS_OS_NAME}. You can use e.g.

if [[ ! "${SALT_NODE_ID}" =~ servo-mac* ]]; then ... fi

Sorry about that. I thought it would be less error-prone (because the servo-mac* could change at some point) but if it needs to be travis independant I totally get that.

aneeshusa

Thought of an optional enhancement for bonus points, or else you can squash the current commits which LGTM.

aneeshusa · 2017-01-02T21:48:12Z

tests/sls/common/timezone.py

+    stdout = ret.stdout.decode('utf-8')
+
+    if ret.returncode == 0 and 'UTC' in stdout:
+        return Success('Date is in UTC')


I was reading an article about testing yesterday which reminded me of expected-failure tests, which is IMO a nicer way to handle the macOS situtation. Roughly, instead of just returning Success here, use sys.platform to check if we're on macOS, and if so, return Failure in that case with an appropriate message; and vice-versa for the Failure. This inverts the expectations and lets us run the sls.common test unconditionally.

Note that in the .travis.yml file, I believe python isn't supported on macOS, so don't update those entries to add the python3.5 spec. However, we may need to install Python 3.5 manually with brew on the OS X builders; make a separate commit and see if it's necessary before adding that.

It may be a little tricky to add so I'll make it optional, for bonus points :)

Should I put the brew python install inside a saltscript or inside the .travis/install...sh ?

Let's not add the brew install unless the test suite fails to run on macOS on Travis (i.e. check first), but if we need it I'd probably put it in the dispatch script before we print the Python version.

aneeshusa · 2017-01-02T22:14:21Z

If you go for the bonus points, in the .travis/dispatch.sh script please also print the Python3 version unconditionally before running the test suite instead of just for the test node.

aneeshusa · 2017-01-06T05:32:33Z

I believe installing Python 3.5 on the Mac builders actually takes a bit of time, so let's skip the xfail stuff for now. Everything else looked good, so discard the last commit and squash the first five, and we'll get this landed.

aneeshusa · 2017-01-06T05:32:52Z

Also, please rebase on top of the latest master to pull in #573.


        Use UTC timezone

aneeshusa · 2017-01-07T08:16:33Z

Thanks for the patch @charlesvdv! @bors-servo r+

bors-servo · 2017-01-07T08:16:34Z

📌 Commit 751f12e has been approved by aneeshusa

bors-servo · 2017-01-07T11:35:09Z

⚡ Test exempted - status


        Auto merge of #569 - charlesvdv:utc, r=aneeshusa

Use UTC timezone Fix the issue #303 and follow up of #439 PR. I'm not sure about the spot to put the ```sls.common``` tests.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/saltfs/569)

highfive assigned aneeshusa Dec 30, 2016

highfive added the S-awaiting-review label Dec 30, 2016

charlesvdv force-pushed the charlesvdv:utc branch from 4cdfad7 to da38056 Dec 30, 2016

aneeshusa suggested changes Dec 31, 2016

View changes

aneeshusa added S-needs-code-changes and removed S-awaiting-review labels Dec 31, 2016

highfive added S-awaiting-review and removed S-needs-code-changes labels Jan 1, 2017

charlesvdv force-pushed the charlesvdv:utc branch 2 times, most recently from 73a8df0 to 4d60976 Jan 1, 2017

aneeshusa suggested changes Jan 1, 2017

View changes

aneeshusa added S-needs-code-changes and removed S-awaiting-review labels Jan 1, 2017

charlesvdv force-pushed the charlesvdv:utc branch from 4d60976 to e809630 Jan 2, 2017

highfive added S-awaiting-review and removed S-needs-code-changes labels Jan 2, 2017

aneeshusa reviewed Jan 2, 2017

View changes

aneeshusa added S-needs-code-changes and removed S-awaiting-review labels Jan 2, 2017

highfive added S-awaiting-review and removed S-needs-code-changes labels Jan 3, 2017

charlesvdv force-pushed the charlesvdv:utc branch 2 times, most recently from 83f74e2 to c2f2054 Jan 3, 2017

aneeshusa added S-needs-code-changes and removed S-awaiting-review labels Jan 6, 2017

charlesvdv force-pushed the charlesvdv:utc branch from c2f2054 to dcc4cb2 Jan 6, 2017

highfive added S-awaiting-review and removed S-needs-code-changes labels Jan 6, 2017

charlesvdv force-pushed the charlesvdv:utc branch from dcc4cb2 to 751f12e Jan 6, 2017

Use UTC timezone

Loading status checks…

751f12e

highfive added S-awaiting-merge and removed S-awaiting-review labels Jan 7, 2017

aneeshusa approved these changes Jan 7, 2017

View changes

bors-servo merged commit 751f12e into servo:master Jan 7, 2017
2 checks passed

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

homu Test exempted
Details

highfive removed the S-awaiting-merge label Jan 7, 2017

bors-servo mentioned this pull request Jan 7, 2017

Changed IPs to Hostnames where possible, and removed host.present tes… #369

Closed

jdm mentioned this pull request Jan 19, 2017

Use UTC Timezone #303

Closed

servo / saltfs

Use UTC timezone #569

Use UTC timezone #569

charlesvdv commented Dec 30, 2016 •

edited by larsbergstrom

highfive commented Dec 30, 2016

charlesvdv commented Dec 30, 2016

aneeshusa left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

aneeshusa left a comment

This comment has been minimized.

This comment has been minimized.

aneeshusa left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

aneeshusa commented Jan 2, 2017

aneeshusa commented Jan 6, 2017

aneeshusa commented Jan 6, 2017

aneeshusa commented Jan 7, 2017

bors-servo commented Jan 7, 2017

bors-servo commented Jan 7, 2017

servo / saltfs

Join GitHub today

Use UTC timezone #569

Use UTC timezone #569

Conversation

charlesvdv commented Dec 30, 2016 • edited by larsbergstrom

highfive commented Dec 30, 2016

charlesvdv commented Dec 30, 2016

aneeshusa left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

aneeshusa left a comment

This comment has been minimized.

This comment has been minimized.

aneeshusa left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

aneeshusa commented Jan 2, 2017

aneeshusa commented Jan 6, 2017

aneeshusa commented Jan 6, 2017

aneeshusa commented Jan 7, 2017

bors-servo commented Jan 7, 2017

bors-servo commented Jan 7, 2017

charlesvdv commented Dec 30, 2016 •

edited by larsbergstrom