callback plugin: use utc timezone for now()

[mattp-]

rather than a tz-naive stamp. ara drf should do the right thing with timezones based on the now included iso tz.

also side note: sorry I know this project is hosted on opendev but I am having some firewall issues with it. are you accepting PR's here? if not I can try to sort out my issues.

Thank you!

[dmsimard]

hey @mattp- and thanks for the PR.

Unfortunately we're not set up to accept PRs yet but you can find docs on how to get started here or I can also send the patch on your behalf.

I understand where you're coming from with that patch... time zones are complicated :)

Right now, on top of the callback supplying timestamps, the server also has it's own configuration in regards to time zones here:

ara/ara/server/settings.py

Lines 206 to 208 in 31a30bf

	USE_TZ = True
	LOCAL_TIME_ZONE = tzlocal.get_localzone().zone
	TIME_ZONE = settings.get("TIME_ZONE", LOCAL_TIME_ZONE)

A concern I'd have with this change is if it'd suddenly cause users' playbooks to show as being recorded in the future (or past) and so, with that in mind, I'd be curious to see how your patch behaves with different server settings.

I also wonder whether we should go a step further and, for example, just unilaterally send and store everything in UTC and shift the timezone calculation down to the local client (or UI).

I don't have a strong opinion right now but it's likely something we should think about.

[mattp-]

@dmsimard I'm totally fine if you want to just cherry-pick this, but I can try to sort things out later tonight/tomorrow from a different computer.
wrt the django-side settings you are indeed correct, I was playing around with USE_TZ/TIME_ZONE as well in relation to this, I came to the realization its information being lost on the way in from the callback, not a missing server setting.
I'm pretty sure this change should be safe in all scenarios, because when specified with timezone.utc, the timestamp becomes unambiguous, when combined with the Datetimefield handling you already set with https://github.com/ansible-community/ara/blob/master/ara/api/models.py#L43 django.utils.timezone here.

[mattp-]

to add to above,

[nav] In [3]: datetime.datetime.now().isoformat()               
Out[3]: '2020-11-10T14:44:16.463320'

[nav] In [4]: datetime.datetime.now(datetime.timezone.utc).isoformat()                                                          
Out[4]: '2020-11-10T19:44:21.711861+00:00'

the +00:00 offset should always parse to the same epoch regardless of the server settings/how the datetime is parsed; and it should always be 1 to 1 with the localtime based stamp that is getting sent to the server currently. then with USE_TZ/TIME_ZONE timestamps get converted on the way back out again.

[mattp-]

@dmsimard made the gerrit review over at https://review.opendev.org/#/c/762673/

[dmsimard]

o/ @mattp-

Appreciate the time put in sending the patch, thanks !

I've just tested the patch and it works great:

I left the details in the review and the patch will land in master soon so I'll close this PR.
This will land in 1.5.4, I don't have an ETA but sooner rather than later.

[mattp-]

Thank you!

…

On Fri, Nov 13, 2020 at 8:15 PM David Moreau Simard < ***@***.***> wrote: Closed #186 <#186>. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#186 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAK2VWZGPF3XUAAXDRLJGDSPXKZLANCNFSM4TRAGVSQ> .

[dmsimard]

Merged: 3e5649b