feat: Add make update cmd
#14652
feat: Add make update cmd
#14652
Conversation
Codecov Report
@@ Coverage Diff @@
## master #14652 +/- ##
=======================================
Coverage 77.54% 77.54%
=======================================
Files 958 958
Lines 48541 48541
Branches 5702 5702
=======================================
Hits 37640 37640
Misses 10700 10700
Partials 201 201
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report at Codecov.
|
Makefile
Outdated
| @@ -38,6 +38,22 @@ superset: | |||
| # Load some data to play with | |||
| superset load-examples | |||
|
|
|||
| update: | |||
There was a problem hiding this comment.
Maybe name this something like update-npm and then only do the final step, as the install will bump versions in the lockfile:
cd superset-frontend
npm install
There was a problem hiding this comment.
I believe npm install will only install what's in the lockfile (when given no args). npm update will bump versions.
(to be clear I do not think we should be running npm update)
There was a problem hiding this comment.
I would consider splitting FE and BE related functionality into different commands that can be run independent of one another. Then, we can also have a command that runs both commands, e.g., update-all: update-be update-fe.
There was a problem hiding this comment.
npm ci will only install what's in the lockfile. install will bump patches. At least it touches the lockfile :)
There was a problem hiding this comment.
npm ci will reinstall everything in the lockfile (essentially wiping node_modules and then installing fresh packages). npm install will instal the missing dependencies in node_modules, if there's no lockfile one will be created, if there is a lockfile it will be adhered to.
There was a problem hiding this comment.
Though, reading through the issue it appears this behavior has been patched in recent versions of npm@^5.4.2 and it behaves as I have described above
npm/npm#17979 (comment)
There was a problem hiding this comment.
Ha ha.. so I've still been seeing updated package-lock.json files in PRs that shouldn't be there, but.. I tested it out with v 7.6.0 and pulled Superset 1.0.0 and ran npm install and it updated the lockfile. :(
Either way, we can leave this as is, but if we do, I think we should be extra vigilant about sneaky tailgaiting package-lock files in PRs. Technically it should do no harm to have them updated, if the package is using semver correctly, but it just creates more ambiguity and room for potential bugs if it's there for no seemingly good reason.
There was a problem hiding this comment.
I'll still continue to recommend that people use npm ci when updating.
There was a problem hiding this comment.
I've seen the same but I think that's due to slight differences in npm version. I haven't seen actual package version changes in the lockfile, it seems like it's usually formatting changes or updates to the integrity sha. I think it would be valuable to lock down the version of node & npm we are all using to a specific version and update these scripts to ensure the correct version of node/npm is being used.
* setup of makefile * lit * update makefile
* setup of makefile * lit * update makefile
* setup of makefile * lit * update makefile
mistercrunch
added
🏷️ bot
SUMMARY
Command that reinstalls all your python and js dependencies
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TEST PLAN
ADDITIONAL INFORMATION