do not ignore package-lock.json

[jtnord]

the package-lock.json is vital for the build reproducibility. Without this what is used for the build is non deterministic (withouth actually hard locking all dependencies in package.json which is not the case here)

this removes the ignore and checks in a package-lock.json that was created from building a clean checkout

see jenkinsci/bom#2121 (comment)

amends #219

Testing done

Submitter checklist

• Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
• Ensure that the pull request title represents the desired changelog entry
• Please describe what you did
• Link to relevant issues in GitHub or Jira
• Link to relevant pull requests, esp. upstream and downstream changes
• Ensure you have provided tests - that demonstrates feature works or fixes the issue

[codecov]

Codecov Report

Merging #220 (08fd5fe) into master (a9178c4) will not change coverage.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master     #220   +/-   ##
=========================================
  Coverage     93.33%   93.33%           
  Complexity        9        9           
=========================================
  Files             2        2           
  Lines            15       15           
=========================================
  Hits             14       14           
  Misses            1        1           

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[uhafner]

This introduces changes in the local workspace just by building the project:

index e85dcd1..385b18e 100644
--- a/package-lock.json
+++ b/package-lock.json
@@ -1260,9 +1260,9 @@
       "dev": true
     },
     "node_modules/electron-to-chromium": {
-      "version": "1.4.416",
-      "resolved": "https://registry.npmjs.org/electron-to-chromium/-/electron-to-chromium-1.4.416.tgz",
-      "integrity": "sha512-AUYh0XDTb2vrj0rj82jb3P9hHSyzQNdTPYWZIhPdCOui7/vpme7+HTE07BE5jwuqg/34TZ8ktlRz6GImJ4IXjA==",
+      "version": "1.4.417",
+      "resolved": "https://registry.npmjs.org/electron-to-chromium/-/electron-to-chromium-1.4.417.tgz",
+      "integrity": "sha512-8rY8HdCxuSVY8wku3i/eDac4g1b4cSbruzocenrqBlzqruAZYHjQCHIjC66dLR9DXhEHTojsC4EjhZ8KmzwXqA==",
       "dev": true
     },
     "node_modules/emoji-regex": {
@@ -4054,9 +4054,9 @@
       }
     },
     "node_modules/path-scurry/node_modules/lru-cache": {
-      "version": "9.1.1",
-      "resolved": "https://registry.npmjs.org/lru-cache/-/lru-cache-9.1.1.tgz",
-      "integrity": "sha512-65/Jky17UwSb0BuB9V+MyDpsOtXKmYwzhyl+cOa9XUiI4uV2Ouy/2voFP3+al0BjZbJgMBD8FojMpAf+Z+qn4A==",
+      "version": "9.1.2",
+      "resolved": "https://registry.npmjs.org/lru-cache/-/lru-cache-9.1.2.tgz",
+      "integrity": "sha512-ERJq3FOzJTxBbFjZ7iDs+NiK4VI9Wz+RdrrAB8dio1oV+YvdPzUEE4QNiT2VD51DkIbCYRUUzCRkssXCHqSnKQ==",
       "dev": true,
       "engines": {
         "node": "14 || >=16.14"
@@ -7352,9 +7352,9 @@
       "dev": true
     },
     "electron-to-chromium": {
-      "version": "1.4.416",
-      "resolved": "https://registry.npmjs.org/electron-to-chromium/-/electron-to-chromium-1.4.416.tgz",
-      "integrity": "sha512-AUYh0XDTb2vrj0rj82jb3P9hHSyzQNdTPYWZIhPdCOui7/vpme7+HTE07BE5jwuqg/34TZ8ktlRz6GImJ4IXjA==",
+      "version": "1.4.417",
+      "resolved": "https://registry.npmjs.org/electron-to-chromium/-/electron-to-chromium-1.4.417.tgz",
+      "integrity": "sha512-8rY8HdCxuSVY8wku3i/eDac4g1b4cSbruzocenrqBlzqruAZYHjQCHIjC66dLR9DXhEHTojsC4EjhZ8KmzwXqA==",
       "dev": true

This is something I would like to avoid, therefore I banned this file from the repository with a good reason. I now pinned all versions in the package.json, so errors as mentioned in jenkinsci/bom#2121 should not occur anymore.

[jtnord]

This introduces changes in the local workspace just by building the project:

because something was just released - these should not cause the depds to be updated but the defined ones to be used. Will look at the project setup

I now pinned all versions in the package.json

not if the package-lock.json has changed when you build the exact same sources that I did :)

[jtnord]

The Build was using npm install unconditionally which does not honour the package-lock file and hence allows builds that are not repeatable.

Switched to npm ci which will use the exact versions of the dependencies

see https://docs.npmjs.com/cli/v9/commands/npm-ci and https://docs.npmjs.com/cli/v9/configuring-npm/package-lock-json

this will make the build slower for devs as node_modules will be wiped out - it is left as an excersize for someone else to choose if this is not wanranted and local builds should behave differently to CI by introducing a profile, however as release builds are not performed in CI this has currently not been done and I do not beleive it should not be done unless release builds also use npm ci or releases are performed only in a I context (where a ci profile would be active).

It may be that we do not need npm ci here but just npm install the changes where due to the file being wiped out in clean (I tested with mvn clean package and saw differences with npm install however this was because the file was removed and re-gernated.
@scherler care to add your expertize here (npm install vs npm ci when a package-lock.json file exists)

[scherler]

The ci command is basically a install which whips out the npm folder before installing the dependencies. It will fail in certain conditions https://docs.npmjs.com/cli/v9/commands/npm-ci

• The project must have an existing package-lock.json or npm-shrinkwrap.json.
• If dependencies in the package lock do not match those in package.json, npm ci will exit with an error, instead of updating the package lock.
• npm ci can only install entire projects at a time: individual dependencies cannot be added with this command.
• If a node_modules is already present, it will be automatically removed before npm ci begins its install.
• It will never write to package.json or any of the package-locks: installs are essentially frozen.

Basically the command is to make sure all versions of dependencies are the same as in the lock that makes sure that if you forget to commit the updated lock after you changed the package.json that the build will fail. It's designed to run on CI since in local dev you normally do not want your npm folder to be whipped every time.

[uhafner]

Seems to work as expected with the new changes in pom.xml now.

[jtnord]

Seems to work as expected with the new changes in pom.xml now.

if you think you have fixed it then you can and should unignore the lock file. if you ever see changes in that lock file then it is not fixed (and if you manually update dependencies in the package file you should regenerate the lock file).

As the lock file is still ignored there is no proof I can see that is indeed fixed. basically the build is not repeatable without a lock file - unless you specify a non floating version of absolutely everything (including all transitive deps) in the package file.