Issue 29573912: Issue 5857 - Retry npm install on failure (Closed)
|
Created:
Oct. 11, 2017, 10:48 p.m. by tlucas Modified:
Oct. 13, 2017, 11:20 a.m. CC:
Vasily Kuznetsov, Wladimir Palant Visibility:
Public. |
DescriptionIssue 5857 - Retry npm install on failure
Patch Set 1 #
Total comments: 6
MessagesTotal messages: 7
Patch Set 1 Let ensure_dependencies.py retry `npm install` after it previously failed.
https://codereview.adblockplus.org/29573912/diff/29573913/ensure_dependencies.py File ensure_dependencies.py (right): https://codereview.adblockplus.org/29573912/diff/29573913/ensure_dependencies... ensure_dependencies.py:47: NPM_LOCKFILE = '.npm_install_lock' This file would have to be added to .hgignore/.gitignore, why not just putting it inside node_modules, which has to be ignored anyway?
https://codereview.adblockplus.org/29573912/diff/29573913/ensure_dependencies.py File ensure_dependencies.py (right): https://codereview.adblockplus.org/29573912/diff/29573913/ensure_dependencies... ensure_dependencies.py:47: NPM_LOCKFILE = '.npm_install_lock' On 2017/10/11 23:22:33, Sebastian Noack wrote: > This file would have to be added to .hgignore/.gitignore, why not just putting > it inside node_modules, which has to be ignored anyway? It is added to .hgignore/.gitignore, please see below. When "npm install" is run for the first time, there is no folder "node_modules" yet - and i did not like the idea to manually add a folder and a file - and only delete the file on failure.
https://codereview.adblockplus.org/29573912/diff/29573913/ensure_dependencies.py File ensure_dependencies.py (right): https://codereview.adblockplus.org/29573912/diff/29573913/ensure_dependencies... ensure_dependencies.py:47: NPM_LOCKFILE = '.npm_install_lock' On 2017/10/11 23:48:55, tlucas wrote: > On 2017/10/11 23:22:33, Sebastian Noack wrote: > > This file would have to be added to .hgignore/.gitignore, why not just putting > > it inside node_modules, which has to be ignored anyway? > > It is added to .hgignore/.gitignore, please see below. > When "npm install" is run for the first time, there is no folder "node_modules" > yet - and i did not like the idea to manually add a folder and a file - and only > delete the file on failure. Ah right, I see. Yeah, if we can just make sure that it gets ignored, it's probably not worth to deal with the scenario in which the node_modules folder doesn't exist yet. LGTM.
LGTM https://codereview.adblockplus.org/29573912/diff/29573913/ensure_dependencies.py File ensure_dependencies.py (right): https://codereview.adblockplus.org/29573912/diff/29573913/ensure_dependencies... ensure_dependencies.py:284: repo_types[vcs].ignore(os.path.join(target, NPM_LOCKFILE), target) I wonder if we should just ignore this file consistently, since we never un-ignore it again anyway?
https://codereview.adblockplus.org/29573912/diff/29573913/ensure_dependencies.py File ensure_dependencies.py (right): https://codereview.adblockplus.org/29573912/diff/29573913/ensure_dependencies... ensure_dependencies.py:284: repo_types[vcs].ignore(os.path.join(target, NPM_LOCKFILE), target) On 2017/10/13 10:14:23, kzar wrote: > I wonder if we should just ignore this file consistently, since we never > un-ignore it again anyway? This file should - in a best case scenario - never remain. IMHO an entry in .gitignore / .hgignore would only confuse people, if it never is created.
Message was sent while issue was closed.
https://codereview.adblockplus.org/29573912/diff/29573913/ensure_dependencies.py File ensure_dependencies.py (right): https://codereview.adblockplus.org/29573912/diff/29573913/ensure_dependencies... ensure_dependencies.py:284: repo_types[vcs].ignore(os.path.join(target, NPM_LOCKFILE), target) On 2017/10/13 10:31:36, tlucas wrote: > On 2017/10/13 10:14:23, kzar wrote: > > I wonder if we should just ignore this file consistently, since we never > > un-ignore it again anyway? > > This file should - in a best case scenario - never remain. IMHO an entry in > .gitignore / .hgignore would only confuse people, if it never is created. Yea, fair enough. 
|
||||||||||||||||||||||||||||||||||||||||||||||
