| Message ID | 20180116175806.21832-1-vkabatov@redhat.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [v2] Avoid timezone confusion | expand |
On Tue, 2018-01-16 at 18:58 +0100, vkabatov@redhat.com wrote: > From: Veronika Kabatova <vkabatov@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 <vkabatov@redhat.com> What effect does this have on existing information in the database? Does this mean they'll all remain UTC+12 or are they stored in UTC format already? The Django docs [1] would lead me to suggest the former, given that we don't have USE_TZ set to True. I guess you can test that by setting up a deployment, creating information, then switching this option over. I'd do this myself but I'm not going to have time this week :( Stephen [1] https://docs.djangoproject.com/en/2.0/topics/i18n/timezones/
On Wed, 2018-01-17 at 19:18 +0000, Stephen Finucane wrote: > On Tue, 2018-01-16 at 18:58 +0100, vkabatov@redhat.com wrote: > > From: Veronika Kabatova <vkabatov@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 <vkabatov@redhat.com> > > What effect does this have on existing information in the database? > Does this mean they'll all remain UTC+12 or are they stored in UTC > format already? The Django docs [1] would lead me to suggest the > former, given that we don't have USE_TZ set to True. > > I guess you can test that by setting up a deployment, creating > information, then switching this option over. I'd do this myself but > I'm not going to have time this week :( Might be relevant. https://gist.github.com/aaugustin/2971450 Stephen > Stephen > > [1] https://docs.djangoproject.com/en/2.0/topics/i18n/timezones/ > _______________________________________________ > Patchwork mailing list > Patchwork@lists.ozlabs.org > https://lists.ozlabs.org/listinfo/patchwork
----- Original Message ----- > From: "Stephen Finucane" <stephen@that.guru> > To: vkabatov@redhat.com, patchwork@lists.ozlabs.org > Sent: Wednesday, January 17, 2018 8:22:27 PM > Subject: Re: [PATCH v2] Avoid timezone confusion > > On Wed, 2018-01-17 at 19:18 +0000, Stephen Finucane wrote: > > On Tue, 2018-01-16 at 18:58 +0100, vkabatov@redhat.com wrote: > > > From: Veronika Kabatova <vkabatov@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 <vkabatov@redhat.com> > > > > What effect does this have on existing information in the database? > > Does this mean they'll all remain UTC+12 or are they stored in UTC > > format already? The Django docs [1] would lead me to suggest the > > former, given that we don't have USE_TZ set to True. > > > > I guess you can test that by setting up a deployment, creating > > information, then switching this option over. I'd do this myself but > > I'm not going to have time this week :( > The effect of the change for the existing data is the same as if the admin decided to override the default TIME_ZONE setting. In the end that's what this change does, move the instance time to UTC. If the TIME_ZONE setting is overriden (production.py) nothing changes, we are only changing the default. To elaborate more, as we use timezone-naive format, the details depend on database backend and what underlying type from it Django uses to store datetimes. For mysql, they are saved as 'datetime' and this type doesn't know timezones and returns what you feed it. Django doesn't modify the times either, generated time is simply passed as a string. If you change timezone, 'datetime' is still the same. So submissions will have the right time, but internally triggered changes keep the original localized time and we can't really modify them since we can't retrieve the original timezone used. For postgres it's the exact opposite. Datetimes are stored as 'timestamp with time zone' in the DB, if no timezone is specified it uses the current local one. So it takes the times from submissions, thinking they are in local time zone and not UTC. OTOH, internally generated events are converted correctly exactly because of the local timezone and consecutive conversion. I have not verified what types are used for other backends but I believe they are similar to the mysql example. > Might be relevant. > > https://gist.github.com/aaugustin/2971450 > That's something different. We need to unify the timezones used. Making PW timezone-aware would not change anything, including the effect on old data (exactly because the dates used are not unified, and we have no way to find out the offsets the data were originally created with anyways). Hope this explains the situation, just ask if I wasn't clear Veronika > Stephen > > > Stephen > > > > [1] https://docs.djangoproject.com/en/2.0/topics/i18n/timezones/
Veronika Kabatova <vkabatov@redhat.com> writes: > ----- Original Message ----- >> From: "Stephen Finucane" <stephen@that.guru> >> To: vkabatov@redhat.com, patchwork@lists.ozlabs.org >> Sent: Wednesday, January 17, 2018 8:22:27 PM >> Subject: Re: [PATCH v2] Avoid timezone confusion >> >> On Wed, 2018-01-17 at 19:18 +0000, Stephen Finucane wrote: >> > On Tue, 2018-01-16 at 18:58 +0100, vkabatov@redhat.com wrote: >> > > From: Veronika Kabatova <vkabatov@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 <vkabatov@redhat.com> >> > >> > What effect does this have on existing information in the database? >> > Does this mean they'll all remain UTC+12 or are they stored in UTC >> > format already? The Django docs [1] would lead me to suggest the >> > former, given that we don't have USE_TZ set to True. >> > >> > I guess you can test that by setting up a deployment, creating >> > information, then switching this option over. I'd do this myself but >> > I'm not going to have time this week :( >> > > The effect of the change for the existing data is the same as if the admin decided > to override the default TIME_ZONE setting. In the end that's what this change does, > move the instance time to UTC. > > If the TIME_ZONE setting is overriden (production.py) nothing changes, we are only > changing the default. > > > To elaborate more, as we use timezone-naive format, the details depend on database > backend and what underlying type from it Django uses to store datetimes. > > For mysql, they are saved as 'datetime' and this type doesn't know timezones and > returns what you feed it. Django doesn't modify the times either, generated time > is simply passed as a string. If you change timezone, 'datetime' is still the > same. So submissions will have the right time, but internally triggered changes > keep the original localized time and we can't really modify them since we can't > retrieve the original timezone used. > > For postgres it's the exact opposite. Datetimes are stored as 'timestamp with > time zone' in the DB, if no timezone is specified it uses the current local one. > So it takes the times from submissions, thinking they are in local time zone and > not UTC. OTOH, internally generated events are converted correctly exactly > because of the local timezone and consecutive conversion. > > I have not verified what types are used for other backends but I believe they > are similar to the mysql example. > > >> Might be relevant. >> >> https://gist.github.com/aaugustin/2971450 >> > > That's something different. We need to unify the timezones used. Making PW > timezone-aware would not change anything, including the effect on old data > (exactly because the dates used are not unified, and we have no way to find > out the offsets the data were originally created with anyways). > Wow what a mess :( To make matters worse, Australia/Canberra osciallates between UTC+10 and UTC+11! So I agree we just completely give up on the correctness of previous data. Rather than changing the TIME_ZONE setting, can we use datetime.datetime.utcnow as the default for date in Event in models.py, rather than datetime.datetime.now? We might need to clean up some other uses - PatchChangeNotification and Check look like likely suspects. I'm happy to go hunting for those, I just want to know if there's a more fundamental reason that I've missed why that wouldn't work. Regards, Daniel > > Hope this explains the situation, just ask if I wasn't clear > > Veronika > > >> Stephen >> >> > Stephen >> > >> > [1] https://docs.djangoproject.com/en/2.0/topics/i18n/timezones/ > _______________________________________________ > Patchwork mailing list > Patchwork@lists.ozlabs.org > https://lists.ozlabs.org/listinfo/patchwork
----- Original Message ----- > From: "Daniel Axtens" <dja@axtens.net> > To: "Veronika Kabatova" <vkabatov@redhat.com>, "Stephen Finucane" <stephen@that.guru> > Cc: patchwork@lists.ozlabs.org > Sent: Tuesday, February 13, 2018 12:36:37 PM > Subject: Re: [PATCH v2] Avoid timezone confusion > > Veronika Kabatova <vkabatov@redhat.com> writes: > > > ----- Original Message ----- > >> From: "Stephen Finucane" <stephen@that.guru> > >> To: vkabatov@redhat.com, patchwork@lists.ozlabs.org > >> Sent: Wednesday, January 17, 2018 8:22:27 PM > >> Subject: Re: [PATCH v2] Avoid timezone confusion > >> > >> On Wed, 2018-01-17 at 19:18 +0000, Stephen Finucane wrote: > >> > On Tue, 2018-01-16 at 18:58 +0100, vkabatov@redhat.com wrote: > >> > > From: Veronika Kabatova <vkabatov@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 <vkabatov@redhat.com> > >> > > >> > What effect does this have on existing information in the database? > >> > Does this mean they'll all remain UTC+12 or are they stored in UTC > >> > format already? The Django docs [1] would lead me to suggest the > >> > former, given that we don't have USE_TZ set to True. > >> > > >> > I guess you can test that by setting up a deployment, creating > >> > information, then switching this option over. I'd do this myself but > >> > I'm not going to have time this week :( > >> > > > > The effect of the change for the existing data is the same as if the admin > > decided > > to override the default TIME_ZONE setting. In the end that's what this > > change does, > > move the instance time to UTC. > > > > If the TIME_ZONE setting is overriden (production.py) nothing changes, we > > are only > > changing the default. > > > > > > To elaborate more, as we use timezone-naive format, the details depend on > > database > > backend and what underlying type from it Django uses to store datetimes. > > > > For mysql, they are saved as 'datetime' and this type doesn't know > > timezones and > > returns what you feed it. Django doesn't modify the times either, generated > > time > > is simply passed as a string. If you change timezone, 'datetime' is still > > the > > same. So submissions will have the right time, but internally triggered > > changes > > keep the original localized time and we can't really modify them since we > > can't > > retrieve the original timezone used. > > > > For postgres it's the exact opposite. Datetimes are stored as 'timestamp > > with > > time zone' in the DB, if no timezone is specified it uses the current local > > one. > > So it takes the times from submissions, thinking they are in local time > > zone and > > not UTC. OTOH, internally generated events are converted correctly exactly > > because of the local timezone and consecutive conversion. > > > > I have not verified what types are used for other backends but I believe > > they > > are similar to the mysql example. > > > > > >> Might be relevant. > >> > >> https://gist.github.com/aaugustin/2971450 > >> > > > > That's something different. We need to unify the timezones used. Making PW > > timezone-aware would not change anything, including the effect on old data > > (exactly because the dates used are not unified, and we have no way to find > > out the offsets the data were originally created with anyways). > > > Wow what a mess :( To make matters worse, Australia/Canberra osciallates > between UTC+10 and UTC+11! So I agree we just completely give up on the > correctness of previous data. > OK, this explains the additional inconsistency I've seen and couldn't explain myself. > Rather than changing the TIME_ZONE setting, can we use > datetime.datetime.utcnow as the default for date in Event in models.py, > rather than datetime.datetime.now? > > We might need to clean up some other uses - PatchChangeNotification and > Check look like likely suspects. I'm happy to go hunting for those, I > just want to know if there's a more fundamental reason that I've missed > why that wouldn't work. > Sure, I'll fix all the .now occurrences. I don't see a reason why it shouldn't work either. Veronika > Regards, > Daniel > > > > > > Hope this explains the situation, just ask if I wasn't clear > > > > Veronika > > > > > >> Stephen > >> > >> > Stephen > >> > > >> > [1] https://docs.djangoproject.com/en/2.0/topics/i18n/timezones/ > > _______________________________________________ > > Patchwork mailing list > > Patchwork@lists.ozlabs.org > > https://lists.ozlabs.org/listinfo/patchwork >
diff --git a/docs/api/rest.rst b/docs/api/rest.rst index 3d7292e..66aee72 100644 --- a/docs/api/rest.rst +++ b/docs/api/rest.rst @@ -107,7 +107,9 @@ 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 (:ref:`overridable<deployment-other-options>` by ``TIME_ZONE`` +setting):: YYYY-MM-DDTHH:MM:SSZ diff --git a/docs/deployment/installation.rst b/docs/deployment/installation.rst index a570dd8..679312b 100644 --- a/docs/deployment/installation.rst +++ b/docs/deployment/installation.rst @@ -267,6 +267,8 @@ below: STATIC_ROOT = '/var/www/patchwork' +.. _deployment-other-options: + Other Options ^^^^^^^^^^^^^ @@ -281,6 +283,12 @@ setting overridden. The purpose of many of these variables is described in * ``DEFAULT_FROM_EMAIL`` * ``NOTIFICATION_FROM_EMAIL`` +.. note:: + + The default time zone is set to UTC. After changing, internally triggered + events will report time using this new timezone but submissions from emails + (cover letters, patches, comments) will still use UTC. + You can generate the ``SECRET_KEY`` with the following Python code: .. code-block:: python diff --git a/patchwork/settings/base.py b/patchwork/settings/base.py index 4b0d551..ec34f90 100644 --- a/patchwork/settings/base.py +++ b/patchwork/settings/base.py @@ -42,7 +42,7 @@ if django.VERSION >= (1, 10): else: MIDDLEWARE_CLASSES = _MIDDLEWARE_CLASSES -TIME_ZONE = 'Australia/Canberra' +TIME_ZONE = 'UTC' LANGUAGE_CODE = 'en-au' 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/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.