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

🐛 Use direct endpoint instead of search to find repository URL from npm database #4118

Merged
merged 16 commits into from
Jun 5, 2024

Conversation

aklevans
Copy link
Contributor

@aklevans aklevans commented May 20, 2024

What kind of change does this PR introduce?

This PR is a bug fix. When finding the github repository URL for an npm package, the application uses the GET·/{package}/latest endpoint instead of the GET·/{package}

What is the current behavior?

The application finds the github URL of an npm package by using the search functionality. If a user incorrectly enters a github URL instead of an npm package name, the user will not receive feedback about their mistake and the application will run the checks on the first package that the search finds.

What is the new behavior (if this is a feature change)?**

When a user enters a github URL instead of an npm package name, an error message is displayed saying that the npm package could not be found.

  • Tests for the changes have been added (for bug fixes/features)

Which issue(s) this PR fixes

Fixes #3166.

Special notes for your reviewer

Does this PR introduce a user-facing change?

NO

NONE

@aklevans aklevans requested a review from a team as a code owner May 20, 2024 20:30
@aklevans aklevans requested review from spencerschrock and raghavkaul and removed request for a team May 20, 2024 20:30
@raghavkaul
Copy link
Contributor

Thanks for the contribution! Could you add some unit tests that expect an error when a GitHub/NPM URL is passed to the --npm flag instead of a project name? Even better to test the projects from #2144 and #3166, which silently score the wrong projects because of the inexact name match.

@aklevans
Copy link
Contributor Author

aklevans commented May 21, 2024

I added unit tests for #3166 and #2441 with mock http responses similar to the existing tests

@raghavkaul
Copy link
Contributor

Still getting linter issues, try make fix-linter.

For package_manager_test.go, can we make the diff smaller by only returning the parts of the HTTP/JSON response in args.result that we'll actually end up parsing?

@aklevans
Copy link
Contributor Author

I don't think fix-linter is working correctly on my machine. It just adds line breaks between every single line. Any reason this may happen?

@raghavkaul
Copy link
Contributor

Hm, that's strange that --fix has that behavior, I think you'll need to fix those issues manually then. It looks like some lints were fixed but there are still some issues. Make sure that make all passes, because our CI requires that for merge.

Signed-off-by: aklevans <[email protected]>
Signed-off-by: aklevans <[email protected]>
Copy link

codecov bot commented May 30, 2024

Codecov Report

Attention: Patch coverage is 72.54902% with 28 lines in your changes are missing coverage. Please review.

Project coverage is 60.12%. Comparing base (6815161) to head (d322d70).
Report is 20 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4118      +/-   ##
==========================================
- Coverage   66.06%   60.12%   -5.95%     
==========================================
  Files         226      214      -12     
  Lines       16291    15621     -670     
==========================================
- Hits        10763     9392    -1371     
- Misses       4854     5537     +683     
- Partials      674      692      +18     

@spencerschrock
Copy link
Member

I don't think fix-linter is working correctly on my machine. It just adds line breaks between every single line. Any reason this may happen?

Can you share a little bit about your environment? You're running the linter with make check-linter? The double-space the whole file approach should be reverted.

@aklevans
Copy link
Contributor Author

I think the issue was stemming from the fact I was working with windows. I have switched to Linux and I believe it is working now.

Copy link
Member

@spencerschrock spencerschrock left a comment

Choose a reason for hiding this comment

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

Overall looks good, thanks!

cmd/package_managers.go Outdated Show resolved Hide resolved
cmd/package_managers.go Outdated Show resolved Hide resolved
@spencerschrock
Copy link
Member

Thanks. I'll update the branch later and get this merged in. The DCO bot is seemingly not running, so can't at the moment.

@spencerschrock spencerschrock merged commit 0448565 into ossf:main Jun 5, 2024
36 checks passed
balteravishay pushed a commit to balteravishay/scorecard that referenced this pull request Jun 12, 2024
…pm database (ossf#4118)

* Update endpoint used when getting repo from npm to solve ossf#3166

Signed-off-by: aklevans <[email protected]>

* Update test files to account for endpoint change when getting repo from npm

Signed-off-by: aklevans <[email protected]>

* Fix linter issues

Signed-off-by: aklevans <[email protected]>

* Added unit tests for ossf#3166 and ossf#2441

Signed-off-by: aklevans <[email protected]>

* fix linter issues and reduce mock json output in package_manager_test to only include necessary data

Signed-off-by: aklevans <[email protected]>

* fix linter issues in package_managers.go

Signed-off-by: aklevans <[email protected]>

* convert windows line breaks to linux

Signed-off-by: aklevans <[email protected]>

* reduce test case size, still has windows line breaks

Signed-off-by: aklevans <[email protected]>

* Fix unit tests

Signed-off-by: aklevans <[email protected]>

* attempt linter fix

Signed-off-by: aklevans <[email protected]>

* Fix linter issues stemming from windows line breaks

Signed-off-by: aklevans <[email protected]>

* Remove magic number and rename variable to be more accurate

Signed-off-by: aklevans <[email protected]>

---------

Signed-off-by: aklevans <[email protected]>
Signed-off-by: aklevans <[email protected]>
Signed-off-by: balteraivshay <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

BUG: --npm wrong input does not throw error
3 participants