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

Passage à Django 1.9 #3245

Closed
wants to merge 65 commits into from
Closed

Passage à Django 1.9 #3245

wants to merge 65 commits into from

Conversation

gustavi
Copy link
Contributor

@gustavi gustavi commented Dec 16, 2015

Suite de #3235 avec un rebase et plus propre

Q R
Correction de bugs ? non
Nouvelle Fonctionnalité ? oui
Tickets (issues) concernés #3214

QA

Tester absolument tout.

Travail qu'il reste à faire pour passer à 1.9

@gustavi gustavi added the C-Back Concerne le back-end Django label Dec 16, 2015
@GerardPaligot
Copy link
Member

Laisser tomber "Tests en parallèle" me semble être un choix judicieux dans cette mise à jour déjà énorme.

@artragis
Copy link
Member

+1 @GerardPaligot
j'ajouterai que tutorialv2 a été migré

@gustavi
Copy link
Contributor Author

gustavi commented Dec 16, 2015

Dans la mesure où c'est très simple à mettre en place (une dépendance à ajouter) si ça passe je mettre cette dépendance sinon on oublie.

@artragis : migré ?

@artragis
Copy link
Member

bah je veux dire "rendu compatible django 9)

Le 16 décembre 2015 à 11:56, Laville Augustin notifications@github.com a
écrit :

Dans la mesure où c'est très simple à mettre en place (une dépendance à
ajouter) si ça passe je mettre cette dépendance sinon on oublie.

@artragis https://github.com/artragis : migré ?


Reply to this email directly or view it on GitHub
#3245 (comment)
.

@gustavi
Copy link
Contributor Author

gustavi commented Dec 16, 2015

Super alors \o/

@artragis
Copy link
Member

Notons qu'il semblerait que j'ai ajouté une erreur dans les forum en ajoutant une migration qui était marquée comme manquante chez moi mais qui était déjà ajoutée. La 004 des forum peut être supprimée.

@gustavi
Copy link
Contributor Author

gustavi commented Dec 16, 2015

Fait, merci.

@gustavi
Copy link
Contributor Author

gustavi commented Dec 26, 2015

Pas de nouvelle ici, j'ai contacté 2 auteurs par mail pour qu'ils mergent mes PR. Je pense que les projets ne sont pas très actifs avec les fêtes.

@DevHugo
Copy link
Contributor

DevHugo commented Jan 1, 2016

Je vais tenter de te faire une PR dans le mois pour passer à la prochaine version de Haystack comme ça, quand il merge le fix pour django 1.9, on pourra continuer la migration.

@DevHugo
Copy link
Contributor

DevHugo commented Jan 1, 2016

De plus, je crois que dans la recherche, on fait pas la différence entre les catégories et les tags, faudrait harmoniser tout ça avec le reste mais pour ça, je ne sais pas si j'aurais le temps.

@gustavi
Copy link
Contributor Author

gustavi commented Jan 1, 2016

@DevHugo super, je vais essayer de bosser un peu sur ça début janvier, ça semble bouger un peu côté dépendances. Pour les tags et les catégories faudra attendre la ZEP-25 non ?

Note : màj de crispy à venir début janvier avec le fix qui ne manque

@DevHugo
Copy link
Contributor

DevHugo commented Jan 1, 2016

Ouai, je pourrais faire une PR sur ta branche zep_25, le souci est que je peux pas te garantir ni de date, ni de faisabilité et je veux pas que tu sois bloquer à cause de moi. Enfait, j'ai parcouru et non lu, les specs, je ne sais même pas la quantité de travail que ça demande. J'ai pas du tout le temps libre en se moment. Je tente et je fais au mieux ^^ !

@artragis
Copy link
Member

artragis commented Jan 3, 2016

faudrait un rebase pour que ça prenne en compte le fait qu'on n'a plus les articles et tuto du vieux module, ça te simplifiera la tâche.

@gustavi
Copy link
Contributor Author

gustavi commented Jan 5, 2016

Bon alors pas mal d'améliorations ici.

Plus d'erreurs sur les tests. Il reste des fails mais c'est en cours de correction.

Les tests en parallèle ça sera vraiment du bonus et uniquement si ça passe sur Travis (c'est buggé chez moi).

Pour les dépendances on en a encore 7 qui ne sont pas officiellement compatibles dont :

  • BLOQUANT, un peu dans l'impasse : Django-Haystack et django-cors-headers
  • À VENIR dans une prochaine version : Django-Cripsy-Form, django-model-utils et django-filter
  • SEMBLE ÊTRE OK, mais c'est pas vraiment officiel : drf-extensions et django-rest-swagger

@DevHugo
Copy link
Contributor

DevHugo commented Jan 6, 2016

J'ai peur pour la librairie django-cors-headers, pas de commit depuis 9 mois, plus aucune réponse dans les PR, ça me semble très très louche.

@SpaceFox
Copy link
Contributor

SpaceFox commented Jan 6, 2016

On a des solutions de secours à django-cors-headers ?

@artragis
Copy link
Member

artragis commented Jan 6, 2016

développer non même?N

2016-01-06 8:30 GMT+01:00 SpaceFox notifications@github.com:

On a des solutions de secours à django-cors-headers ?


Reply to this email directly or view it on GitHub
#3245 (comment)
.

@pierre-24
Copy link
Member

développer non même?

Vu comment ça à l'air employé quand on fait une recherche, si on maintiens ça nous même, on va se manger des requêtes de plein de devs'

@artragis
Copy link
Member

artragis commented Jan 6, 2016

a priori il existe une méthode simple pour le court terme : utiliser le fork de gustavi.

Ensuite, à l'association de voir, c'est la "beauté" du monde opensource.

@pierre-24
Copy link
Member

a priori il existe une méthode simple pour le court terme : utiliser le fork de gustavi.

Ben si on a pas le choix ...

@DevHugo
Copy link
Contributor

DevHugo commented Jan 6, 2016

L'utilisation qu'on fait de la librairie est faible. On se contente de dire
"ouai, rajoute moi deux headers toujours les mêmes aux réponses et autorise
que les headers suivant dans les demandes". Si on veut pas forker le
projet, on pourrait faire un middleware comme le recommande la doc django
framework rest, en remplacement de la librairie.

Sinon, on a déjà forker un projet, ça c'est bien passé donc je pense qu'il
faut garder cette expérience dans notre réflexion.

Tu en pense que c'est faisable avec un middleware @andr0 ?

@GerardPaligot
Copy link
Member

La bibliothèque django-cors-headers n'est qu'un middleware finalement. Ca ne couterait pas grand chose d'avoir soit notre fork, soit un middleware dans ZdS inspiré directement de la bibliothèque. Checkez le code de la dépendance, ça tient dans une centaine de lignes.

Sinon, c'est qui Andr0 ? ^^

@aesteve
Copy link

aesteve commented Jan 6, 2016

class CorsMiddleware(object):


    def process_response(self, request, response):
        origin = request.META.get('HTTP_ORIGIN')
        if self.is_enabled(request) and origin:
            # todo: check hostname from db instead
            url = urlparse(origin)

            if settings.CORS_MODEL is not None:
                model = get_model(*settings.CORS_MODEL.split('.'))
                if model.objects.filter(cors=url.netloc).exists():
                    response[ACCESS_CONTROL_ALLOW_ORIGIN] = origin

            if (not settings.CORS_ORIGIN_ALLOW_ALL and
                    self.origin_not_found_in_white_lists(origin, url)):
                return response

            response[ACCESS_CONTROL_ALLOW_ORIGIN] = "*" if (
                settings.CORS_ORIGIN_ALLOW_ALL and
                not settings.CORS_ALLOW_CREDENTIALS) else origin

            if len(settings.CORS_EXPOSE_HEADERS):
                response[ACCESS_CONTROL_EXPOSE_HEADERS] = ', '.join(
                    settings.CORS_EXPOSE_HEADERS)

            if settings.CORS_ALLOW_CREDENTIALS:
                response[ACCESS_CONTROL_ALLOW_CREDENTIALS] = 'true'

            if request.method == 'OPTIONS':
                response[ACCESS_CONTROL_ALLOW_HEADERS] = ', '.join(
                    settings.CORS_ALLOW_HEADERS)
                response[ACCESS_CONTROL_ALLOW_METHODS] = ', '.join(
                    settings.CORS_ALLOW_METHODS)
                if settings.CORS_PREFLIGHT_MAX_AGE:
                    response[ACCESS_CONTROL_MAX_AGE] = \
                        settings.CORS_PREFLIGHT_MAX_AGE

        return response

Cette partie là doit suffire, non ?

@GerardPaligot
Copy link
Member

Aux quelques dépendances près et théoriquement, oui.

@gustavi
Copy link
Contributor Author

gustavi commented Jan 6, 2016

Je serai pour utiliser mon fork temporairement car je n'ai changé que 4 lignes.

Dans un second temps on relance le dev une ou deux fois et sans nouvelle dans un mois on prend une décision : on fork ou on inclus le strict minimum qu'on utilise dans notre projet.

@SpaceFox ça te va ?

@SpaceFox
Copy link
Contributor

SpaceFox commented Jan 6, 2016

Ça me paraît bien.

PS : idéalement, on rappatrie ton fork sur le compte de ZdS.

@GerardPaligot
Copy link
Member

PS : idéalement, on rappatrie ton fork sur le compte de ZdS.

C'est l'idée que j'avais aussi et je plussoie. Faudrait juste que ton fork passe sur Travis parce que c'est pas le cas de ta PR (sur le projet django-cors-header).

@gustavi
Copy link
Contributor Author

gustavi commented Jan 6, 2016

J'ai rajouté les informations sur les Warning et crée un PR pour django-munin.

@SpaceFox tu peux fork le projet et me donner les droits ? Je fais une PR prope des que c'est bon :)

@gustavi
Copy link
Contributor Author

gustavi commented Jan 16, 2016

Bon alors bonne nouvelle : on peut tester ça !!

Ils reste des dépendances temporairement pas full OK dans le dernière version mais là ça passe.

C'est rebase, tout beau tout propre.

# full compatibility with django 1.8 and 1.9
django-crispy-forms==1.6.0
# TODO django-haystack
https://github.com/gustavi/django-haystack/archive/django_19.zip
Copy link
Member

Choose a reason for hiding this comment

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

Ca vient d'où ? Comment pouvons-nous savoir que c'est pas une dépendance foireuse ? :/

Copy link
Member

Choose a reason for hiding this comment

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

ça vient de "haystack est trop lent à réagir alors on utilise
temporairement le dépôt de gustavi".
En tout cas c'est approuvé par @SpaceFox.

Le 17/01/2016 17:26, Gérard Paligot a écrit :

In requirements.txt
#3245 (comment):

coverage==4.0.1
-django-crispy-forms==1.4.0
-django-haystack==2.4.1
-django-model-utils==2.2
-django-munin==0.2.0
+# full compatibility with django 1.8 and 1.9
+django-crispy-forms==1.6.0
+# TODO django-haystack
+https://github.com/gustavi/django-haystack/archive/django_19.zip

Ca vient d'où ? Comment pouvons-nous savoir que c'est pas une
dépendance foireuse ? :/


Reply to this email directly or view it on GitHub
https://github.com/zestedesavoir/zds-site/pull/3245/files#r49948467.

Copy link
Member

Choose a reason for hiding this comment

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

Je ne vois aucune PR de @gustavi sur le dépôt de haystack. S'il a une version compatible Django 1.9, ça serait bien de faire la PR au moins et pour qu'on puisse voir que tout est au vert sur la dépendance.

Copy link
Member

Choose a reason for hiding this comment

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

j'ai tendance à dire que :
django-haystack/django-haystack#1277 est la
bonne chose

Le 17/01/2016 17:31, Gérard Paligot a écrit :

+# TODO django-haystack

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Il n'y a aucun risque pour ZdS ?

Copy link
Member

Choose a reason for hiding this comment

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

Si c'est rouge, c'est sans doute pas pour faire jolie. @SpaceFox tu es vraiment ok avec ça ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Non, la modification doit être propre même si elle est de notre côté.

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'attends qu'il merge ma PR mais c'est pas un violent lui non plus.

En tous cas la recherche fonctionne. Sinon tout le reste est testable alors go QA.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ping le la ou tu as fait la PR. On reçoit pas de mail de mémoire quand on a une pr sur son repo.

acdha self-assigned this 3 hours ago
acdha added this to the v2.5.0 milestone 3 hours ago

acdha c'est le mainteneur principal du projet de django-haystack

@vhf
Copy link
Contributor

vhf commented Jan 21, 2016

@GerardPaligot
Copy link
Member

Si je regarde la situation actuelle de cette PR, nous avons plusieurs points bloquants :

  • Travis ne passent toujours pas.
  • Des dépendances qui ne supportent pas encore Django 1.9, qui ne semblent pas évoluer du tou et depuis plusieurs jours/semanes.

Est-ce qu'il ne serait pas plus pertinent de descendre à Django 1.8 qui est encore maintenue, qui est supportée par toutes les dépendances manquantes et qui pourrait aider au merge de cette branche dans la branche dev.

Qu'est ce que vous en pensez ?

@gustavi
Copy link
Contributor Author

gustavi commented Jan 22, 2016

La seule dépendance qui fait chier c'est django-haystack. Le reste ça relève du warning.

@DevHugo
Copy link
Contributor

DevHugo commented Jan 22, 2016

On peut commencer à QA sans et passer le code en béta. Peut-être que cela avancé du côté de haystack d'ici là.

@gustavi
Copy link
Contributor Author

gustavi commented Jan 22, 2016

C'est exactement ce que j'ai dit il y a 6 jours.

@DevHugo
Copy link
Contributor

DevHugo commented Jan 22, 2016

Ma manière détourné de relancer le sujet en disant pas juste +1.

@DevHugo
Copy link
Contributor

DevHugo commented Apr 20, 2016

Les gars, haystack est compatible 1.9 o/

@SpaceFox
Copy link
Contributor

Il reste quoi d'autre d'incompatible ?

@gustavi
Copy link
Contributor Author

gustavi commented Apr 20, 2016

Je regarde ça la semaine prochaine OK ?

@gustavi
Copy link
Contributor Author

gustavi commented May 3, 2016

Juste pour signaler que toutes les dépendances sont compatibles mais n'ont pas toutes une version publiée sur Pypi. Je fais une PR quand ça sera le cas et j'essaye de relancer régulièrement les auteurs.

@GerardPaligot
Copy link
Member

En attendant, on peut penser à travailler sur la compatibilité Python 3. Rien ne presse de ce côté ci.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bloquant Ticket qui doit être traité avant la prochaine mise à jour C-Back Concerne le back-end Django
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants