#2424 Fix time formatting for timezone values
Merged 3 years ago by tkopecek. Opened 3 years ago by tkopecek.
tkopecek/koji issue2423  into  master

file modified
+2 -2
@@ -5275,8 +5275,8 @@ 

          if target is not None:

              dest_tag = target['dest_tag_name']

          status = koji.BUILD_STATES[build['state']].lower()

-         creation_time = koji.formatTimeLong(build['creation_time'])

-         completion_time = koji.formatTimeLong(build['completion_time'])

+         creation_time = koji.formatTimeLong(build['creation_ts'])

+         completion_time = koji.formatTimeLong(build['completion_ts'])

          task_id = build['task_id']

  

          task_data = self._getTaskData(task_id)

file modified
+2 -2
@@ -3225,7 +3225,7 @@ 

              print("Task: %s %s" % (task['id'], koji.taskLabel(task)))

          else:

              print("Task: none")

-         print("Finished: %s" % koji.formatTimeLong(info['completion_time']))

+         print("Finished: %s" % koji.formatTimeLong(info['completion_ts']))

          maven_info = session.getMavenBuild(info['id'])

          if maven_info:

              print("Maven groupId: %s" % maven_info['group_id'])
@@ -4729,7 +4729,7 @@ 

          oldrepo = params[2]

          if oldrepo:

              lines.append("Old Repo ID: %i" % oldrepo['id'])

-             lines.append("Old Repo Creation: %s" % koji.formatTimeLong(oldrepo['creation_time']))

+             lines.append("Old Repo Creation: %s" % koji.formatTimeLong(oldrepo['create_ts']))

          if len(params) > 3:

              lines.append("External Repos: %s" %

                           ', '.join([ext['external_repo_name'] for ext in params[3]]))

file modified
+18 -5
@@ -50,6 +50,7 @@ 

  import xml.sax.handler

  from fnmatch import fnmatch

  

+ import dateutil.parser

  import requests

  import six

  import six.moves.configparser
@@ -3256,10 +3257,12 @@ 

  

  def formatTime(value):

      """Format a timestamp so it looks nicer"""

-     if not value:

+     if not value and not isinstance(value, (int, float)):

          return ''

      if isinstance(value, xmlrpc_client.DateTime):

          value = datetime.datetime.strptime(value.value, "%Y%m%dT%H:%M:%S")

+     elif isinstance(value, (int, float)):

+         value = datetime.datetime.fromtimestamp(value)

      if isinstance(value, datetime.datetime):

          return value.strftime('%Y-%m-%d %H:%M:%S')

      else:
@@ -3275,12 +3278,22 @@ 

      """Format a timestamp to a more human-reable format, i.e.:

      Sat, 07 Sep 2002 00:00:01 GMT

      """

-     if not value:

+     if not value and not isinstance(value, (int, float)):

          return ''

+     if isinstance(value, six.string_types):

+         t = dateutil.parser.parse(value)

+     elif isinstance(value, xmlrpc_client.DateTime):

+         t = dateutil.parser.parse(value.value)

+     elif isinstance(value, (int, float)):

+         t = datetime.datetime.fromtimestamp(value)

      else:

-         # Assume the string value passed in is the local time

-         localtime = time.mktime(time.strptime(formatTime(value), '%Y-%m-%d %H:%M:%S'))

-         return time.strftime('%a, %d %b %Y %H:%M:%S %Z', time.localtime(localtime))

+         t = value

+     # return date in local timezone, py 2.6 has tzone as astimezone required parameter

+     # would work simply as t.astimezone() for py 2.7+

+     if t.tzinfo is None:

+         t = t.replace(tzinfo=dateutil.tz.gettz())

+     t = t.astimezone(dateutil.tz.gettz())

+     return datetime.datetime.strftime(t, '%a, %d %b %Y %H:%M:%S %Z')

  

  

  def buildLabel(buildInfo, showEpoch=False):

@@ -11,8 +11,8 @@ 

  Status: complete

  Built by: user

  ID: 612609

- Started: Wed, 18 Feb 2015 14:50:37 EST

- Finished: Wed, 18 Feb 2015 14:57:37 EST

+ Started: Wed, 18 Feb 2015 09:50:37 EST

+ Finished: Wed, 18 Feb 2015 09:57:37 EST

  Changelog:

  * Wed Feb 18 2015 Happy Koji User <user@example.com> - 1:0.3.0-0.2.M1

  - Unbundle ASM

@@ -59,6 +59,7 @@ 

  class TestBuildNotification(unittest.TestCase):

  

      def setUp(self):

+         self.maxDiff = None

          self.original_timezone = os.environ.get('TZ')

          os.environ['TZ'] = 'US/Eastern'

          time.tzset()
@@ -111,5 +112,5 @@ 

              msg_expect = fp.read()

          if six.PY2:

              msg_expect = msg_expect.decode()

-         self.assertEqual(message, msg_expect)

+         self.assertMultiLineEqual(message.decode(), msg_expect.decode())

          locale.resetlocale()

@@ -218,7 +218,7 @@ 

          self.__run_parseTask_test('prepRepo', params, expect)

  

      def test_createRepo(self):

-         params = [1, 'x86_64', {'id': 1, 'creation_time': '1970-1-1 0:0:0'},

+         params = [1, 'x86_64', {'id': 1, 'create_ts': 0},

                    [{'external_repo_name': 'fedoraproject.net'},

                     {'external_repo_name': 'centos.org'}]]

          expect = ["Repo ID: %i" % params[0]]

@@ -1,5 +1,7 @@ 

  from __future__ import absolute_import

  import datetime

+ import os

+ import time

  import locale

  try:

      import unittest2 as unittest
@@ -11,6 +13,16 @@ 

  from koji import formatTime, formatTimeLong

  

  class TestFormatTime(unittest.TestCase):

+     def setUp(self):

+         self._orig_tz = os.environ.get('TZ')

+ 

+     def tearDown(self):

+         if self._orig_tz:

+             os.environ['TZ'] = self._orig_tz

+         elif 'TZ' in os.environ:

+             del os.environ['TZ']

+         time.tzset()

+ 

      def test_format_time(self):

          self.assertEqual(formatTime(None), '')

          self.assertEqual(formatTime(''), '')
@@ -33,6 +45,8 @@ 

      def test_format_time_long(self):

          # force locale to compare 'desired' value

          locale.setlocale(locale.LC_ALL, ('en_US', 'UTF-8'))

+         os.environ['TZ'] = 'GMT'

+         time.tzset()

  

          self.assertEqual(formatTimeLong(None), '')

          self.assertEqual(formatTimeLong(''), '')
@@ -62,4 +76,41 @@ 

          r = r[:r.rfind(' ')]

          self.assertEqual(r, desired)

  

+         # str + timezone

+         d3 = '2017-10-05 09:52:31+02:00'

+         desired = 'Thu, 05 Oct 2017 07:52:31 GMT'

+         os.environ['TZ'] = 'GMT'

+         time.tzset()

+         r = formatTimeLong(d3)

+         self.assertEqual(r, desired)

+ 

+         # non-GMT without DST

+         d3 = '2017-06-05 09:52:31+02:00'

+         desired = 'Mon, 05 Jun 2017 09:52:31 CEST'

+         os.environ['TZ'] = 'Europe/Prague'

+         time.tzset()

+         r = formatTimeLong(d3)

+         self.assertEqual(r, desired)

+ 

+         # non-GMT with DST

+         d3 = '2017-12-05 09:52:31+02:00'

+         desired = 'Tue, 05 Dec 2017 08:52:31 CET'

+         os.environ['TZ'] = 'Europe/Prague'

+         time.tzset()

+         r = formatTimeLong(d3)

+         self.assertEqual(r, desired)

+ 

+         # timestamps, local timezone

+         d4 = 0

+         desired = 'Thu, 01 Jan 1970 01:00:00 CET'

+         r = formatTimeLong(d4)

+         self.assertEqual(r, desired)

+ 

+         # timestamps, GMT

+         desired = 'Thu, 01 Jan 1970 00:00:00 GMT'

+         os.environ['TZ'] = 'GMT'

+         time.tzset()

+         r = formatTimeLong(d4)

+         self.assertEqual(r, desired)

+ 

          locale.resetlocale()

file modified
+2 -2
@@ -69,7 +69,7 @@ 

        <td>$build.volume_name</td>

      </tr>

      <tr>

-       <th>Started</th><td>$util.formatTimeLong($start_time)</td>

+       <th>Started</th><td>$util.formatTimeLong($start_ts)</td>

      </tr>

      #if $build.state == $koji.BUILD_STATES.BUILDING

      #if $estCompletion
@@ -79,7 +79,7 @@ 

      #end if

      #else

      <tr>

-       <th>Completed</th><td>$util.formatTimeLong($build.completion_time)</td>

+       <th>Completed</th><td>$util.formatTimeLong($build.completion_ts)</td>

      </tr>

      #end if

      #if $build.cg_id

file modified
+1 -1
@@ -127,7 +127,7 @@ 

            <td><a href="taginfo?tagID=$build.tag_id">$build.tag_name</a></td>

            #end if

            <td class="user-$build.owner_name"><a href="userinfo?userID=$build.owner_id">$build.owner_name</a></td>

-           <td>$util.formatTime($build.completion_time)</td>

+           <td>$util.formatTime($build.completion_ts)</td>

            #set $stateName = $util.stateName($build.state)

            <td class="$stateName">$util.stateImage($build.state)</td>

          </tr>

file modified
+1 -1
@@ -23,7 +23,7 @@ 

      </tr>

      #if 'mtime' in $file and $file.mtime

      <tr>

-       <th>Modification time</th><td>$util.formatTimeLong($datetime.datetime.fromtimestamp($file.mtime))</td>

+       <th>Modification time</th><td>$util.formatTimeLong($file.mtime)</td>

      </tr>

      #end if

      #if 'user' in $file and $file.user

file modified
+2 -2
@@ -24,7 +24,7 @@ 

        #if not $user

        <td class="user-$build.owner_name"><a href="userinfo?userID=$build.owner_id">$build.owner_name</a></td>

        #end if

-       <td>$util.formatTime($build.completion_time)</td>

+       <td>$util.formatTime($build.completion_ts)</td>

        <td class="$stateName">$util.stateImage($build.state)</td>

      </tr>

      #end for
@@ -65,7 +65,7 @@ 

        </td>

        #end if

        <td>$task.arch</td>

-       <td>$util.formatTime($task.completion_time)</td>

+       <td>$util.formatTime($task.completion_ts)</td>

        <td class="task$state">$util.imageTag($state)</td>

      </tr>

      #end for

file modified
+2 -2
@@ -1276,12 +1276,12 @@ 

          if field not in values:

              values[field] = None

  

-     values['start_time'] = build.get('start_time') or build['creation_time']

+     values['start_ts'] = build.get('start_ts') or build['creation_ts']

      # the build start time is not accurate for maven and win builds, get it from the

      # task start time instead

      if 'maven' in typeinfo or 'win' in typeinfo:

          if task:

-             values['start_time'] = task['start_time']

+             values['start_ts'] = task['start_ts']

      if build['state'] == koji.BUILD_STATES['BUILDING']:

          avgDuration = server.getAverageBuildDuration(build['package_id'])

          if avgDuration is not None:

@@ -48,7 +48,7 @@ 

            <tr class="$util.rowToggle($self)">

              <td><a href="buildinfo?buildID=$build.build_id">$build.nvr</a></td>

              <td class="user-$build.owner_name"><a href="userinfo?userID=$build.owner_id">$build.owner_name</a></td>

-             <td>$util.formatTime($build.completion_time)</td>

+             <td>$util.formatTime($build.completion_ts)</td>

              #set $stateName = $util.stateName($build.state)

              <td class="$stateName">$util.stateImage($build.state)</td>

            </tr>

@@ -43,7 +43,7 @@ 

        <title>$koji.BUILD_STATES[$build.state].lower(): $koji.buildLabel($build)#if $build.task then ', target: ' + $build.task.request[1] else ''#</title>

        <link>$weburl/buildinfo?buildID=$build.build_id</link>

        #if $build.completion_time

-       <pubDate>$util.formatTimeRSS($build.completion_time)</pubDate>

+       <pubDate>$util.formatTimeRSS($build.completion_ts)</pubDate>

        #end if

        #if $build.state == $koji.BUILD_STATES['COMPLETE'] and $build.changelog

        <description>&lt;pre&gt;$util.escapeHTML($koji.util.formatChangelog($build.changelog))&lt;/pre&gt;</description>

file modified
+1 -1
@@ -11,7 +11,7 @@ 

    <tr><th>Tag</th><td><a href="taginfo?tagID=$repo.tag_id">$repo.tag_name</a></td></tr>

    #set $state = $util.repoState($repo.state)

    <tr><th>State</th><td class="repo$state">$state</td></tr>

-   <tr><th>Event</th><td>$repo.create_event ($util.formatTimeLong($repo.creation_time))</td></tr>

+   <tr><th>Event</th><td>$repo.create_event ($util.formatTimeLong($repo.create_ts))</td></tr>

    #if $repo.state != koji.REPO_STATES['DELETED']

    <tr><th>URL</th><td><a href="$url">repodata</a></td></tr>

    <tr><th>Repo json</th><td><a href="$repo_json">repo.json</a></td></tr>

file modified
+1 -1
@@ -115,7 +115,7 @@ 

        <th>Repo&nbsp;created</th>

        <td>

        #if $repo

-         <a href="repoinfo?repoID=$repo.id">$util.formatTimeRSS($repo.creation_time)</a>

+         <a href="repoinfo?repoID=$repo.id">$util.formatTimeRSS($repo.create_ts)</a>

        #end if

        </td>

      </tr>

file modified
+3 -3
@@ -77,11 +77,11 @@ 

      #end for

      #end if

      <tr>

-       <th>Created</th><td>$util.formatTimeLong($task.create_time)</td>

+       <th>Created</th><td>$util.formatTimeLong($task.create_ts)</td>

      </tr>

      #if $task.start_time

      <tr>

-       <th>Started</th><td>$util.formatTimeLong($task.start_time)</td>

+       <th>Started</th><td>$util.formatTimeLong($task.start_ts)</td>

      #end if

      #set $end_ts = None

      #if $task.state == $koji.TASK_STATES.OPEN
@@ -93,7 +93,7 @@ 

      #end if

      #elif $task.completion_time

      <tr>

-       <th>Completed</th><td>$util.formatTimeLong($task.completion_time)</td>

+       <th>Completed</th><td>$util.formatTimeLong($task.completion_ts)</td>

      </tr>

      #set $end_ts = $task.completion_ts

      #end if

@@ -195,7 +195,7 @@ 

  #set $oldrepo = $params[2]

  #if $oldrepo

      <strong>Old Repo ID:</strong> <a href="repoinfo?repoID=$oldrepo.id">$oldrepo.id</a><br/>

-     <strong>Old Repo Creation:</strong> $koji.formatTimeLong($oldrepo.creation_time)<br/>

+     <strong>Old Repo Creation:</strong> $koji.formatTimeLong($oldrepo.create_ts)<br/>

  #end if

  #if $len($params) > 4 and $params[4]

      <strong>External Repos:</strong> $printValue(None, [ext['external_repo_name'] for ext in $params[3]])<br/>

file modified
+1 -1
@@ -153,7 +153,7 @@ 

              #end if

            </td>

            <td>$task.arch</td>

-           <td>$util.formatTime($task.completion_time)</td>

+           <td>$util.formatTime($task.completion_ts)</td>

            <td class="task$state">$util.imageTag($taskState)</td>

          </tr>

        #end for

file modified
+1 -1
@@ -93,7 +93,7 @@ 

            <tr class="$util.rowToggle($self)">

              #set $stateName = $util.stateName($build.state)

              <td><a href="buildinfo?buildID=$build.build_id">$build.nvr</a></td>

-             <td>$util.formatTime($build.completion_time)</td>

+             <td>$util.formatTime($build.completion_ts)</td>

              <td class="$stateName">$util.stateImage($build.state)</td>

            </tr>

            #end for

Metadata Update from @tkopecek:
- Pull-request tagged with: testing-ready

3 years ago

1 new commit added

  • fix older python behaviour
3 years ago

1 new commit added

  • fix timezone for rhel7
3 years ago

1 new commit added

  • fix timezone for rhel7
3 years ago

broken for now (doesn't work on py 2.6)

Metadata Update from @tkopecek:
- Pull-request untagged with: testing-ready

3 years ago

rebased onto 8617a8600f7f34b1d4cf1219a436a9d197a4cbc9

3 years ago

rebased onto a25b3ffed3515a4a5f549e83d709588877375039

3 years ago

rebased onto b9a07496631bd7173c8c7450b691adb19acaa73e

3 years ago

rebased onto be9a1908b5e57350eab00277fac9b779680ecd04

3 years ago

rebased onto 90e34402cb3b685d0a4bf1899df30f0418a84e80

3 years ago

Fixes on py3 at least

rebased onto c00a81a8e9cde72cd5d25512267a38d88c1aa55d

3 years ago

First thought...

At one point very early on, the hub was only returning times as string fields direct from the db, and we had code like formatTimeLong() to help out on the client side. Later we realized what a pain that was, so we we added more return fields like creation_ts to complement creation_time, etc.

It would probably make sense to go through the places in our client code where we consume those text timestamp fields and change them to consume the corresponding *_ts field instead.

It would probably also make sense to have the formatTime and formatTime functions handle numeric timestamps as well. This would make the above change fairly straightforward.

rebased onto 03e5c1cb9b18b9dbfad7bf8a2585cdb69735d8df

3 years ago

rebased onto 2cfad1d

3 years ago

The above would sidestep this issue for our own clients, but any other clients doing the same thing would still suffer, so we should still do something there. The original version of formatTimeLong only handled a single rigid format. The tz change we introduced should only result in some limited formats, so perhaps we can do something more manual as a fallback for 2.6?

I've resigned to solve it with datetime/time modules and used dateutil which is alredy in requires. I previously thought that we've it only in CLI, but it is in base library.

still, no need for the clients to parse dates when the hub is returning numeric timestamp fields

Yes, maybe another issue for finding all those places for 1.23?

Sure, we can separate that out

It is the same for web UI.

Metadata Update from @tkopecek:
- Pull-request tagged with: testing-ready

3 years ago

Note the cli can also hit this, so we'll continue to see users with old clients running into this when running buildinfo.

I've always considered the *_ts fields to be the preferred ones for code needing to read the dates as dates, with the *_time simply being human readable text, but still I wonder if this is an issue we need to fix on the hub for api compatibility.

1 new commit added

  • formatTime/Long can handle timestamp
3 years ago

I've added timestamp handling to formatting functions and replace *_time in most places. (Myabe we can add _ts fields to buildroot repo_create_event_time + retire_event_time

1 new commit added

  • fix start_ts
3 years ago

Metadata Update from @jcupova:
- Pull-request tagged with: testing-done

3 years ago

1 new commit added

  • fix tests
3 years ago

Metadata Update from @tkopecek:
- Pull-request untagged with: testing-done, testing-ready

3 years ago

1 new commit added

  • fix create_ts for repos
3 years ago

Metadata Update from @tkopecek:
- Pull-request tagged with: testing-ready

3 years ago

1 new commit added

  • fix test
3 years ago

Maybe we can add _ts fields to buildroot repo_create_event_time + retire_event_time

I think we should look into something like that, but not for this issue. I wouldn't simply add fields with those names though. For the repos, I think we need to track:

  • the event that the repo is generated from
  • the time the repo was created
  • the time the repo was retired

The first is decidedly an event reference, but the original field name of create_event is inaccurate.

The latter two are timestamps primarily, and could possibly not be events at all (though they could be).

So we could just add two timestamp field as you say, but the naming is confusing. We could rename things a bit to be more accurate, but we'd need to be careful about api_compatibility.

At any rate, this is way outside the scope of this issue. If we want to follow up on this, we should have a separate issue for it.

Commit b104e7a fixes this pull-request

Pull-Request has been merged by tkopecek

3 years ago

Metadata Update from @jcupova:
- Pull-request tagged with: testing-done

3 years ago
Metadata