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

nodejs: add package.json support #834

Merged
merged 1 commit into from
Jan 11, 2024
Merged

nodejs: add package.json support #834

merged 1 commit into from
Jan 11, 2024

Conversation

RTann
Copy link
Contributor

@RTann RTann commented Feb 16, 2023

Adds support for analyzing NodeJS packages in the form of npm package.json.

Note: This does not currently account for the package.json files being installed due to an rpm. In that case, the package.json file should not be scanned.

@RTann RTann marked this pull request as ready for review February 21, 2023 18:11
@RTann RTann requested a review from a team as a code owner February 21, 2023 18:11
@RTann RTann requested review from crozzy and removed request for a team February 21, 2023 18:11
Copy link
Contributor

@crozzy crozzy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, just a few comments

@RTann RTann force-pushed the npm-package-json branch 3 times, most recently from 43c053d to ab13c9a Compare March 1, 2023 23:40
@RTann RTann force-pushed the npm-package-json branch from ab13c9a to 588400b Compare January 4, 2024 07:45
Copy link

codecov bot commented Jan 4, 2024

Codecov Report

Attention: 33 lines in your changes are missing coverage. Please review.

Comparison is base (7247afe) 52.43% compared to head (70d2937) 52.54%.

Files Patch % Lines
nodejs/packagescanner.go 66.23% 20 Missing and 6 partials ⚠️
nodejs/ecosystem.go 0.00% 5 Missing ⚠️
nodejs/coalescer.go 93.33% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #834      +/-   ##
==========================================
+ Coverage   52.43%   52.54%   +0.11%     
==========================================
  Files         221      224       +3     
  Lines       16893    17005     +112     
==========================================
+ Hits         8857     8936      +79     
- Misses       7224     7251      +27     
- Partials      812      818       +6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@RTann RTann force-pushed the npm-package-json branch 2 times, most recently from 9131f9b to c2553d6 Compare January 4, 2024 18:25
@RTann RTann requested review from hdonnay and crozzy January 4, 2024 21:28
@RTann RTann force-pushed the npm-package-json branch from c2553d6 to 8c938c5 Compare January 4, 2024 23:24
@crozzy
Copy link
Contributor

crozzy commented Jan 10, 2024

Just for posterity I believe the idea is that this could be merged but not enabled by default (at least until we have a better idea of how the large number of added package affects performance).

Copy link
Contributor

@crozzy crozzy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This LGTM code-wise, could you add a test for the coalescer (akin the rhcc one)? It'd be good to get back into the habit as there is usually some interesting logic in them.

@RTann RTann requested a review from crozzy January 11, 2024 07:33
@RTann RTann merged commit 656d6d1 into quay:main Jan 11, 2024
9 checks passed
@RTann RTann deleted the npm-package-json branch January 11, 2024 19:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants