Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Intègre le code de django_munin à notre dépôt #6578

Merged
merged 4 commits into from
Feb 24, 2024

Conversation

Arnaud-D
Copy link
Contributor

@Arnaud-D Arnaud-D commented Jan 20, 2024

Cette PR intègre le code de django_munin au sein de notre dépôt. C'est la solution qui a été décidée collégialement par l'équipe de développement plutôt que de reprendre le rôle de mainteneurs du paquet. Gérer un paquet nécessite plus de cérémonie dans le process de mise à jour et on est les derniers utilisateurs connus de ce paquet.

Techniquement :

  • le code de django_munin est en BSD 3 clauses, ce qui permet bien de l'intégrer à notre produit GPL en conservant la notice de copyright et le texte de la licence ;
  • l'intégration se fait sous forme d'app Django,
  • j'en ai profité pour exprimer plus clairement les routes associées,
  • il y a une subtilité avec la migration, voir ci-dessous.

Mise en prod

La subtilité avec la migration est qu'en dev, la table utilisée par django_munin pour faire les tests de performance de la base de données n'existe pas (en tout cas, pas chez moi). Cela signifie que les migrations incluses dans le paquet font leur job et créent la table qui va bien.

Cependant, cette table existe bel et bien en prod et sur la bêta (vu que la fonctionnalité marche). Son contenu ne semble pas critique, mais mieux vaut faire ça proprement. Je pense qu'il faudra faire un python manage.py migrate --fake-initial pour éviter des soucis. Voir la doc associée de Django.

Questions

  • Est-ce qu'on se sert du fichier dans django_munin/plugins ? C'est un fichier auxiliaire pour interfacer avec munin. Je l'ai converti vers Python 3, mais je ne sais pas si on s'en sert tel que c'est décrit dans la doc de django_munin.
  • Est-ce qu'on clarifie quelque part que la licence GPL ne s'applique pas à tout, mais seulement à ce qui est n'est pas indiqué autrement ? Parce que j'y pense pour le code de django_munin, mais on a d'autres petits bouts avec des licences différentes (les smileys, de mémoire).

Contrôle qualité

Tester les différentes routes munin (voir urls.py pour savoir lesquelles).

Ça vaudra aussi le coup de tester sur un déploiement bêta je pense, pour voir si ça colle toujours avec le serveur munin.

@Arnaud-D Arnaud-D added the C-Back Concerne le back-end Django label Jan 20, 2024
@coveralls
Copy link

coveralls commented Jan 20, 2024

Coverage Status

coverage: 88.71% (-0.1%) from 88.827%
when pulling 0bffb12 on Arnaud-D:integration_django_munin
into 726ada0 on zestedesavoir:dev.

@Arnaud-D Arnaud-D force-pushed the integration_django_munin branch 2 times, most recently from 040d1c1 to 914a0fa Compare January 21, 2024 17:27
@Arnaud-D Arnaud-D marked this pull request as ready for review January 21, 2024 17:27
Copy link
Member

@philippemilink philippemilink left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Migration bizarre

Oui, j'avais déjà remarqué qu'il y avait un truc pas net...

Fichier django_munin/plugins/django.py

Je l'avais déjà intégré dans le dépôt ansible-zestedesavoir en le convertissant à Python 3 quand j'ai déployé Munin, en notant qu'il faudrait le copier depuis zds-site quand il y sera. Donc oui, tu peux le laisser là, vérifie juste qu'il n'y a pas (trop) de différence entre ta version et la mienne.

Préciser la licence quelque part

Tu peux ajouter un paragraphe au fichier LICENSE à la racine du dépôt. Et ouvrir un ticket pour qu'on pense un jour à y ajouter les licenses des autres ressources que tu mentionnes.

zds/munin/tests.py Show resolved Hide resolved
zds/settings/abstract_base/django.py Show resolved Hide resolved
@Arnaud-D
Copy link
Contributor Author

Fichier django_munin/plugins/django.py

[...] Donc oui, tu peux le laisser là, vérifie juste qu'il n'y a pas (trop) de différence entre ta version et la mienne.

Il y a quelques menues différences. Beaucoup dans "l'en-tête" que je ne comprends pas vraiment et dont je ne peux pas juger l'impact. Il y en a deux autres avec des encode/decode différents d'un côté et de l'autre (pour celles-ci, l'impact me semble faible).

Préciser la licence quelque part

Tu peux ajouter un paragraphe au fichier LICENSE à la racine du dépôt. Et ouvrir un ticket pour qu'on pense un jour à y ajouter les licenses des autres ressources que tu mentionnes.

C'est fait.

Sinon, j'ai répondu aux commentaires plus haut.

@Arnaud-D Arnaud-D force-pushed the integration_django_munin branch from 914a0fa to 9987482 Compare February 17, 2024 18:57
]

operations = [
migrations.CreateModel(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Je viens de tester la migration, et en effet, en l'état actuel de la PR, elle ne passe pas parce que la table existe déjà. En ajoutant l'opération RunSQL suivante, ça fonctionne. Tu peux l'ajouter, stp ?

Suggested change
migrations.CreateModel(
migrations.RunSQL("DROP TABLE IF EXISTS munin_test"),
migrations.CreateModel(

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

J'ai fait la modif

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Désolé, j'aurais pu le dire lors de ma revue de code initiale. Est-ce que tu peux remplacer ce fichier, par celui qui est dans le dépôt ansible-zestedesavoir : https://github.com/zestedesavoir/ansible-zestedesavoir/blob/main/roles/munin/files/plugins/django.py, stp ? Puisque c'est celui qui est déjà utilisé en production, on est sûr qu'il fonctionnera correctement (j'ai regardé un peu les différences, je ne suis pas sûr que la version du fichier actuellement dans la PR fonctionnement exactement comme la version dans ansible-zestedesavoir).

Dès que c'est fait, je fusionne.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

C'est fait.

@philippemilink philippemilink merged commit 7a83d90 into zestedesavoir:dev Feb 24, 2024
12 checks passed
@Arnaud-D Arnaud-D deleted the integration_django_munin branch February 24, 2024 18:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-Back Concerne le back-end Django
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants