Adding ability to run npm ci by default #236
Conversation
|
Can one of the admins verify this patch? |
5 similar comments
|
Can one of the admins verify this patch? |
|
Can one of the admins verify this patch? |
|
Can one of the admins verify this patch? |
|
Can one of the admins verify this patch? |
|
Can one of the admins verify this patch? |
|
[test] |
10/s2i/bin/assemble
Outdated
| echo "---> npm ci failed, installing dependencies with npm install"; \ | ||
| npm install | ||
|
|
||
| NODE_ENV=development |
There was a problem hiding this comment.
| NODE_ENV=development |
10/s2i/bin/assemble
Outdated
| # npm ci will fail if certain conditions aren't met as mentioned in | ||
| # https://docs.npmjs.com/cli-commands/ci.html#description | ||
| echo "---> npm ci failed, installing dependencies with npm install"; \ | ||
| npm install |
There was a problem hiding this comment.
| npm install | |
| NODE_ENV=development npm install |
|
@phracek that is correct will push updated PR. Also is there any changes that I have to make for the test to be successful? |
|
[test] |
|
@phracek can the test failure be because of |
|
@aksgupta14 ---> Installing dependencies with npm ci
�[91mnpm ERR! cipm can only install packages with an existing package-lock.json or npm-shrinkwrap.json with lockfileVersion >= 1. Run an install with npm@5 or later to generate it, then try again.
�[0m�[91m
npm ERR! A complete log of this run can be found in:
npm ERR! /opt/app-root/src/.npm/_logs/2020-04-27T08_37_33_712Z-debug.log
�[0m---> npm ci failed, installing dependencies with npm install |
|
@phracek after going through the test cases, I was able to figure out the issue. The issue appears only with incremental build and the reason being Now when we will use |
|
Can one of the admins verify this patch? |
|
I will investigate it a bit more after a couple days. Please be patient. |
|
@phracek Are there any updates on this feature? |
|
I am also looking for that capability, is there a plan to make npm ci avalable in the S2I? |
|
[test-all] |
|
This PR seems to make the change for Node.js versions 10 and 12 which are no longer supported. While I can see the change with a working fallback makes sense to me I'd suggest it should only be made for the currently supported LTS versions. This is probably because it was submitted many years ago. At this point I think we should likely only do for 16 so that we minimize risk to people already using the containers. |
|
hi folks, friendly ping here about this PR. We found a situation that maybe this feature can help us: This error is not happening when using the version 14. |
|
@aksgupta14 are you still willing/interested in getting this feature added? I know this is quite old but looking to see how we move it forward. |
|
@aksgupta14 Can you please update this pull request? There are several conflicts. |
Pull Request validationFailed🔴 Review - Missing review from a member (2 required) Success🟢 CI - All checks have passed |
|
@aksgupta14 The new versions also are present. Please rebase and update it. |
Making
npm cito run by default and if it fails for any reason we will runnpm installas a backup, this will prevent build from failure for developers who doesn't use or updatepackage-lock.json.This is the one way and another way is to use variable
RUN_NPM_CIlike we useNODE_ENV, but that would require existing developers to update theirbuild_templateand for new developers who are not aware of this env variable won't be able to use this approach.Please refer below docs for more information:
npm ci
npm install
#212
@phracek please review.