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

Handle package not found in package registries #3363

Merged
merged 3 commits into from
May 20, 2024

Conversation

jhrozek
Copy link
Contributor

@jhrozek jhrozek commented May 18, 2024

Summary

If we don't find a package in the configured package registry, we would
have errored out. Let's instead reply inline that there is a
vulnerability but that we can't find information about the package.

Fixes: #3368

Change Type

  • Bug fix (resolves an issue without affecting existing features)
  • Feature (adds new functionality without breaking changes)
  • Breaking change (may impact existing functionalities or require documentation updates)
  • Documentation (updates or additions to documentation)
  • Refactoring or test improvements (no bug fixes or new functionality)

Testing

Manual testing, see e.g. jakubtestorg/demo-repo-js#4

Review Checklist:

  • Reviewed my own code for quality and clarity.
  • Added comments to complex or tricky code sections.
  • Updated any affected documentation.
  • Included tests that validate the fix or feature.
  • Checked that related changes are merged.

@jhrozek jhrozek requested a review from a team as a code owner May 18, 2024 18:37
Copy link
Contributor

@stacklokbot stacklokbot left a comment

Choose a reason for hiding this comment

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

✅ No Invisible Unicode Characters Detected.

@jhrozek jhrozek marked this pull request as draft May 18, 2024 18:37
@jhrozek
Copy link
Contributor Author

jhrozek commented May 18, 2024

This is a draft because a) there are no tests and b) I think there is room for improvement (see inline comments)

@coveralls
Copy link

coveralls commented May 18, 2024

Coverage Status

coverage: 50.265% (-0.1%) from 50.388%
when pulling 916b690 on jhrozek:pkg_not_found
into df58820 on stacklok:main.

Copy link
Contributor

@stacklokbot stacklokbot left a comment

Choose a reason for hiding this comment

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

✅ No Invisible Unicode Characters Detected.

Copy link
Contributor

@stacklokbot stacklokbot left a comment

Choose a reason for hiding this comment

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

✅ No Mixed Scripts Detected.

If we don't find a package in the configured package registry, we would
have errored out. Let's instead reply inline that there is a
vulnerability but that we can't find information about the package.
Copy link
Contributor

@stacklokbot stacklokbot left a comment

Choose a reason for hiding this comment

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

✅ No Invisible Unicode Characters Detected.

Copy link
Contributor

@stacklokbot stacklokbot left a comment

Choose a reason for hiding this comment

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

✅ No Mixed Scripts Detected.

@jhrozek jhrozek marked this pull request as ready for review May 20, 2024 08:07
JAORMX
JAORMX previously approved these changes May 20, 2024
@@ -386,6 +393,9 @@ func TestPyPiPkgDb(t *testing.T) {
}
reply, err := repo.SendRecvRequest(context.Background(), dep, tt.patchedVersion, latest)
if tt.expectError {
if tt.errorToExpect != nil {
assert.Equal(t, tt.errorToExpect, err, "Expected error")
Copy link
Contributor

Choose a reason for hiding this comment

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

why not use assert.ErrorIs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

because I'm dumb :-) let me fix it

@rdimitrov rdimitrov merged commit eddc285 into mindersec:main May 20, 2024
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Minder errors out when a package is not found in NPM while evaluating a PR
5 participants