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

Met à jour les instructions du deploy.sh et de la doc concernant npm #1926

Merged
merged 4 commits into from
Jan 14, 2015

Conversation

Situphen
Copy link
Member

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

Attention, ça touche à la MEP !!!

Cette PR doit être mergée avant la MEP de la v1.5 !!

Suppression de sudo npm -q update bower gulp -g et remplacement de gulp pack par npm run gulp -- build car on est passé à npm v2 et la tâche "pack" n'existe plus.

Suppression du sudo de sudo npm -q update car il n'y a pas besoin normalement. Ainsi que suppression de l'option -q qui n'existe pas (cf la doc ; j'ai aussi testé et il n'y a aucune différence avec et sans).


Il faut voir la version de npm en production actuellement et la mettre à jour si besoin (sudo npm install -g npm) !

@firm1 firm1 added the unknown label Dec 18, 2014
sudo npm -q update bower gulp -g
gulp pack
npm -q update
npm run-script gulp pack
Copy link
Contributor

Choose a reason for hiding this comment

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

c'est npm run gulp -- pack (run fait la même chose que run-script ; il faut séparer les arguments npm des arguments du script par -- )

Et je suis pas sûr qu'on garde ce pack, peut-être passer sur build...

Copy link
Member Author

Choose a reason for hiding this comment

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

Même si ça fait la même chose, c'est ce qui est dans la documentation donc il vaut mieux rester cohérent, non ?

Ça ne coûte rien de garder le pack pour ceux qui ne voudraient pas installer les outils front !

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok ok ; par contre, il faut quand même corriger avec le -- 😊

Copy link
Member Author

Choose a reason for hiding this comment

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

J'ai regardé et effectivement il faut le --. Du coup, je met npm run gulp -- pack ici et je fais une autre PR pour mettre à jour la documentation ?

@sandhose
Copy link
Contributor

Je pense qu'il va falloir virer le dossier dist/ et le fichier assets/scss/_sprites.scss lors de la prochaine MEP pour éviter les conflits niveau permissions

@Situphen
Copy link
Member Author

Bah, sois on attend la MEP de la v1.4 et si ça bug on met en place ça, soit on le fait tout de suite.

@sandhose
Copy link
Contributor

@Situphen: Comment ca ?

Je viens de penser, il y aura sûrement le même problème pour les dossiers node_modules/ et assets/bower_components/, qui, vu qu'ils ont été créés en root, devraient être modifiables que par root... Donc soit un chmod de tout ça, soit il faut virer, et ça sera réinstallé par le script

@Situphen
Copy link
Member Author

Ah, je croyais que tu parlais du bug lors de la mise en préproduction de la v1.4 mais tu parles du passage de "sudo" à normal, au temps pour moi.

Du coup, le plus simple est de mettre à jour de la "update.md" pour que celui qui fait la MEP vide "node_modules/" et "dist/".

@Situphen Situphen force-pushed the maj_deploy_script branch 2 times, most recently from 9ea24ee to e13bb73 Compare December 18, 2014 18:37
@Situphen Situphen changed the title Met à jour les instructions du deploy.sh concernant npm Met à jour les instructions du deploy.sh et de la doc concernant npm Dec 18, 2014
@Situphen Situphen force-pushed the maj_deploy_script branch 5 times, most recently from f0d606f to b6958d2 Compare December 21, 2014 14:55
@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling b6958d2 on Situphen:maj_deploy_script into 149a065 on zestedesavoir:dev.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling b6958d2 on Situphen:maj_deploy_script into 149a065 on zestedesavoir:dev.

@Eskimon
Copy link
Contributor

Eskimon commented Dec 22, 2014

Travis passe avec ce script, on est bon pour merge ?

@SpaceFox
Copy link
Contributor

Faudrait que je prenne le temps de le relire en détail d'abord.

2014-12-22 9:23 GMT+01:00 Eskimon notifications@github.com:

Travis passe avec ce script, on est bon pour merge ?


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

@gustavi gustavi added C-Front Concerne l'interface du site and removed unknown labels Dec 22, 2014

- `clean`: Nettoie le dossier `dist/`
- `build`: Compile tout (CSS, JS, et images)
- `test`: Lance les tests (JSHint, ...)
- `watch`: Compile les différents fichiers dès qu'ils sont modifiés (utile pour le développement; `Ctrl+C` pour arrêter)

Si vous voulez utiliser directement la commande `gulp [tâche]` au lieu de `npm run-script gulp [tâche]`, il vous faut lancer cette commande avec les droits administrateurs :
Si vos modifications ne s'apparaissent pas dans votre navigateur et que ce n'est pas dû à Gulp, pensez à vider le cache de votre navigateur !
Copy link
Contributor

Choose a reason for hiding this comment

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

ne s'apparaissent pas

Ca veut rien dire :D

Copy link
Member Author

Choose a reason for hiding this comment

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

Tu as raison ! :D

@Situphen
Copy link
Member Author

Situphen commented Jan 6, 2015

J'ai mis à jour avec les commentaires de @Eskimon et j'ai supprimé le "update.md" car ce n'est plus d'actualité.

@Situphen
Copy link
Member Author

J'ai tout mis à jour !

@sandhose : Tu peux QA pour que l'on puisse merger ?

Quelqu'un sait pourquoi il y a l'argument -q pour la commande npm dans le script de déploiement ?

@Situphen Situphen force-pushed the maj_deploy_script branch 2 times, most recently from 583d18d to ca3012c Compare January 14, 2015 13:52
@sandhose
Copy link
Contributor

C'est bon pour moi, je merge

sandhose added a commit that referenced this pull request Jan 14, 2015
Met à jour les instructions du deploy.sh et de la doc concernant npm
@sandhose sandhose merged commit cacd13e into zestedesavoir:dev Jan 14, 2015
@Situphen Situphen deleted the maj_deploy_script branch April 4, 2015 12:02
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.

7 participants