Development #20410
Passer convert-to-sql en commande de management
0%
Description
Prérequis pour pouvoir passer également hobo_deploy.
Entre les paramètres de la ligne de commande et la configuration stockée, un nom diffère (dbname vs database; on avait pris dbname parce que c'est ce qui est utilisé pour la commande psql), c'est quelque chose qu'on pourrait changer à cette occasion.
Fichiers
Demandes liées
Révisions associées
Historique
Mis à jour par Christophe Siraut il y a presque 6 ans
- Fichier 0003-management-passer-convert-to-sql-en-commande-de-mana.patch 0003-management-passer-convert-to-sql-en-commande-de-mana.patch ajouté
- Fichier 0002-tests-add-a-function-to-force-connections-close-when.patch 0002-tests-add-a-function-to-force-connections-close-when.patch ajouté
- Fichier 0001-management-convert_to_sql-add-failing-test.patch 0001-management-convert_to_sql-add-failing-test.patch ajouté
- Patch proposed changé de Non à Oui
Voici une première version, je veux bien une relecture rapide pour confirmer que c'est la bonne direction.
Les questions que je me pose:
- est-ce ok d'avoir dépouillé wcs/ctl/convertsql.py et à terme le supprimer?
- est-ce ok de récupérer les paramètres de connexion depuis le skeleton? Faut-il aussi proposer les arguments manuels?
- je ne suis pas encore à l'aise avec la façon de travailler de quixote, notamment comment est-ce que sql.get_connection_and_cursor() trouve les paramètres de connexion à postgresql?
Mis à jour par Frédéric Péters il y a presque 6 ans
- Statut changé de Nouveau à En cours
- est-ce ok d'avoir dépouillé wcs/ctl/convertsql.py et à terme le supprimer?
Je ne vois pas ça dans les patchs attachés. Mais 1) oui c'est utile de conserver la commande existante, dans un temps de transition, et oui elle peut devenir coquille presque vide, faisant appel à du code qui se trouverait désormais ailleurs.
- est-ce ok de récupérer les paramètres de connexion depuis le skeleton? Faut-il aussi proposer les arguments manuels?
Non, il faut passer par des paramètres sur la ligne de commande.
- je ne suis pas encore à l'aise avec la façon de travailler de quixote, notamment comment est-ce que sql.get_connection_and_cursor() trouve les paramètres de connexion à postgresql?
sql.get_connection_and_cursor() vient de wcs, pas de quixote. sql.get_connection() établit la connexion avec les paramètres présents dans la configuration, /var/lib/wcs/<tenant>/config.pck, paramètres chargés dans le publisher, obtenus via get_cfg().
Dans convertsql.py aujourd'hui, le code :
pub.cfg['postgresql'] = {
'database': sub_options.dbname,
'user': sub_options.user,
'password': sub_options.password,
'host': sub_options.host,
'port': sub_options.port,
}
sql.get_connection_and_cursor(new=True)
la config est remplie avec les paramètres passés à la fonction, une connexion est établie ensuite.
Mis à jour par Christophe Siraut il y a presque 6 ans
- Fichier 0004-add-management-command-convert_to_sql-20410.patch 0004-add-management-command-convert_to_sql-20410.patch ajouté
- Fichier 0003-add-management-command-configure_connection.patch 0003-add-management-command-configure_connection.patch ajouté
Voici une proposition de patches et les tests associés, j'ai pris le parti de séparer une commande de configuration des paramètres de connexion postgresql (pouvant être utilisée dans le cas d'un changement de serveur comme actuellement SaaS v2) et la commande convert_to_sql qui s'occupe de migrer les objets.
Mis à jour par Frédéric Péters il y a presque 6 ans
(lecture très rapide)
Tu peux modifier la configuration de ton éditeur pour autoriser des lignes plus longues, le bout suivant devrait tenir sur une ligne :
options_file = os.path.join(self.publisher.app_dir,
'site-options.cfg')
Il faut ajouter l'entête copyright/licence aux fichiers (sauf les tests).
Plutôt setup_db que configure_connection. Mais plutôt quand même continuer avec une seule commande, juste assurer qu'elle ne modifie pas les données si les tables existent déjà.
Utiliser add_arguments plutôt que create_parser.
Si tu veux factoriser un WcsTenantCommand, il faut adapter les autres commandes de gestion pour l'utiliser aussi. (commencer ça dans un commit isolé).
Mis à jour par Christophe Siraut il y a presque 6 ans
Plutôt setup_db que configure_connection. Mais plutôt quand même
continuer avec une seule commande, juste assurer qu'elle ne modifie
pas les données si les tables existent déjà.
Pas sûr de comprendre: est-ce que tu proposes de garder une seule
commande "convert_to_sql" qui soit abusable dans le scénario de
reconfiguration d'un tenant?
Utiliser add_arguments plutôt que create_parser.
Dans le cas de WcsTenantCommand j'avais opté pour surcharger
create_parser pour que les classes dérivées n'aient pas à invoquer
super().add_arguments().
Quelle est la longueur de ligne maximale? (j'étais à 80 caractères)
Mis à jour par Frédéric Péters il y a presque 6 ans
Pas sûr de comprendre: est-ce que tu proposes de garder une seule commande "convert_to_sql" qui soit abusable dans le scénario de reconfiguration d'un tenant?
Que convert_to_sql écrive les paramètres de configuration, sans écriture de données, quand la base de données "cible" contient déjà les tables, oui.
(après, que ce ticket ne touche à rien sur le fonctionnement, et qu'on discute d'évolutions dans un autre, ça me va aussi).
Quelle est la longueur de ligne maximale? (j'étais à 80 caractères)
Je suis surpris de ne pas trouver de référence à ça dans le wiki; comme http://docs.python-requests.org/en/latest/dev/contributing/#kenneth-reitz-s-code-style (Line-length can exceed 79 characters, to 100, when convenient. / Line-length can exceed 100 characters, when doing otherwise would be terribly inconvenient.)
Mis à jour par Christophe Siraut il y a presque 6 ans
Voici une version adaptée suite à tes remarques.
Mis à jour par Christophe Siraut il y a presque 6 ans
Mis à jour par Frédéric Péters il y a presque 6 ans
cur.execute('''SELECT pg_terminate_backend(pg_stat_activity.pid)
FROM pg_stat_activity
WHERE pg_stat_activity.datname = '{}'
AND pid <> pg_backend_pid();'''.format(known_elements.sql_db_name))
cur.close()
Il faut indenter différemment l'SQL, là son indentation le confond avec le code Python. (tu peux regarder dans wcs/sql.py).
On n'utilise pas .format() (je n'avais pas fait gaffe dans le précédent où je l'aurais déjà dit).
- Copyright (C) 2005-2017 Entr'ouvert
2018.
import os
...
On sépare les imports en différentes zones avec une ligne blanche entre (grosso modo modules systèmes, modules django, quixote, puis wcs).
Dans la version actuelle on ne touche pas au site-options.cfg, je n'aurais pas touché à ça mais ok, par contre utilise plutôt atomic_write pour remplacer le fichier (de qommon.storage).
Je pensais que tu partagerais le code entre l'ancien et le nouveau convert-to-sql, que l'ancien ne devienne qu'un wrapper vers le nouveau.
La barre de progression, ce serait bien de la conserver. (quand on utilise la commande sur un site existant depuis un certain temps, c'est vraiment plus confortable).
Je note pour plus tard, il y aurait en évolution à voir dans quelle mesure la méthode initialize_sql() de la classe Publisher pourrait être exploitée.
Mis à jour par Christophe Siraut il y a presque 6 ans
- Fichier 0001-tests-utilities-add-force_connections_close.patch 0001-tests-utilities-add-force_connections_close.patch ajouté
- Fichier 0002-move-convert_to_sql-to-management-command-20410.patch 0002-move-convert_to_sql-to-management-command-20410.patch ajouté
Nouvelle version, en tenant compte des commentaires.
Mis à jour par Frédéric Péters il y a presque 6 ans
Restent les parties qui utilisent format, comme ('{} {}'.format(formdata.fromdef, formdata.id)); le reste d'un œil rapide me semble ok. (je n'ai pas exécuté pour de vrai et jenkins annonce une erreur qui est également à voir, https://jenkins.entrouvert.org/job/wcs-wip-branches/178/).
Mis à jour par Christophe Siraut il y a presque 6 ans
Je ne comprends pas bien l'erreur de jenkins, et ne parviens pas à la reproduire en local (pytest/jessie).
L'erreur ne survient pas si on garde intact wcs/ctl/convertsql.py.
Mis à jour par Christophe Siraut il y a presque 6 ans
- Fichier 0002-move-convert_to_sql-to-management-command-20410.patch 0002-move-convert_to_sql-to-management-command-20410.patch ajouté
- Fichier 0001-tests-utilities-add-force_connections_close.patch 0001-tests-utilities-add-force_connections_close.patch ajouté
J'ai mis à jour la branche wip afin de passer les tests, patches attachés.
La différence avec la version précédente c'est que j'ai laissé tomber la modification du code original pour pointer vers le nouveau.
Mis à jour par Frédéric Péters il y a presque 6 ans
La différence avec la version précédente c'est que j'ai laissé tomber la modification du code original pour pointer vers le nouveau.
Qu'il soit donc supprimé après un tour un peu général voir où il pourrait être utilisé (peut-être même nulle part de manière automatique, je ne sais pas); on ne peut pas avoir du code dupliqué ainsi.
Mis à jour par Christophe Siraut il y a presque 6 ans
- Fichier FAIL_0003-link-old-convertsql-command-to-current-implementatio.patch FAIL_0003-link-old-convertsql-command-to-current-implementatio.patch ajouté
(Juste pour l'historique, j'attache le patch que refuse jenkins, et les erreurs associées)
=================================== FAILURES ===================================
______________________________ test_cron_command _______________________________
def test_cron_command():
pub = create_temporary_pub()
with mock.patch('wcs.qommon.management.commands.cron.cron_worker') as cron_worker:
> call_command('cron')
tests/test_publisher.py:175:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
../../../shiningpanda/jobs/22cf219b/virtualenvs/d41d8cd9/local/lib/python2.7/site-packages/django/core/management/__init__.py:120: in call_command
return command.execute(*args, **defaults)
../../../shiningpanda/jobs/22cf219b/virtualenvs/d41d8cd9/local/lib/python2.7/site-packages/django/core/management/base.py:441: in execute
output = self.handle(*args, **options)
wcs/qommon/management/commands/cron.py:43: in handle
publisher_class.register_cronjobs()
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
cls = <class 'publisher.WcsPublisher'>
@classmethod
def register_cronjobs(cls):
> super(WcsPublisher, cls).register_cronjobs()
E TypeError: super() argument 1 must be type, not None
wcs/publisher.py:115: TypeError
_________________________ test_migration_12_users_fts __________________________
@postgresql
def test_migration_12_users_fts():
conn, cur = sql.get_connection_and_cursor()
cur.execute('UPDATE wcs_meta SET value = 11 WHERE key = %s', ('sql_level',))
sql.SqlUser.wipe()
user = sql.SqlUser()
user.name = 'Pierre'
user.store()
# remove the fts column
cur.execute('ALTER TABLE users DROP COLUMN fts')
assert not column_exists_in_table(cur, 'users', 'fts')
sql.migrate()
assert column_exists_in_table(cur, 'users', 'fts')
assert migration_level(cur) >= 12
# no fts, migration only prepare re-index
assert len(sql.SqlUser.get_ids_from_query('pierre')) == 0
assert sql.is_reindex_needed('user', conn=conn, cur=cur) is True
assert sql.is_reindex_needed('formdata', conn=conn, cur=cur) is True
> call_command('cron') # first cron = reindex
tests/test_sql.py:1112:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
../../../shiningpanda/jobs/22cf219b/virtualenvs/d41d8cd9/local/lib/python2.7/site-packages/django/core/management/__init__.py:120: in call_command
return command.execute(*args, **defaults)
../../../shiningpanda/jobs/22cf219b/virtualenvs/d41d8cd9/local/lib/python2.7/site-packages/django/core/management/base.py:441: in execute
output = self.handle(*args, **options)
wcs/qommon/management/commands/cron.py:43: in handle
publisher_class.register_cronjobs()
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
cls = <class 'publisher.WcsPublisher'>
@classmethod
def register_cronjobs(cls):
> super(WcsPublisher, cls).register_cronjobs()
E TypeError: super() argument 1 must be type, not None
wcs/publisher.py:115: TypeError
______________________ test_migration_21_users_ascii_name ______________________
@postgresql
def test_migration_21_users_ascii_name():
conn, cur = sql.get_connection_and_cursor()
cur.execute('UPDATE wcs_meta SET value = 11 WHERE key = %s', ('sql_level',))
sql.SqlUser.wipe()
user = sql.SqlUser()
user.name = 'Jean Sénisme'
user.store()
# remove the ascii_name column
cur.execute('ALTER TABLE users DROP COLUMN ascii_name')
assert not column_exists_in_table(cur, 'users', 'ascii_name')
sql.migrate()
assert column_exists_in_table(cur, 'users', 'ascii_name')
assert migration_level(cur) >= 21
# no fts, migration only prepare re-index
assert sql.SqlUser.count([st.Equal('ascii_name', 'jean senisme')]) == 0
assert sql.is_reindex_needed('user', conn=conn, cur=cur) is True
assert sql.is_reindex_needed('formdata', conn=conn, cur=cur) is True
> call_command('cron') # first cron = reindex
tests/test_sql.py:1146:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
../../../shiningpanda/jobs/22cf219b/virtualenvs/d41d8cd9/local/lib/python2.7/site-packages/django/core/management/__init__.py:120: in call_command
return command.execute(*args, **defaults)
../../../shiningpanda/jobs/22cf219b/virtualenvs/d41d8cd9/local/lib/python2.7/site-packages/django/core/management/base.py:441: in execute
output = self.handle(*args, **options)
wcs/qommon/management/commands/cron.py:43: in handle
publisher_class.register_cronjobs()
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
cls = <class 'publisher.WcsPublisher'>
@classmethod
def register_cronjobs(cls):
> super(WcsPublisher, cls).register_cronjobs()
E TypeError: super() argument 1 must be type, not None
wcs/publisher.py:115: TypeError
generated xml file: /var/lib/jenkins/jobs/wcs-wip-branches/workspace/test_results.xml
---------- coverage: platform linux2, python 2.7.13-final-0 ----------
Coverage XML written to file coverage.xml
============= 3 failed, 1398 passed, 21 skipped in 837.86 seconds ==============
Build step 'Virtualenv Builder' marked build as failure
Enregistrement des résultats des tests
Sending e-mails to: admin+jenkins-wcs@entrouvert.com csiraut@entrouvert.com
[Cobertura] Publishing Cobertura coverage report...
[Cobertura] Publishing Cobertura coverage results...
[Cobertura] Cobertura coverage report found.
Finished: FAILURE
Mis à jour par Christophe Siraut il y a presque 6 ans
(désolé pour l'objet Struct(), c'est une scorie)
Mis à jour par Christophe Siraut il y a presque 6 ans
- Fichier 0003-remove-previous-convert-to-sql-command-20410.patch 0003-remove-previous-convert-to-sql-command-20410.patch ajouté
- Fichier 0002-move-convert_to_sql-to-management-command-20410.patch 0002-move-convert_to_sql-to-management-command-20410.patch ajouté
- Fichier 0001-tests-utilities-add-force_connections_close.patch 0001-tests-utilities-add-force_connections_close.patch ajouté
Voici le patch pour la suppression de l'ancienne implémentation.
Mis à jour par Christophe Siraut il y a presque 6 ans
- Lié à Bug #24716: convert_to_sql: créér la base de données si inexistante ajouté
Mis à jour par Christophe Siraut il y a presque 6 ans
- Lié à Bug #24715: Ajouter une commande de management pour configurer la base de données ajouté
Mis à jour par Frédéric Péters il y a presque 6 ans
Qu'il soit donc supprimé après un tour un peu général voir où il pourrait être utilisé. (...)
C'est au moins utilisé chez imio (mention dans leur module docker-teleservices), comme on n'assure pas de compatibilité, j'attendrais de pouvoir en discuter avec eux.
Mis à jour par Frédéric Péters il y a presque 6 ans
- Statut changé de En cours à Solution validée
Voilà, je pense que c'est ok de pousser ça, en réunissant 0002 et 0003 en un unique commit.
Mis à jour par Frédéric Péters il y a presque 6 ans
- Statut changé de Solution validée à Résolu (à déployer)
commit bb064934b3261b5c4dfdce7c93247193e5198070
Author: Christophe Siraut <csiraut@entrouvert.com>
Date: Tue Jun 26 17:13:15 2018 +0200
move convert_to_sql to management command (#20410)
commit 888622ea185b734dd684dd434454cf110dbe8bbf
Author: Christophe Siraut <csiraut@entrouvert.com>
Date: Thu Jun 21 17:07:00 2018 +0200
tests/utilities: add force_connections_close()
Mis à jour par Frédéric Péters il y a plus de 5 ans
- Statut changé de Résolu (à déployer) à Solution déployée
move convert_to_sql to management command (#20410)