Extend PR vulnerability checks with a configurable action to set commit status #966
Conversation
|
I'm working on adding tests, feel free to leave comments about the code while I'm doing that, but let's not merge the PR without tests.. |
|
I added a unit test. More cleanups to review.go are incoming (we need to handle non-200 errors from APIs more gracefully and we need to reuse http.Client across calls) but this gives us a baseline test coverage. |
| @@ -129,6 +133,10 @@ func locateDepInPr( | |||
| return &loc, nil | |||
| } | |||
|
|
|||
| func reviewBodyWithSuggestion(comment string) string { | |||
| return fmt.Sprintf("```suggestion\n"+"%s\n"+"```\n", comment) | |||
There was a problem hiding this comment.
Why use + to combine these strings?
There was a problem hiding this comment.
because I'm using copilot and didn't check what it suggested :-) Fixed, thanks!
| } | ||
|
|
||
| return &commitStatusPrHandler{ | ||
| reviewPrHandler: *rph.(*reviewPrHandler), |
There was a problem hiding this comment.
Can we change the return type of newReviewPrHandler to be (*reviewPrHandler, error) and have it still match the target schema? That would avoid this (potentially panicking) cast here without needing to either introduce error-handling code or otherwise clue in the compiler that this cast is safe.
There was a problem hiding this comment.
Ha, good idea, thank you.
| err := csh.reviewPrHandler.submit(ctx) | ||
| if err != nil { |
There was a problem hiding this comment.
Slight preference for the more compact define-err-in-condition-body:
| err := csh.reviewPrHandler.submit(ctx) | |
| if err != nil { | |
| if err := csh.reviewPrHandler.submit(ctx); err != nil { |
Note that this causes a new instance of err to be allocated, so isn't always appropriate.
There was a problem hiding this comment.
do you have a rule of thumb as per when do you use one or the other? I've been honestly treating them mostly as a matter of style and didn't think about the extra allocation as something worth noting much.
that said, suggestion taken.
| if err != nil { | ||
| return err | ||
| } | ||
| return nil |
There was a problem hiding this comment.
Can this just be:
| if err != nil { | |
| return err | |
| } | |
| return nil | |
| return err |
There was a problem hiding this comment.
sure, why not (it is easier on my eyes to see an explicit return nil in the happy path, but I don't really care one way or the other)
…it status Because using code reviews to block commits might be problematic for self-enrollment, this patch adds a new `action` enum value named `commit_status`. When the PR vulnerability check is enabled and this action is selected, mediator will either mark the commit status as succeeded if no vulns are found or failed if vulnerabilities are found. In the repository branch protection rules, the user can then configure a rule that would require the `mediator.stacklok.dev/pr-vulncheck` rule to pass. The vulnerable dependencies are still pointed out in review comments, but the review is submitted as "COMMENT", the blocking decision is made by the commit status. Fixes: #935
jhrozek
force-pushed
the
prevent_commit_status
branch
from
d9153c5
to
0a3e85b
Compare
Because using code reviews to block commits might be problematic for
self-enrollment, this patch adds a new
actionenum value namedcommit_status. When the PR vulnerability check is enabled and thisaction is selected, mediator will either mark the commit status as
succeeded if no vulns are found or failed if vulnerabilities are found.
In the repository branch protection rules, the user can then configure a
rule that would require the
mediator.stacklok.dev/pr-vulncheckrule topass.
The vulnerable dependencies are still pointed out in review comments,
but the review is submitted as "COMMENT", the blocking decision is made
by the commit status.
Fixes: #935