Project

General

Profile

Development #16599

cook : vérification des noms de domaine avant de lancer le déploiement

Added by Frédéric Péters over 2 years ago. Updated 5 months ago.

Status:
Solution déployée
Priority:
Normal
Category:
-
Target version:
-
Start date:
29 May 2017
Due date:
% Done:

0%

Patch proposed:
Yes
Planning:
No

Description

Si le nom de domaine ne résoud pas, le déploiement partira dans le vide, autant détecter ça dès que possible.

0002-cook-add-prechecks-on-recipe-16599.patch View (3.16 KB) Christophe Siraut, 13 Sep 2018 11:27 AM

0002-cook-add-prechecks-on-recipe-16599.patch View (3.16 KB) Christophe Siraut, 13 Sep 2018 12:07 PM

0002-cook-add-prechecks-on-recipe-16599.patch View (3.25 KB) Christophe Siraut, 13 Sep 2018 03:20 PM

0002-cook-add-prechecks-on-recipe-16599.patch View (3.08 KB) Christophe Siraut, 14 Sep 2018 02:39 PM

0002-cook-add-prechecks-on-recipe-16599.patch View (4.88 KB) Christophe Siraut, 17 Sep 2018 01:21 PM

0001-commands-cook-add-prechecks-on-recipe-16599.patch View (2.91 KB) Christophe Siraut, 03 Oct 2018 06:20 PM

test_cook.py View (2.29 KB) Nicolas Roche, 16 Apr 2019 02:50 PM


Related issues

Related to Hobo - Development #23823: Fournir une API de vérification de bon fonctionnement Solution déployée 14 May 2018
Related to Hobo - Bug #32687: regression sur cook avec plusieurs variables Solution déployée 29 Apr 2019

Associated revisions

Revision 73848f82 (diff)
Added by Christophe Siraut 5 months ago

commands/cook: add prechecks on recipe (#16599)

History

#1 Updated by Thomas Noël about 2 years ago

En fait on pourrait aller jusqu'à tester si les URL à déployer ne répondent pas 404 : dans ce cas, message d'erreur et arrêt. (si c'est pas 404 c'est que le logiciel qui doit faire le déploiement n'est pas là)

#2 Updated by Serghei Mihai over 1 year ago

Yep, et dans le message d'échec de deploiement rajouter la raison: nom de domaine inexistant, reponse HTTP foireuse, erreur de certificat.

#3 Updated by Christophe Siraut over 1 year ago

  • Related to Development #23823: Fournir une API de vérification de bon fonctionnement added

#5 Updated by Christophe Siraut about 1 year ago

proposition en attente de #23823

on pourrait aller jusqu'à tester si les URL à déployer ne répondent pas 404

je n'ai pas été jusque là (je me dis que c'est probablement suffisant en l'état)

#6 Updated by Christophe Siraut about 1 year ago

  • ajouté une condition pour les actions sans url.

#7 Updated by Thomas Noël about 1 year ago

Christophe Siraut a écrit :

  • ajouté une condition pour les actions sans url.

(c'est le même patch)

#8 Updated by Christophe Siraut about 1 year ago

(c'est le même patch)

bizarre (une fenêtre ouverte dans nautilus au contenu périmé, le contenant est un lien symbolique)
voici.

#9 Updated by Christophe Siraut about 1 year ago

version simplifiée (moins de code)

#10 Updated by Thomas Noël about 1 year ago

Typo : s/resovable/resolvable/

Et ça serait mieux de raiser un CommandError plutôt qu'un sys.exit.

Et c'est juste moi, mais je préfère cette forme pour gérer l'absence d'url dans l'action:

   if 'url' not in action_args:
       continue
   ...

Mais pour moi, ça marche pas avec des déclarations comme "url": "https://${combo}/" qu'on pose dans nos recipe.json. Il faut gérer les variables d'abord.

#11 Updated by Christophe Siraut 12 months ago

Il faut gérer les variables d'abord.

Oui c'est juste, ajouté une méthode get_steps() pour compiler les arguments.

#12 Updated by Thomas Noël 12 months ago

Christophe Siraut a écrit :

Il faut gérer les variables d'abord.

Oui c'est juste, ajouté une méthode get_steps() pour compiler les arguments.

Je trouve pas ça super joli (une méthode appelée une seule fois, je doute toujours ; et le patch serait plus lisible sans). Je sais, je suis pénible, mais j'ai pas envie que cette partie du code devienne trop touffue...

#14 Updated by Benjamin Dauvergne 7 months ago

  • Status changed from Solution proposée to En cours
  • Assignee set to Christophe Siraut

Pas bon le découpage de la boucle for step in a perdu des bouts au passage, je ferai une première passe d'assemblage des action/arction_args, puis éventuels checks(), puis application, comme cela c'est bien découpé (j'aurai préféré des méthodes check_<action>() mais comme on a autant de méthodes que de type de briques, ça attendra une évolution quand on aura une action avec un paramètre url qui n'est pas le déploiement d'une brique).

#15 Updated by Christophe Siraut 6 months ago

J'ai adapté le patch en suivant la remarque de Benjamin.

#16 Updated by Benjamin Dauvergne 6 months ago

Il me semble qu'on a aucun test sur cook...

#19 Updated by Christophe Siraut 6 months ago

  • Status changed from En cours to Solution proposée

J'ai ajouter deux tests sommaires, ça me semble suffisant (sinon il faut mocker les différentes fonctions réseau pour éviter par exemple de faire des vérification de certificat en vrai à chaque exécution des tests)

#22 Updated by Christophe Siraut 6 months ago

Déplacé les tests vers le bon dossier. (Et je ne suis pas parvenu à concilier pytest-django et django-tenant-schema)

#23 Updated by Benjamin Dauvergne 6 months ago

Christophe Siraut a écrit :

Déplacé les tests vers le bon dossier. (Et je ne suis pas parvenu à concilier pytest-django et django-tenant-schema)

Sans charger debian_config_common c'est difficile effectivement, il suffirait de l'ajouter à tests/settings.py comme on le fait pour multitenant (le fait est que hobo le charge aussi dans debian/server/debian_config.py).

#26 Updated by Frédéric Péters 6 months ago

C'est un truc que je disais à Nicolas par rapport à l'intégration Matomo je préfère garder le maximum hobo indépendant du côté multitenants (comme les autres applications).

Tester un bout de code vérifiant une série de noms de domaine, ça me semble pouvoir tenir sans faire appel à du multitenant.

(qu'il y ait, par ailleurs, de nouveaux tests appelant toute l'infrastructure multitenant pour un gros test de cook, très bien).

Dans la pratique, ça m'ennuie de voir un changement comme :

-def test_response(app, admin_user, services, monkeypatch):
+def test_response(tenant_schema, fake_notify, app, admin_user, services, monkeypatch):

qui me fait dire que tous les tests devraient désormais dépendre de deux fixtures supplémentaires.

#27 Updated by Christophe Siraut 6 months ago

  • Status changed from Solution proposée to Information nécessaire

qui me fait dire que tous les tests devraient désormais dépendre de deux fixtures supplémentaires.

Une seule fixture supplémentaire : tenant_schema. (L'autre c'était une erreur de ma part)

je préfère garder le maximum hobo indépendant du côté multitenants

C'est bête j'ai travaillé à convertir les tests existants pour fonctionner avec tenant-schemas. En pratique les installations que je connais utilisent majoritairement cette configuration (et je voulais éviter d'ajouter un namespace "test_schemas" supplémentaire). Préfères-tu abandonner bb73ce7669559236887d19a7350f4797d4b54526 ou doubler ces tests : avec et sans tenant-schemas?

#28 Updated by Frédéric Péters 6 months ago

Je confirme mon commentaire précédent, je préfère garder le maximum indépendant du multitenant. (et ça inclut les tests).

#29 Updated by Benjamin Dauvergne 6 months ago

Je n'ai pas d'avis sur les remarques de Fred, de mon coté ça m'intéressait juste de ne pas mélanger les tests de hobo avec les tests de l'infra multitenant; si seul la commande hook demande le mélange des deux, je ne pense pas que ça pose problème que seul ses tests dépendent d'une fixture avec multitenant.

#30 Updated by Christophe Siraut 6 months ago

  • Status changed from Information nécessaire to Solution proposée

J'ai adapté le patch en séparant les tests qui utilisent tenant-schema.

#31 Updated by Nicolas Roche 5 months ago

J'ai relu.
Peut-être ajouter les 2 tests ci-dessous pour couvrir tout le code de la fonction ajoutée.

def test_cook_invalid_certificat(db, fake_notify, monkeypatch):
    monkeypatch.setattr(ServiceBase, 'is_resolvable', lambda x: True)
    monkeypatch.setattr(ServiceBase, 'has_valid_certificate', lambda x: False)
    with pytest.raises(CommandError) as e_info:
        call_command('cook', 'tests_schemas/recipe.json')
        assert 'has no valid certificate' in str(e_info.value)

def test_cook_unknown_action(db, fake_notify, monkeypatch):
    RECEIPE=""" 
{
  "steps": [
    {"create-Bobo": {
      "url": "https://dev.entrouvert.org/" 
    }}
  ]
}
""" 
    recipe_file = '/tmp/recipe_having_wrong_action.json'
    with open(recipe_file, 'w') as handler:
        handler.write(RECEIPE)
    with pytest.raises(CommandError) as e_info:
        call_command('cook', recipe_file)
        assert 'Unknown action Bobo' in str(e_info.value)

Voir si on peut pas plutôt tester Command.check_action() dans 'tests/',
mais garder quand même 'tests_schemas' pour les futurs tests sur les déploiements via cook.
Je regarde si je peux proposer quelque-chose dans ce sens...

#32 Updated by Nicolas Roche 5 months ago

  • Status changed from Solution proposée to En cours
  • File test_cook.py View added

Je vous propose d'ajouter ce fichier dans le répertoire '/tests'.
Mais je suis également preneur pour un nouveau répertoire '/tests_schemas' pour les test à venir sur les déploiements pré-configurés qui nécessiteraient d'avoir un vrai objet Command sous la main (comme par exemple #14630).

#33 Updated by Christophe Siraut 5 months ago

  • Status changed from En cours to Solution proposée

J'ai inclus ton fichier de tests (plus clair, merci) en sortant les assert des blocs pytest.raises().

#34 Updated by Nicolas Roche 5 months ago

  • Status changed from Solution proposée to Solution validée

Je valide.

#35 Updated by Christophe Siraut 5 months ago

  • Status changed from Solution validée to Résolu (à déployer)
commit 73848f82f3ac0eeca52b2e1891b4e15d4019c970 (HEAD -> master, origin/master, origin/HEAD)
Author: Christophe Siraut <csiraut@entrouvert.com>
Date:   Wed Mar 27 17:34:01 2019 +0100

    commands/cook: add prechecks on recipe (#16599)

#36 Updated by Frédéric Péters 5 months ago

  • Status changed from Résolu (à déployer) to Solution déployée

#37 Updated by Nicolas Roche 5 months ago

  • Related to Bug #32687: regression sur cook avec plusieurs variables added

Also available in: Atom PDF