Produits Entr'ouvert » Hobo

Je coince sur le test.

diff --git a/hobo/multitenant/settings_loaders.py b/hobo/multitenant/settings_loaders.py
index a355f21..1421c7e 100644
--- a/hobo/multitenant/settings_loaders.py
+++ b/hobo/multitenant/settings_loaders.py
@@ -387,7 +387,12 @@ class SettingsJSON(FileBaseSettingsLoader, SettingsDictUpdateMixin):

     def update_settings_from_path(self, tenant_settings, path):
         with open(path) as f:
-            self.handle_settings(tenant_settings, json.load(f))
+            try:
+                settings_json = json.load(f)
+            except ValueError:
+                pass
+            else:
+                self.handle_settings(tenant_settings, settings_json)

 class DictAdapter(dict):
diff --git a/tests_multitenant/test_settings.py b/tests_multitenant/test_settings.py
index 98ee238..8331d25 100644
--- a/tests_multitenant/test_settings.py
+++ b/tests_multitenant/test_settings.py
@@ -303,3 +303,20 @@ def test_tenant_json_settings_reload(tenants, settings, freezer):
         for tenant in tenants:
             with tenant_context(tenant):
                 assert django.conf.settings.EXTEND_ME == [1, 3]
+
+        # move 1 minute in the future
+        freezer.move_to(datetime.timedelta(seconds=60))
+
+        # update EXTEND_ME tenant value
+        for tenant in tenants:
+            with open(os.path.join(tenant.get_directory(), 'settings.json'), 'w') as fd:
+                json.dump({
+                    'EXTEND_ME': [99]
+                }, fd)
+
+        # move 1 minute in the future
+        freezer.move_to(datetime.timedelta(seconds=60))
+
+        for tenant in tenants:
+            with tenant_context(tenant):
+                assert django.conf.settings.EXTEND_ME == [99]

Le test ici n'est pas encore écrit jusqu'à tester un json mal formé, je chercher d'abord à changer la valeur de EXTEND_ME à 99 mais ça ne passe pas, je n'ai pas encore compris pourquoi.

            for tenant in tenants:
                with tenant_context(tenant):
>                   assert django.conf.settings.EXTEND_ME == [99]
E                   assert [1, 3] == [99]
E                     At index 0 diff: 1 != 99
E                     Left contains more items, first extra item: 3
E                     Use -v to get the full diff

Pas encore compris ce qui se passe, je me demande si c'est pas freezer.move_to(datetime.timedelta(seconds=60)) qui, en fait, ne fait pas son job ?... Un peu perdu je suis.

Je pense qu'ils ne faut pas "juste ne pas planter", il faut garder l'ancienne version des setttings le temps que ça remarche et balancer par ailleurs un gros mail d'erreur (mais alors là faut doser, parce qu'on peut facilement s'en prendre 1000)... parce qu'à chaque rechargement les settings sont nettoyés, et si on ne charge pas settings.json on a juste un truc vide (modulo tout ce que les autres settings-loader peuvent charger).

En gros faudrait chopper l'exception plus haut pendant le parcours des settings-loader, si un foire, on revient aux settings précédents on fait un logger.exception(.), et on note dans les settings qu'on l'a fait

settings.LOADER_ERROR = True

On se mangera juste autant de mails que de workers mais c'est mieux que 1000.

Pour ton souci de test, je dirai de tracer dans load_tenant_settings() je ne vois pas mieux.

Construit au dessus de #113270 .

Benjamin Dauvergne a écrit (#note-11):

Construit au dessus de #113270 .

Du coup maintenant que le service global ne plante plus sur une erreur dans un fichier JSON d'un tenant particulier grâce à #113270, ce ticket devient "ne pas planter sur le tenant particulier" ?

Perso pas trop convaincu par l'utilité, je trouve plus simple et efficace de pourvoir constater immédiatement le problème parce que ça plante et de corriger immédiatement, même si ça fait une micro interruption de service.

Emmanuel Cazenave a écrit (#note-12):

Perso pas trop convaincu par l'utilité, je trouve plus simple et efficace de pourvoir constater immédiatement le problème parce que ça plante

Le truc c'est qu'on ne constate pas immédiatement, perso si j'active un feature flag sur 3 instances en même temps je ne vais pas forcément voir que ça a fonctionné sur les 3 instances, et les traces arrivent dans un dossier qu'il m'est facile d'oublier de regarder.

De plus si on prend l'exemple qui a (re)lancé cette série de ticket, c'est un JSON invalide qui n'a pas fait planter tout de suite, mais au moment de la màj des recettes, faite par une tierce personne qui ne se doutait de rien.

Emmanuel Cazenave a écrit (#note-12):

Benjamin Dauvergne a écrit (#note-11):

Construit au dessus de #113270 .

Du coup maintenant que le service global ne plante plus sur une erreur dans un fichier JSON d'un tenant particulier grâce à #113270, ce ticket devient "ne pas planter sur le tenant particulier" ?

Oui c'est ça.

Perso pas trop convaincu par l'utilité, je trouve plus simple et efficace de pourvoir constater immédiatement le problème parce que ça plante et de corriger immédiatement, même si ça fait une micro interruption de service.

Pas du tout d'accord je préfère qu'on plante pas du tout, au pire ça lit la version précédente ce qui est tout ce qu'on demande dans ce cas là, à charge pour celui qui fait des modifications de constater que ça a produit l'effet escompté et de surveiller les journaux (sudo journalctl -f TENANT=<tenant> suffit). Les usagers ne sont pas là pour faire nos tests... après je veux bien conditionner ça pour ne le faire qu'en prod et et pas en recette si vous trouvez ça plus pratique.

Valentin Deniaud a écrit (#note-13):

Le truc c'est qu'on ne constate pas immédiatement, perso si j'active un feature flag sur 3 instances en même temps je ne vais pas forcément voir que ça a fonctionné sur les 3 instances, et les traces arrivent dans un dossier qu'il m'est facile d'oublier de regarder.

De mon coté, je fais toujours un par un avec à chaque fois un passage par l'interface pour voir si c'est ok. Mais ok deux contre un je me range à la majorité.

Benjamin Dauvergne a écrit (#note-14):

de surveiller les journaux (sudo journalctl -f TENANT=<tenant> suffit).

Je peux me mettre à faire ça au lieu de faire un tour dans l'interface, je découvre cette forme d'invocation avec TENANT=<tenant>. Ça pourrait être l'occasion d'initier un HowDoWeDoTenantSettings dans https://dev.entrouvert.org/projects/prod-eo/wiki avec un mini procédure de mise à jour des settings qui dirait ce qu'on doit voir dans les logs quand on a merdé le settings.json.