PR#50409: Issue 50052 - Add package-lock.json and use "npm ci" - 389-ds-base

389-ds-base

#50409 Issue 50052 - Add package-lock.json and use "npm ci"

Closed 3 years ago by spichugi. Opened 4 years ago by spichugi.

spichugi/389-ds-base npm-lock into master

Issue 50052 - Add package-lock.json and use "npm ci"

Simon Pichugin • 4 years ago

255faf9

file added

		`@@ -0,0 +1,8 @@`
		`+ {`
		`+ "moderate": true,`
		`+ "package-manager": "auto",`
		`+ "report": true,`
		`+ "advisories": [],`
		`+ "_comment": "jquery should be removed from the whitelist after https://github.com/patternfly/patternfly/pull/1174 is merged",`
		`+ "whitelist": ["jquery"]`
		`+ }`

src/cockpit/389-console/node_modules.mk

file modified

+2 -2

		`@@ -1,8 +1,8 @@`
		`install: package.json`
		`- test -f node_modules/webpack \|\| npm install`
		`+ test -f node_modules/webpack \|\| npm ci`

		`build-cockpit-plugin: webpack.config.js`
		`- npm run build; cp -r dist cockpit_dist`
		`+ npm run audit-ci && npm run build && cp -r dist cockpit_dist`

		`eslint-fix:`
		`npm run eslint:fix`

src/cockpit/389-console/package-lock.json

file added

+9195

The added file is too large to be shown here, see it at: src/cockpit/389-console/package-lock.json

src/cockpit/389-console/package.json

file modified

+14 -8

		`@@ -2,11 +2,16 @@`
		`"version": "1.0.0",`
		`"name": "389-console",`
		`"description": "Cockpit plugin for managing 389 Directory Server",`
		`+ "repository": {`
		`+ "type": "git",`
		`+ "url": "https://pagure.io/389-ds-base.git"`
		`+ },`
		`"author": "",`
		`"license": "GPL-3.0-or-later",`
		`"main": "index.js",`
		`"scripts": {`
		`"build": "webpack",`
		`+ "audit-ci": "audit-ci --config audit-ci.json",`
		`"eslint": "eslint --ext .jsx --ext .es6 src/",`
		`"eslint:fix": "eslint --fix --ext .jsx --ext .es6 src/"`
		`},`
		`@@ -15,12 +20,13 @@`
		`"@babel/preset-env": "^7.0.0",`
		`"@babel/preset-react": "^7.0.0",`
		`"ajv": "^6.0.0",`
		`+ "audit-ci": "^1.7.0",`
		`"babel-eslint": "^9.0.0",`
		`"babel-loader": "^8.0.0",`
		`"chrome-remote-interface": "^0.25.5",`
		`"compression-webpack-plugin": "^1.1.11",`
		`"copy-webpack-plugin": "^4.5.2",`
		`- "css-loader": "^0.28.11",`
		`+ "css-loader": "^2.1.1",`
		`"eslint": "^5.4.0",`
		`"eslint-config-standard": "^11.0.0",`
		`"eslint-config-standard-react": "^6.0.0",`
		`@@ -42,16 +48,16 @@`
		`"webpack-cli": "^3.1.0"`
		`},`
		`"dependencies": {`
		`- "patternfly": "3.58.0",`
		`- "patternfly-react": "2.24.5",`
		`- "bootstrap": "4.2.1",`
		`+ "bootstrap": "^4.3.1",`
		`"node-sass": "4.11.0",`
		`+ "patternfly": "^3.59.1",`
		`+ "patternfly-react": "^2.34.3",`
		`+ "prop-types": "15.6.2",`
		`+ "react": "16.6.1",`
		`"react-bootstrap": "0.32.4",`
		`"react-bootstrap-typeahead": "3.2.4",`
		`- "react": "16.6.1",`
		`"react-dom": "16.6.1",`
		`- "prop-types": "15.6.2",`
		`- "table-resolver": "4.1.1",`
		`- "recompose": "0.30.0"`
		`+ "recompose": "0.30.0",`
		`+ "table-resolver": "4.1.1"`
		`}`
		`}`

spichugi commented 4 years ago

Bug description: All software changes incur some risk,
and it's critical to be able to manage this risk.
We can use a common way of dealing with it - npm-shrinkwrap.

Fix description: The suggested approach - npm-shrinkwrap - is an "overkill"
for our case. We don't need to publish the package on NPM.
It will be sufficient enough to use existing NPM functionality added in 5.7 version.

Replace npm install with npm ci which uses package-lock.json
and throws an error if any inconsistencies with pachage.json are found.
Add package-lock.json to the repo.
When we change the package.json content, a new pachage-lock.json should be
generated (using npm install) and the change should be commited.

https://pagure.io/389-ds-base/issue/50052

Reviewed by: ?

mhonek commented 4 years ago

LGTM

vashirov commented 4 years ago

I'd like to take this a bit further and incorporate npm audit-ci in our build process.
Currently we have known vulnerabilities in our dependency tree:

added 1082 packages from 741 contributors and audited 9442 packages in 22.219s
found 37 vulnerabilities (36 moderate, 1 high)
  run `npm audit fix` to fix them, or `npm audit` for details

audit-ci is similar to npm ci, but will stop the build if it finds vulnerabilities above configured level.

1 new commit added

Add audit-ci tool, fix audit issues, add "repository" field

4 years ago

spichugi commented 4 years ago

I'd like to take this a bit further and incorporate npm audit-ci in our build process.
Currently we have known vulnerabilities in our dependency tree:
added 1082 packages from 741 contributors and audited 9442 packages in 22.219s
found 37 vulnerabilities (36 moderate, 1 high)
run npm audit fix to fix them, or npm audit for details

audit-ci is similar to npm ci, but will stop the build if it finds vulnerabilities above configured level.

Nice catch! I've added it.
I just hope it won't get in a way of shipping RPMs on time (some small issue will pop up in the audit and it'll block the process).
For now, I've set the moderate vulnerability level but I think it's possible we will have to move it to high in the future.
What do you think?

2 new commits added

Add audit-ci tool, fix audit issues, add "repository" field
Issue 50052 - Add package-lock.json and use "npm ci"

4 years ago

mhonek commented 4 years ago

@spichugi Good thinking. However, I think the "hope" not being fulfilled might cost a lot in the future. Additionally this seems to me as going against reproducible build idea. Could we make the audit only a part of e.g. make check? Running that in nightly CI would result in much smoother experience, I believe.

mhonek commented 4 years ago

Disregard my previous comment. Seems like we run this before e.g. creating upstream tarball (thanks Simon for clarification), which is ok as we don't want to release known vulnerable code.

Once more, LGTM.

vashirov commented 4 years ago

LGTM too, thanks!

rebased onto 255faf9

4 years ago

Pull-Request has been merged by spichugi

4 years ago

spichugi commented 3 years ago

389-ds-base is moving from Pagure to Github. This means that new issues and pull requests
will be accepted only in 389-ds-base's github repository.

This pull request has been cloned to Github as issue and is available here:
- https://github.com/389ds/389-ds-base/issues/3467

If you want to continue to work on the PR, please navigate to the github issue,
download the patch from the attachments and file a new pull request.

Thank you for understanding. We apologize for all inconvenience.