should we remove save-artifacts step?
lance opened this issue · 6 comments
The restore-artifacts step of our assemble script which will restore npm dependencies is intended to make redeployments quicker because we don't need to resinstall the dependencies on every build. However, with the permission handling that openshift uses, and the fact that users may indeed be doing things during the run phase which need to write to the node_modules directory, this seems to be causing more problems than it's worth. For example: #92
I think we should consider removing this optimization from the builder image.
perhaps.
I wonder if we should try to see how npm ci fits into our builder,
https://docs.npmjs.com/cli/ci
https://blog.npmjs.org/post/171556855892/introducing-npm-ci-for-faster-more-reliable
I guess it replaces npm install and is a lot faster(?). I think you need to have a package-lock/shrinkwrap.json or it errors, but this might be an option to look into.
from the description:
This command is similar to npm-install, except it's meant to be used in automated environments such as test platforms, continuous integration, and deployment.
@lholmquist are you suggesting that we eliminate the [save|restore]-artifacts steps, and use npm ci in production mode - and leave dev mode alone? That sounds reasonable. I will play around with this.
maybe? i saw something shiny, and thought it might work for us.
And how much time is this really saving(the current save/restore artifact step). even if we didn't have it we are probably still way faster than any Java based build :)
if we start using npm ci or not, i would say, lets remove it.
@lholmquist we can't use npm ci until Node.js upstream bumps the npm version. Both 8.x and 10.x are still on 5.6, while npm ci was introduced in 5.7. https://github.com/npm/npm/releases/tag/v5.7.0
We can track upstream progress here: nodejs/node#20190. It's moving forward and will likely be in the next semver-minor release of 10.x, and should be backported to 8.x. I will open another issue for this item specifically, since it's really a bit tangential to the save-artifacts issue.
ah, right. i didn't actually look to see what the npm version was that started to include npm ci