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

Dismiss stale reviews when reviewing PRs with vulnerable dependencies #934

Merged
merged 1 commit into from
Sep 13, 2023

Conversation

jhrozek
Copy link
Contributor

@jhrozek jhrozek commented Sep 12, 2023

When the user accepts mediator's suggestions on PRs that container
vulnerable dependencies, we need to re-review the PR. This patch
dismisses the earlier review and adds a new one, either just commenting
that no vulnerable packages were found or listing the remainign
vulnerabilities.

Also moves the package review code behind an interface that will be used
later to just add comments and optionally set commit status to prevent
merge with vulnerable commits.

Fixes: #914

@lukehinds
Copy link
Contributor

Thanks @jhrozek , was just thinking about this. At some point (up to you if its in this patch) we should add details of the vulnerability to the initial comment. We can include the CVE number, its severity etc and perhaps a link to the osv advisory page.

JAORMX
JAORMX previously approved these changes Sep 12, 2023
internal/engine/eval/vulncheck/review.go Outdated Show resolved Hide resolved
@jhrozek
Copy link
Contributor Author

jhrozek commented Sep 12, 2023

Thanks @jhrozek , was just thinking about this. At some point (up to you if its in this patch) we should add details of the vulnerability to the initial comment. We can include the CVE number, its severity etc and perhaps a link to the osv advisory page.

This part of the workflow was always included in the acceptance criteria for the epic, there was just not enough time last week :-) I'll look into providing the details.

@jhrozek
Copy link
Contributor Author

jhrozek commented Sep 12, 2023

Thanks @jhrozek , was just thinking about this. At some point (up to you if its in this patch) we should add details of the vulnerability to the initial comment. We can include the CVE number, its severity etc and perhaps a link to the osv advisory page.

I'll include this in a separate PR if you don't mind, I wanted to refactor the parsing of the OSV vulnerability anyway so I'll do those in one PR which will also make the formatting easier for the commit status action (as opposed to the PR review action)

When the user accepts mediator's suggestions on PRs that container
vulnerable dependencies, we need to re-review the PR. This patch
dismisses the earlier review and adds a new one, either just commenting
that no vulnerable packages were found or listing the remainign
vulnerabilities.

Also moves the package review code behind an interface that will be used
later to just add comments and optionally set commit status to prevent
merge with vulnerable commits.

Fixes: #914
@JAORMX JAORMX merged commit d1570a2 into stacklok:main Sep 13, 2023
12 checks passed
@JAORMX JAORMX deleted the vuln_review branch September 13, 2023 05:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants