----- Original Message ----- > From: "Daniel Axtens" <d...@axtens.net> > To: vkaba...@redhat.com, patchwork@lists.ozlabs.org > Sent: Saturday, February 24, 2018 2:58:56 AM > Subject: Re: [PATCH v3] Avoid timezone confusion > > Hi Veronika, > > I am testing this now and I think I'm happy with it. > > However, I set up a container in UTC+11, and while watching the events > api endpoint: > - I added some patches without this patch > - I applied this patch and migrated > - I added some more patches > - I noticed that the new events were no longer added to the front of > the events list; they were now 11 hours back. > > This is, on reflection, a straight-forward consequence of moving from > UTC+11 to UTC without attempting to migrate old data. > > I still think migration is an incredibly difficult and error-prone waste > of time, so I still don't want to go down that road. What I do want is a > piece of documentation in the release notes to warn people that if you > are in UTC+n, your events feed will be out of order for n hours. >
Yes, the "delayed" events are expected. Additional documentation about it seems fair. > I don't know of any users of the event feed at the moment, so I'm not > worried about breaking anything: I just want a couple of sentences so > that any sysadmins/users who are especially observant don't freak out > that they're suddenly losing events. > We tried it but the events API was very slow and unreliable so we switched back to polling series. I believe Stephen worked on fixing up the API so hopefully there will be a working solution merged soon :) > You can do a respin of this, or just send a new patch on top of this and > I will apply both at the same time. > I'll send an additional patch with releasnote changes and cc you so you can find it easily. > Regards, > Daniel > > vkaba...@redhat.com writes: > > > From: Veronika Kabatova <vkaba...@redhat.com> > > > > Patchwork saves patches, comments etc with UTC timezone and reports > > this time when opening the patch details. However, internally generated > > processes such as events are reported with the instance's local time. > > There's nothing wrong with that and making PW timezone-aware would add > > useless complexity, but in a world-wide collaboration a lot of confusion > > may arise as the timezone is not reported at all. Instance's local time > > might be very different from the local time of CI integrating with PW, > > which is different from the local time of person dealing with it etc. > > > > Use UTC everywhere by default instead of UTC for sumbissions and local > > timezone for internally generated events (which is not reported). > > > > Signed-off-by: Veronika Kabatova <vkaba...@redhat.com> > > --- > > Changes: Implement Daniel's idea about using datetime.datetime.utcnow > > --- > > docs/api/rest.rst | 3 +- > > patchwork/migrations/0024_timezone_unify.py | 46 > > ++++++++++++++++++++++ > > patchwork/models.py | 12 +++--- > > patchwork/notifications.py | 4 +- > > patchwork/signals.py | 2 +- > > patchwork/templates/patchwork/submission.html | 4 +- > > patchwork/tests/test_checks.py | 10 ++--- > > patchwork/tests/test_expiry.py | 8 ++-- > > patchwork/tests/test_notifications.py | 2 +- > > patchwork/tests/utils.py | 6 +-- > > .../notes/unify-timezones-0f7022f0c2a371be.yaml | 5 +++ > > 11 files changed, 77 insertions(+), 25 deletions(-) > > create mode 100644 patchwork/migrations/0024_timezone_unify.py > > create mode 100644 > > releasenotes/notes/unify-timezones-0f7022f0c2a371be.yaml > > > > diff --git a/docs/api/rest.rst b/docs/api/rest.rst > > index 3d7292e..d526b27 100644 > > --- a/docs/api/rest.rst > > +++ b/docs/api/rest.rst > > @@ -107,7 +107,8 @@ Schema > > ------ > > > > Responses are returned as JSON. Blank fields are returned as ``null``, > > rather > > -than being omitted. Timestamps use the ISO 8601 format:: > > +than being omitted. Timestamps use the ISO 8601 format, times are by > > default > > +in UTC:: > > > > YYYY-MM-DDTHH:MM:SSZ > > > > diff --git a/patchwork/migrations/0024_timezone_unify.py > > b/patchwork/migrations/0024_timezone_unify.py > > new file mode 100644 > > index 0000000..99f0642 > > --- /dev/null > > +++ b/patchwork/migrations/0024_timezone_unify.py > > @@ -0,0 +1,46 @@ > > +# -*- coding: utf-8 -*- > > +# Generated by Django 1.10.8 on 2018-02-22 23:11 > > +from __future__ import unicode_literals > > + > > +import datetime > > +from django.db import migrations, models > > + > > + > > +class Migration(migrations.Migration): > > + > > + dependencies = [ > > + ('patchwork', '0023_submissiontag'), > > + ] > > + > > + operations = [ > > + migrations.AlterField( > > + model_name='check', > > + name='date', > > + field=models.DateTimeField(default=datetime.datetime.utcnow), > > + ), > > + migrations.AlterField( > > + model_name='comment', > > + name='date', > > + field=models.DateTimeField(default=datetime.datetime.utcnow), > > + ), > > + migrations.AlterField( > > + model_name='emailconfirmation', > > + name='date', > > + field=models.DateTimeField(default=datetime.datetime.utcnow), > > + ), > > + migrations.AlterField( > > + model_name='event', > > + name='date', > > + field=models.DateTimeField(default=datetime.datetime.utcnow, > > help_text=b'The time this event was created.'), > > + ), > > + migrations.AlterField( > > + model_name='patchchangenotification', > > + name='last_modified', > > + field=models.DateTimeField(default=datetime.datetime.utcnow), > > + ), > > + migrations.AlterField( > > + model_name='submission', > > + name='date', > > + field=models.DateTimeField(default=datetime.datetime.utcnow), > > + ), > > + ] > > diff --git a/patchwork/models.py b/patchwork/models.py > > index a8bb015..c5a2059 100644 > > --- a/patchwork/models.py > > +++ b/patchwork/models.py > > @@ -308,7 +308,7 @@ class EmailMixin(models.Model): > > # email metadata > > > > msgid = models.CharField(max_length=255) > > - date = models.DateTimeField(default=datetime.datetime.now) > > + date = models.DateTimeField(default=datetime.datetime.utcnow) > > headers = models.TextField(blank=True) > > > > # content > > @@ -828,7 +828,7 @@ class Check(models.Model): > > > > patch = models.ForeignKey(Patch, on_delete=models.CASCADE) > > user = models.ForeignKey(User, on_delete=models.CASCADE) > > - date = models.DateTimeField(default=datetime.datetime.now) > > + date = models.DateTimeField(default=datetime.datetime.utcnow) > > > > state = models.SmallIntegerField( > > choices=STATE_CHOICES, default=STATE_PENDING, > > @@ -900,7 +900,7 @@ class Event(models.Model): > > db_index=True, > > help_text='The category of the event.') > > date = models.DateTimeField( > > - default=datetime.datetime.now, > > + default=datetime.datetime.utcnow, > > help_text='The time this event was created.') > > > > # event object > > @@ -965,7 +965,7 @@ class EmailConfirmation(models.Model): > > email = models.CharField(max_length=200) > > user = models.ForeignKey(User, null=True, on_delete=models.CASCADE) > > key = HashField() > > - date = models.DateTimeField(default=datetime.datetime.now) > > + date = models.DateTimeField(default=datetime.datetime.utcnow) > > active = models.BooleanField(default=True) > > > > def deactivate(self): > > @@ -973,7 +973,7 @@ class EmailConfirmation(models.Model): > > self.save() > > > > def is_valid(self): > > - return self.date + self.validity > datetime.datetime.now() > > + return self.date + self.validity > datetime.datetime.utcnow() > > > > def save(self, *args, **kwargs): > > limit = 1 << 32 > > @@ -999,5 +999,5 @@ class EmailOptout(models.Model): > > class PatchChangeNotification(models.Model): > > patch = models.OneToOneField(Patch, primary_key=True, > > on_delete=models.CASCADE) > > - last_modified = models.DateTimeField(default=datetime.datetime.now) > > + last_modified = models.DateTimeField(default=datetime.datetime.utcnow) > > orig_state = models.ForeignKey(State, on_delete=models.CASCADE) > > diff --git a/patchwork/notifications.py b/patchwork/notifications.py > > index 88e9662..a5f6423 100644 > > --- a/patchwork/notifications.py > > +++ b/patchwork/notifications.py > > @@ -35,7 +35,7 @@ from patchwork.models import PatchChangeNotification > > > > > > def send_notifications(): > > - date_limit = datetime.datetime.now() - datetime.timedelta( > > + date_limit = datetime.datetime.utcnow() - datetime.timedelta( > > minutes=settings.NOTIFICATION_DELAY_MINUTES) > > > > # We delay sending notifications to a user if they have other > > @@ -104,7 +104,7 @@ def expire_notifications(): > > Users whose registration confirmation has expired are removed. > > """ > > # expire any invalid confirmations > > - q = (Q(date__lt=datetime.datetime.now() - EmailConfirmation.validity) > > | > > + q = (Q(date__lt=datetime.datetime.utcnow() - > > EmailConfirmation.validity) | > > Q(active=False)) > > EmailConfirmation.objects.filter(q).delete() > > > > diff --git a/patchwork/signals.py b/patchwork/signals.py > > index e5e7370..f7b4f54 100644 > > --- a/patchwork/signals.py > > +++ b/patchwork/signals.py > > @@ -65,7 +65,7 @@ def patch_change_callback(sender, instance, raw, > > **kwargs): > > notification.delete() > > return > > > > - notification.last_modified = dt.now() > > + notification.last_modified = dt.utcnow() > > notification.save() > > > > > > diff --git a/patchwork/templates/patchwork/submission.html > > b/patchwork/templates/patchwork/submission.html > > index 6ed20c3..e817713 100644 > > --- a/patchwork/templates/patchwork/submission.html > > +++ b/patchwork/templates/patchwork/submission.html > > @@ -255,7 +255,7 @@ function toggle_div(link_id, headers_id) > > <div class="comment"> > > <div class="meta"> > > <span>{{ submission.submitter|personify:project }}</span> > > - <span class="pull-right">{{ submission.date }}</span> > > + <span class="pull-right">{{ submission.date }} UTC</span> > > </div> > > <pre class="content"> > > {{ submission|commentsyntax }} > > @@ -271,7 +271,7 @@ function toggle_div(link_id, headers_id) > > <div class="comment"> > > <div class="meta"> > > <span>{{ item.submitter|personify:project }}</span> > > - <span class="pull-right">{{ item.date }} | <a href="{% url > > 'comment-redirect' comment_id=item.id %}" > > + <span class="pull-right">{{ item.date }} UTC | <a href="{% url > > 'comment-redirect' comment_id=item.id %}" > > >#{{ forloop.counter }}</a></span> > > </div> > > <pre class="content"> > > diff --git a/patchwork/tests/test_checks.py > > b/patchwork/tests/test_checks.py > > index c0487f3..797390c 100644 > > --- a/patchwork/tests/test_checks.py > > +++ b/patchwork/tests/test_checks.py > > @@ -86,12 +86,12 @@ class PatchChecksTest(TransactionTestCase): > > self.assertChecksEqual(self.patch, [check_a, check_b]) > > > > def test_checks__duplicate_checks(self): > > - self._create_check(date=(dt.now() - timedelta(days=1))) > > + self._create_check(date=(dt.utcnow() - timedelta(days=1))) > > check = self._create_check() > > # this isn't a realistic scenario (dates shouldn't be set by user > > so > > # they will always increment), but it's useful to verify the > > removal > > # of older duplicates by the function > > - self._create_check(date=(dt.now() - timedelta(days=2))) > > + self._create_check(date=(dt.utcnow() - timedelta(days=2))) > > self.assertChecksEqual(self.patch, [check]) > > > > def test_checks__nultiple_users(self): > > @@ -107,7 +107,7 @@ class PatchChecksTest(TransactionTestCase): > > self.assertCheckCountEqual(self.patch, 1, {Check.STATE_SUCCESS: > > 1}) > > > > def test_check_count__multiple_checks(self): > > - self._create_check(date=(dt.now() - timedelta(days=1))) > > + self._create_check(date=(dt.utcnow() - timedelta(days=1))) > > self._create_check(context='new/test1') > > self.assertCheckCountEqual(self.patch, 2, {Check.STATE_SUCCESS: > > 2}) > > > > @@ -117,14 +117,14 @@ class PatchChecksTest(TransactionTestCase): > > self.assertCheckCountEqual(self.patch, 2, {Check.STATE_SUCCESS: > > 2}) > > > > def test_check_count__duplicate_check_same_state(self): > > - self._create_check(date=(dt.now() - timedelta(days=1))) > > + self._create_check(date=(dt.utcnow() - timedelta(days=1))) > > self.assertCheckCountEqual(self.patch, 1, {Check.STATE_SUCCESS: > > 1}) > > > > self._create_check() > > self.assertCheckCountEqual(self.patch, 2, {Check.STATE_SUCCESS: > > 1}) > > > > def test_check_count__duplicate_check_new_state(self): > > - self._create_check(date=(dt.now() - timedelta(days=1))) > > + self._create_check(date=(dt.utcnow() - timedelta(days=1))) > > self.assertCheckCountEqual(self.patch, 1, {Check.STATE_SUCCESS: > > 1}) > > > > self._create_check(state=Check.STATE_FAIL) > > diff --git a/patchwork/tests/test_expiry.py > > b/patchwork/tests/test_expiry.py > > index 054d156..ce308bc 100644 > > --- a/patchwork/tests/test_expiry.py > > +++ b/patchwork/tests/test_expiry.py > > @@ -46,7 +46,7 @@ class TestRegistrationExpiry(TestCase): > > return (user, conf) > > > > def test_old_registration_expiry(self): > > - date = ((datetime.datetime.now() - EmailConfirmation.validity) - > > + date = ((datetime.datetime.utcnow() - EmailConfirmation.validity) > > - > > datetime.timedelta(hours=1)) > > user, conf = self.register(date) > > > > @@ -57,7 +57,7 @@ class TestRegistrationExpiry(TestCase): > > EmailConfirmation.objects.filter(pk=conf.pk).exists()) > > > > def test_recent_registration_expiry(self): > > - date = ((datetime.datetime.now() - EmailConfirmation.validity) + > > + date = ((datetime.datetime.utcnow() - EmailConfirmation.validity) > > + > > datetime.timedelta(hours=1)) > > user, conf = self.register(date) > > > > @@ -68,7 +68,7 @@ class TestRegistrationExpiry(TestCase): > > EmailConfirmation.objects.filter(pk=conf.pk).exists()) > > > > def test_inactive_registration_expiry(self): > > - user, conf = self.register(datetime.datetime.now()) > > + user, conf = self.register(datetime.datetime.utcnow()) > > > > # confirm registration > > conf.user.is_active = True > > @@ -87,7 +87,7 @@ class TestRegistrationExpiry(TestCase): > > submitter = patch.submitter > > > > # ... then starts registration... > > - date = ((datetime.datetime.now() - EmailConfirmation.validity) - > > + date = ((datetime.datetime.utcnow() - EmailConfirmation.validity) > > - > > datetime.timedelta(hours=1)) > > user = create_user(link_person=False, email=submitter.email) > > user.is_active = False > > diff --git a/patchwork/tests/test_notifications.py > > b/patchwork/tests/test_notifications.py > > index 6cd3200..6d902f8 100644 > > --- a/patchwork/tests/test_notifications.py > > +++ b/patchwork/tests/test_notifications.py > > @@ -120,7 +120,7 @@ class PatchNotificationEmailTest(TestCase): > > self.project = create_project(send_notifications=True) > > > > def _expire_notifications(self, **kwargs): > > - timestamp = datetime.datetime.now() - \ > > + timestamp = datetime.datetime.utcnow() - \ > > datetime.timedelta(minutes=settings.NOTIFICATION_DELAY_MINUTES > > + 1) > > > > qs = PatchChangeNotification.objects.all() > > diff --git a/patchwork/tests/utils.py b/patchwork/tests/utils.py > > index 004c2ca..12b6d9e 100644 > > --- a/patchwork/tests/utils.py > > +++ b/patchwork/tests/utils.py > > @@ -214,7 +214,7 @@ def create_check(**kwargs): > > values = { > > 'patch': create_patch() if 'patch' not in kwargs else None, > > 'user': create_user() if 'user' not in kwargs else None, > > - 'date': dt.now(), > > + 'date': dt.utcnow(), > > 'state': Check.STATE_SUCCESS, > > 'target_url': 'http://example.com/', > > 'description': '', > > @@ -229,7 +229,7 @@ def create_series(**kwargs): > > """Create 'Series' object.""" > > values = { > > 'project': create_project() if 'project' not in kwargs else None, > > - 'date': dt.now(), > > + 'date': dt.utcnow(), > > 'submitter': create_person() if 'submitter' not in kwargs else > > None, > > 'total': 1, > > } > > @@ -275,7 +275,7 @@ def _create_submissions(create_func, count=1, > > **kwargs): > > 'submitter': create_person() if 'submitter' not in kwargs else > > None, > > } > > values.update(kwargs) > > - date = dt.now() > > + date = dt.utcnow() > > > > objects = [] > > for i in range(0, count): > > diff --git a/releasenotes/notes/unify-timezones-0f7022f0c2a371be.yaml > > b/releasenotes/notes/unify-timezones-0f7022f0c2a371be.yaml > > new file mode 100644 > > index 0000000..2513650 > > --- /dev/null > > +++ b/releasenotes/notes/unify-timezones-0f7022f0c2a371be.yaml > > @@ -0,0 +1,5 @@ > > +--- > > +other: > > + - | > > + Unify timezones used -- use UTC for both email submissions and > > internal > > + events. > > -- > > 2.13.6 > > > > _______________________________________________ > > Patchwork mailing list > > Patchwork@lists.ozlabs.org > > https://lists.ozlabs.org/listinfo/patchwork > _______________________________________________ Patchwork mailing list Patchwork@lists.ozlabs.org https://lists.ozlabs.org/listinfo/patchwork