Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Check for non-standard NPM registry in package-lock.json as a step in CI #2876

Closed
datho7561 opened this issue Jan 12, 2023 · 3 comments · Fixed by #3134
Closed

Check for non-standard NPM registry in package-lock.json as a step in CI #2876

datho7561 opened this issue Jan 12, 2023 · 3 comments · Fixed by #3134

Comments

@datho7561
Copy link
Contributor

datho7561 commented Jan 12, 2023

Having references to the Red Hat internal npm repository in the package-lock.json prevents non-Red Hat developers from installing the vscode-java npm dependencies. As mentioned in #2874 (comment), npm will always use the configured repository to download packages, regardless of what's in the package-lock.json, although it will attempt to validate cached packages against what's in the package-lock.json.

It would be nice to check for the internal repo in package-lock.json as a part of the CI so that in the future we don't accidentally commit changes like this.

@fsouza
Copy link
Contributor

fsouza commented May 29, 2023

@datho7561 I just ran into this and opened this PR for a one-off removal: #3120.

In terms of checking it, do you have any preferences for the approach? Do you want to be able to point the line number or something like that? Or would a jq/grep powered check work? Something like:

% cat package-lock.json | jq -r '.dependencies|to_entries[].value.resolved|select(startswith("https://repository.engineering.redhat.com"))'
https://repository.engineering.redhat.com/nexus/repository/registry.npmjs.org/@types/sinon/-/sinon-10.0.13.tgz
https://repository.engineering.redhat.com/nexus/repository/registry.npmjs.org/ansi-styles/-/ansi-styles-2.2.1.tgz
https://repository.engineering.redhat.com/nexus/repository/registry.npmjs.org/chalk/-/chalk-1.1.3.tgz
https://repository.engineering.redhat.com/nexus/repository/registry.npmjs.org/escape-string-regexp/-/escape-string-regexp-1.0.5.tgz
https://repository.engineering.redhat.com/nexus/repository/registry.npmjs.org/has-ansi/-/has-ansi-2.0.0.tgz
https://repository.engineering.redhat.com/nexus/repository/registry.npmjs.org/minimist/-/minimist-1.2.6.tgz
https://repository.engineering.redhat.com/nexus/repository/registry.npmjs.org/supports-color/-/supports-color-2.0.0.tgz
https://repository.engineering.redhat.com/nexus/repository/registry.npmjs.org/yargs-parser/-/yargs-parser-5.0.1.tgz

@datho7561
Copy link
Contributor Author

Thanks for submitting the PR! I'll give it a review.

For the CI step, I think a check using jq or grep would be fine. Once we've identified the issue, it should be easy enough for the PR author to do a find and replace and push an amend, so I don't think reporting the line number or automatically fixing the PR through the job are worth the effort.

datho7561 pushed a commit that referenced this issue May 29, 2023
Should make it possible for mere mortals like me to install packages :D

Related to #2876.
@rgrunber rgrunber added this to the End May 2023 milestone May 31, 2023
@rgrunber rgrunber modified the milestones: End May 2023, End June 2023 Jun 26, 2023
@rgrunber rgrunber changed the title Check for repository.engineering.redhat.com in package-lock.json as a step in CI Check for non-standard NPM registry in package-lock.json as a step in CI Jun 26, 2023
@rgrunber
Copy link
Member

Thanks for taking the time to fix this up! It should run as part of the verification job now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants