misc: improve use of spooler (#76419) #70

bdauvergne · 2023-04-08T02:19:37+02:00

bdauvergne commented

2023-04-08 02:19:37 +02:00

Trying to import uwsgi should not be used to detect if we are running in
pytest or not, in order to run callbacks attached to on_commit event in
the context of pytest-django (which run all tests inside implicit
transactions).

Here we replace somme spooler functions by a simpler to_spooler() which
can launch inside the spooler any picklable functions.

For running on_commit() callbacks, the django.db.transaction module is
mirrored inside chrono.utils.transaction and we monkeypatch it during
tests to immediately run any defined on_commit callback.

Trying to import uwsgi should not be used to detect if we are running in pytest or not, in order to run callbacks attached to on_commit event in the context of pytest-django (which run all tests inside implicit transactions). Here we replace somme spooler functions by a simpler to_spooler() which can launch inside the spooler any picklable functions. For running on_commit() callbacks, the django.db.transaction module is mirrored inside chrono.utils.transaction and we monkeypatch it during tests to immediately run any defined on_commit callback.

bdauvergne commented

2023-04-08 02:24:05 +02:00

J'ai un léger doute sur ce code

+def refresh_exception_source(source):
     try:
-        event = Event.objects.get(pk=args['event_id'])
-    except Event.DoesNotExist:
-        return
-
-    event.notify_checked()
+        source.refresh_timeperiod_exceptions_from_ics()
+    except ICSError:
+        pass
....

     def refresh_timeperiod_exceptions(self, data=None):
-        if 'uwsgi' in sys.modules:
-            from chrono.utils.spooler import refresh_exception_source
-
-            tenant = getattr(connection, 'tenant', None)
-            transaction.on_commit(
-                lambda: refresh_exception_source.spool(
-                    source_id=str(self.pk), domain=getattr(tenant, 'domain_url', None)
-                )
-            )
-            return
-
-        self.refresh_timeperiod_exceptions_from_ics(data=data)
+        transaction.on_commit(lambda: refresh_exception_source.spool(self))

Je ne comprends pas pourquoi dans les tests on passe data=data mais pas quand le spooler est utilisé, dans le patch j'ai supposé que ça devait toujours marcher sans le paramètre data. J'ai l'impression que c'est juste du code tordu pour ne pas systématiquement refaire une requête HTTP pour obtenir le contenu du fichier .ics (qu'il faut refaire de toute façon dans le spooler, pour ne pas passer trop de contenu via le fichier du job).

J'ai un léger doute sur ce code ``` +def refresh_exception_source(source): try: - event = Event.objects.get(pk=args['event_id']) - except Event.DoesNotExist: - return - - event.notify_checked() + source.refresh_timeperiod_exceptions_from_ics() + except ICSError: + pass .... def refresh_timeperiod_exceptions(self, data=None): - if 'uwsgi' in sys.modules: - from chrono.utils.spooler import refresh_exception_source - - tenant = getattr(connection, 'tenant', None) - transaction.on_commit( - lambda: refresh_exception_source.spool( - source_id=str(self.pk), domain=getattr(tenant, 'domain_url', None) - ) - ) - return - - self.refresh_timeperiod_exceptions_from_ics(data=data) + transaction.on_commit(lambda: refresh_exception_source.spool(self)) ``` Je ne comprends pas pourquoi dans les tests on passe data=data mais pas quand le spooler est utilisé, dans le patch j'ai supposé que ça devait toujours marcher sans le paramètre data. J'ai l'impression que c'est juste du code tordu pour ne pas systématiquement refaire une requête HTTP pour obtenir le contenu du fichier .ics (qu'il faut refaire de toute façon dans le spooler, pour ne pas passer trop de contenu via le fichier du job).

bdauvergne force-pushed wip/76419-utiliser-hobo-multitenant-uwsgid from 193c2e59fe to 1182e566b9

2023-04-08 02:25:22 +02:00

Compare

bdauvergne force-pushed wip/76419-utiliser-hobo-multitenant-uwsgid from 1182e566b9 to bb543a1ba5

2023-04-08 02:57:36 +02:00

Compare

bdauvergne force-pushed wip/76419-utiliser-hobo-multitenant-uwsgid from bb543a1ba5 to 9f18031a83

2023-04-08 03:05:41 +02:00

Compare

bdauvergne force-pushed wip/76419-utiliser-hobo-multitenant-uwsgid from 9f18031a83 to 6348e798c8

2023-04-08 12:05:58 +02:00

Compare

bdauvergne force-pushed wip/76419-utiliser-hobo-multitenant-uwsgid from 6348e798c8 to 29caf73f79

2023-04-08 12:09:16 +02:00

Compare

bdauvergne force-pushed wip/76419-utiliser-hobo-multitenant-uwsgid from 29caf73f79 to 636292c2fd

2023-04-08 16:17:21 +02:00

Compare

bdauvergne force-pushed wip/76419-utiliser-hobo-multitenant-uwsgid from 636292c2fd to 0aa45508b3

2023-04-08 16:23:52 +02:00

Compare

bdauvergne force-pushed wip/76419-utiliser-hobo-multitenant-uwsgid from 0aa45508b3 to 80a25a831a

2023-04-09 00:56:20 +02:00

Compare

bdauvergne force-pushed wip/76419-utiliser-hobo-multitenant-uwsgid from 80a25a831a to 78708d456a

2023-04-11 11:42:45 +02:00

Compare

gitea/chrono/pipeline/head This commit looks good

Details

This pull request has changes conflicting with the target branch.

chrono/agendas/models.py
chrono/utils/spooler.py
tests/conftest.py

You can also view command line instructions.

Step 1:

From your project repository, check out a new branch and test the changes.

git checkout -b wip/76419-utiliser-hobo-multitenant-uwsgid main

git pull origin wip/76419-utiliser-hobo-multitenant-uwsgid

Step 2:

Merge the changes and update on Gitea.

git checkout main

git merge --no-ff wip/76419-utiliser-hobo-multitenant-uwsgid

git push origin main

Sign in to join this conversation.

No reviewers

No Label

No Milestone

No Assignees

1 Participants

Notifications

Due Date

The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: entrouvert/chrono#70