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

Add trusty scores #771

Draft
wants to merge 35 commits into
base: main
Choose a base branch
from
Draft

Conversation

therealnb
Copy link

This is some work to include trusty scores in the dependency review action.

There are some more unit tests and this has been manually tested with https://github.com/StacklokLabs/DepRevTest/actions.

Many thanks.

CC @lukehinds

Signed-off-by: nigel brown <nigel@stacklok.com>
Signed-off-by: nigel brown <nigel@stacklok.com>
Signed-off-by: nigel brown <nigel@stacklok.com>
Signed-off-by: nigel brown <nigel@stacklok.com>
Signed-off-by: nigel brown <nigel@stacklok.com>
Signed-off-by: nigel brown <nigel@stacklok.com>
Signed-off-by: nigel brown <nigel@stacklok.com>
Signed-off-by: nigel brown <nigel@stacklok.com>
Signed-off-by: nigel brown <nigel@stacklok.com>
Signed-off-by: nigel brown <nigel@stacklok.com>
Signed-off-by: nigel brown <nigel@stacklok.com>
Signed-off-by: nigel brown <nigel@stacklok.com>
Signed-off-by: nigel brown <nigel@stacklok.com>
Signed-off-by: nigel brown <nigel@stacklok.com>
Signed-off-by: nigel brown <nigel@stacklok.com>
Signed-off-by: nigel brown <nigel@stacklok.com>
Signed-off-by: nigel brown <nigel@stacklok.com>
Signed-off-by: nigel brown <nigel@stacklok.com>
Signed-off-by: nigel brown <nigel@stacklok.com>
Signed-off-by: nigel brown <nigel@stacklok.com>
Signed-off-by: nigel brown <nigel@stacklok.com>
Signed-off-by: nigel brown <nigel@stacklok.com>
Signed-off-by: nigel brown <nigel@stacklok.com>
Signed-off-by: nigel brown <nigel@stacklok.com>
@therealnb therealnb requested a review from a team as a code owner May 15, 2024 15:02
Signed-off-by: nigel brown <nigel@stacklok.com>
@jonjanego
Copy link
Contributor

Hi @lukehinds and @therealnb !

Some initial comments and observations:

  • In the results, what is the "+ / -" column indicating? It seems to be including two pieces of information, a "+" or "-", then an overall stoplight measure for the package. I think this should be broken into two separate columns, although i'm not sure what the "+" or "-" indicates?
  • This PR also includes a change to dependency-review.yml which is the workflow definition for the repository that runs dependency review on itself. It's included a change to run it against your action - which we won't be able to merge.

I'll make some other suggestions and feedback inline in the PR

Copy link
Contributor

@jonjanego jonjanego left a comment

Choose a reason for hiding this comment

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

Some initial feedback; not a tech review

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
.github/workflows/dependency-review.yml Outdated Show resolved Hide resolved
src/schemas.ts Outdated Show resolved Hide resolved
Co-authored-by: Jon Janego <jonjanego@github.com>
@therealnb
Copy link
Author

@jonjanego thanks for the feedback.

+/- is whether the file was added or removed. The next symbol is whether this is a good idea or not. Basically, removing any file is safe (green tick). Adding one with a score lower than the configured levels might get a warning or a cross.

I did consider only showing files that were added, but it might be useful context to show the ones that are removed too (i.e. removing a high scoring one and replacing it with a low scoring one). I am open to suggestions here.

I'll work on your other comments.

Cheers

therealnb and others added 2 commits May 17, 2024 13:53
Co-authored-by: Jon Janego <jonjanego@github.com>
Signed-off-by: nigel brown <nigel@stacklok.com>
@jonjanego
Copy link
Contributor

@jonjanego thanks for the feedback.

+/- is whether the file was added or removed. The next symbol is whether this is a good idea or not. Basically, removing any file is safe (green tick). Adding one with a score lower than the configured levels might get a warning or a cross.

I did consider only showing files that were added, but it might be useful context to show the ones that are removed too (i.e. removing a high scoring one and replacing it with a low scoring one). I am open to suggestions here.

I'll work on your other comments.

Cheers

thanks for the clarification. i think it's fine to include that, but a bit of UX feedback would be to split that information into two separate columns. it's a cleaner view that way.

Signed-off-by: nigel brown <nigel@stacklok.com>
(I hope)

Signed-off-by: nigel brown <nigel@stacklok.com>
@therealnb
Copy link
Author

I just saw your comment after I committed this code
StacklokLabs@73dc706
Which is another way to do it.

See https://github.com/StacklokLabs/DepRevTest/actions/runs/9131982560?pr=5 for an example.

If you want I can revert and we can have two columns. Let me know.

@therealnb therealnb requested a review from jonjanego May 17, 2024 17:50
@therealnb
Copy link
Author

For information these ones

Dependency Change Version Score Not Malicious Not Deprecated Not Archived
❌ added bugsnagmw 1.0.3 0
⚠️ added psycopg 3.1.18  
⚠️ added psycopg_pool 3.2.1  
⚠️ added pyarrow 16.0.0  
⚠️ added python_json_logger 2.0.7  
⚠️ added scikit_learn 1.4.2  
⚠️ added slack_sdk 3.27.1  

Are a bug I found in our ingestion. There is a fix for those that should be deployed next week.

@therealnb therealnb marked this pull request as draft May 20, 2024 16:06
Signed-off-by: nigel brown <nigel@stacklok.com>
Signed-off-by: nigel brown <nigel@stacklok.com>
Signed-off-by: nigel brown <nigel@stacklok.com>
and add bearer token

Signed-off-by: nigel brown <nigel@stacklok.com>
Signed-off-by: nigel brown <nigel@stacklok.com>
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.

None yet

2 participants