feat: Add license enrichment from pypi to python packages#4295
feat: Add license enrichment from pypi to python packages#4295spiffcs merged 4 commits intoanchore:mainfrom
Conversation
Signed-off-by: Tim Olshansky <456103+timols@users.noreply.github.com>
Signed-off-by: Tim Olshansky <456103+timols@users.noreply.github.com>
cf95dc2 to
519e9f0
Compare
Signed-off-by: Tim Olshansky <456103+timols@users.noreply.github.com>
kzantow
left a comment
There was a problem hiding this comment.
A few small things I think should be updated
| if resp.StatusCode != 200 { | ||
| return "", fmt.Errorf("unable to get package from pypi registry") | ||
| } | ||
| defer func() { |
There was a problem hiding this comment.
Should this precede the status code check? there can be response bodies that need to be closed with other statuses, I think
| require.Equal(t, tc.expectedError.Error(), err.Error()) | ||
| } else { | ||
| require.Equal(t, tc.expectedError, err) |
There was a problem hiding this comment.
tc.expectedError could be nil here. there are some require helpers for errors like require.NoError and require.ErrorContains. I think this could be a bit better written like:
if tc.expectedError != nil {
require.ErrorContains(t, err, tc.expectedError.Error())
} else {
require.NoError(t, err)
or similar
There was a problem hiding this comment.
In reality we control what the expected error is and this would blow up in a very obvious way when running unit tests. That said, it's a small change to have an even better error message. 😄
| return "", fmt.Errorf("unable to format remote request: %w", err) | ||
| } | ||
|
|
||
| httpClient := &http.Client{ |
There was a problem hiding this comment.
We should have some common configuration for timeout options, retry behavior possibly, rate limiting, etc.. This is becoming more important as we add more online resolution. We also might think about adding these features in a way that could be used in parallel and to enhance existing SBOMs rather than only at creation time. I don't think that has to happen as part of this PR, but I think we're getting to the point that we need to start being conscious of some problems people will start running into more frequently like the Maven rate limiting.
There was a problem hiding this comment.
Yep, that makes sense.
Signed-off-by: Tim Olshansky <456103+timols@users.noreply.github.com>
|
Hi @kzantow - let me know if there is anything remaining to get this across the line |
|
Hi @kzantow anything holding this back from being approved and merged? |
|
LGTM TY @timols these contributions have been excellent and I really appreciate all the thought and effort you put into both PR |
|
Thanks @spiffcs - happy to contribute! 😄 |
Description
This PR introduces license enrichment from PyPI for python-related packages (excluding source based packages defined by a wheel or egg since license info should typically be present on the file system)
Type of change
Checklist: