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

Update des dépendances NPM #1398

Merged
merged 1 commit into from Sep 3, 2014
Merged

Conversation

sandhose
Copy link
Contributor

Q R
Correction de bugs ? non
Nouvelle Fonctionnalité ? non
Tickets concernés #1430

On avait pas mal de dépendances NPM qui étaient pas à jour (rien d'alarmant pour autant).

Y'a un service qui s'appelle David qui permet de voir l'état des dépendances, et ils fournissent un badge, si vous voulez l'intégrer dans le Readme: Dependency Status

Notes pour la QA

Pour tester cette PR, il suffit de mettre à jour les dépendances (npm install), de lancer le build gulp (gulp build), et de lancer l'app en vérifiant que le site fonctionne bien (en gros que les fichiers CSS/JS/Sprites se soient bien compilés)

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 36e8907 on sandhose:npm-update into 914da02 on zestedesavoir:dev.

@SpaceFox
Copy link
Contributor

SpaceFox commented Sep 3, 2014

Est-ce qu'on pourrait créer le ticket correspondant à cette PR ?

@SpaceFox SpaceFox added this to the Version 1.1 milestone Sep 3, 2014
@sandhose
Copy link
Contributor Author

sandhose commented Sep 3, 2014

Est-ce vraiment la peine ? Sérieusement, c'est juste une update des dépendances qui impacte que le build du front... Je comprends pas d'ailleurs qu'elle traîne autant, y'a absolument rien qui risque de complètement casser l'app, ou de poser problème...

S'il le faut, je peux la créer, mais je vois pas trop trop l'intérêt

@SpaceFox
Copy link
Contributor

SpaceFox commented Sep 3, 2014

Est-ce vraiment la peine ?

Oui. Pas de PR sans son ticket, pour pouvoir suivre ce qui a été fait de manière claire.

Je comprends pas d'ailleurs qu'elle traîne autant, y'a absolument rien qui risque de complètement casser l'app, ou de poser problème...

Mise à jour de dépendance = toujours un risque de casser quelque chose.

D'autre part, cette issue, aussi triviale soit-elle, suivra le même workflow que toutes les autres. Comme elle n'était pas indispensable à la v1.0, elle arrivera avec la v1.1, laquelle sera lancée dès que la PR #1400 le permettra.

@sandhose
Copy link
Contributor Author

sandhose commented Sep 3, 2014

Done

Sur le côté "non-indispensable à la v1.0" de cette PR, comme je l'ai dit dans l'issue #1430, la version de spritesmith qu'on utilise marche pas sur tous les systèmes, et donc peut potentiellement bloquer l'installation... C'est pas urgent non plus, mais bon

@Alex-D
Copy link
Contributor

Alex-D commented Sep 3, 2014

Aller zou, ça roule !

EDIT : je fais un message plus clair pour dire que oui ça passe la QA et que ça fonctionne, avant qu'on ne me dise "t'as pas testé".

Alex-D added a commit that referenced this pull request Sep 3, 2014
@Alex-D Alex-D merged commit 10cc8be into zestedesavoir:dev Sep 3, 2014
@sandhose sandhose deleted the npm-update branch September 3, 2014 16:46
@Alex-D
Copy link
Contributor

Alex-D commented Sep 3, 2014

Comme elle n'était pas indispensable à la v1.0
@SpaceFox

peut potentiellement bloquer l'installation...
@sandhose

Nous n'avons définitivement pas la même définition de "pas indispensable".

@SpaceFox SpaceFox modified the milestone: Version 1.1 Sep 24, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-Front Concerne l'interface du site
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants