LGTM!

Nice, thanks!

BTW, I find it rather irritating to commit package-lock.json. It's a lot of noise in commits and git, and not really necessary. The point of package-lock.json is that you generate it for building a tarball and ship it in releases. But your npm version constraints should be in package.json rather, so that you keep up with minor npm versions of your development dependencies as long as they match the specification in package.json.

BTW, I find it rather irritating to commit package-lock.json. It's a lot of noise in commits and git, and not really necessary. The point of package-lock.json is that you generate it for building a tarball and ship it in releases. But your npm version constraints should be in package.json rather, so that you keep up with minor npm versions of your development dependencies as long as they match the specification in package.json.

I didn't actually implement the change where we started committing the lock file changes. I recall it having to do with it resolving some packaging issues on RHEL or Fedora. @spichugi implemented this, can you elaborate why we did this Simon?

BTW, I find it rather irritating to commit package-lock.json. It's a lot of noise in commits and git, and not really necessary. The point of package-lock.json is that you generate it for building a tarball and ship it in releases. But your npm version constraints should be in package.json rather, so that you keep up with minor npm versions of your development dependencies as long as they match the specification in package.json.

As far as I understand it is exactly intended to be checked into source control.
It is even mentioned in the help.

https://github.com/npm/npm/blob/v5.0.0/doc/files/package-lock.json.md

"This file is intended to be committed into source repositories, and serves various purposes:

Describe a single representation of a dependency tree such that teammates, deployments, and continuous integration are guaranteed to install exactly the same dependencies.

Provide a facility for users to "time-travel" to previous states of node_modules without having to commit the directory itself.

To facilitate greater visibility of tree changes through readable source control diffs."

So it makes our package more secure and stable.
Yes, it looks not so beautiful but, I think, the ugliness is a lesser evil here. :)

@spichugi : Yes, that documentation page has it exactly wrong. "intended to be committed" and "it cannot be published" is utter bogus. You want to publish it on releases to guarantee that a release can be reproduced exactly, but for git development it's a rather poor replacement for specifying proper dependencies.

But shrug, I don't want to start a big discussion about it, I just wanted to mention that we don't do that in all other Cockpit packages.

@spichugi : Yes, that documentation page has it exactly wrong. "intended to be committed" and "it cannot be published" is utter bogus.

If I understand correctly, it talks about 'npm' publishing here - https://www.npmjs.com/package/ldap

And the end of the sentence - it cannot be published, and it will be ignored if found in any place other than the toplevel package - makes sense then.

So it is not published among other packages here - https://www.npmjs.com/package/ldap
And source repository - it is git. And the package-lock.json should be published in git if used.
As of this, I don't see the utter bogus here... If I understood it correctly.

You want to publish it on releases to guarantee that a release can be reproduced exactly, but for git development it's a rather poor replacement for specifying proper dependencies.

It helps to be with other developers on the same page while reproducing bugs while being on the same branch/HEAD.
And it is not a replacement it is an original npm feature which helps the CI process.

But shrug, I don't want to start a big discussion about it, I just wanted to mention that we don't do that in all other Cockpit packages.

Maybe we should...
More info can be found here - https://stackoverflow.com/questions/44206782/do-i-commit-the-package-lock-json-file-created-by-npm-5

P.S. of course, we don't want to commit the noise with other changes in the same commit. I understand it can be annoying. It is better to have special commits that contain package.json and package-lock.json changes.

@spichugi : Thanks for the interpretation -- in the context of publishing NPM modules on npmjs.org that makes much more sense indeed!

So let's just agree to disagree, and continue what we do (differently). Sorry, I didn't mean to start a hubbub here.

It is better to have special commits that contain package.json and package-lock.json changes.

Right, and you'll need lots of these PRs anyway (ideally by a bot), to keep up with version updates then, as you'd never automatically update to security patches and such. When keeping them separate, they are much less annoying.

@spichugi : Thanks for the interpretation -- in the context of publishing NPM modules on npmjs.org that makes much more sense indeed!
So let's just agree to disagree, and continue what we do (differently). Sorry, I didn't mean to start a hubbub here.

Sure, sorry if I was too 'pushy' :D

It is better to have special commits that contain package.json and package-lock.json changes.

Right, and you'll need lots of these PRs anyway (ideally by a bot), to keep up with version updates then, as you'd never automatically update to security patches and such. When keeping them separate, they are much less annoying.

We use audit-ci for that - https://www.npmjs.com/package/audit-ci
It notifies us if the build has any vulnerabilities so we can deal with them in the following ways:

a. Commit an updated package.json, package-lock.json pair and only then build RPM;
b. If some other package has the vulnerability in their dependencies (like patternly->jquerry) we file an issue to their tracker if it's not already present.

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/4252

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.