-
Notifications
You must be signed in to change notification settings - Fork 37
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
Create single status comment and correctly dismiss reviews #2171
Conversation
I accidentally marked it off draft since I was reviewing from my phone. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great, thanks for working on improving the UX and fixing the bug! I left a couple of comments inline.
const ( | ||
reviewBodyMagicComment = "<!-- minder: pr-review-body -->" | ||
statusBodyMagicComment = "<!-- minder: pr-status-body -->" | ||
separatorComment = "<!--[SEP]-->" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
separatorComment
seems to be unused.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Separator is meant for adding metadata to the body of the review and allowing us to easily parse it. So we can include data from a previous review as a comment in JSON form and use it to provide a comparison to the current review. i.e the below will provide info on the previous status report:
<!-- minder: pr-status-body --><!--[SEP]--><!--{ {"VulnerabilityCount":1,"RemediationCount":0,"TrackedDepsCount":1,"CommitSHA":"27d6810b861c81e8c61e09c651875f5a976781d1"} }--><!--[SEP]-->
<h2>Minder Vulnerability Report ⚠️</h2>
...
Probably should have made this more clear. It was also mentioned it here #2149 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, but the constant separatorComment
is still unused, the "string constant is used verbatim in
minderTemplateString`.
Previous Minder review was dismissed because the PR was updated. | ||
` | ||
vulnFoundWithNoPatch = "Vulnerability found, but no patched version exists yet." | ||
stacklokBotUser = "stacklokbot" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I don't think we want to hardcode a bot username here. Later the code does:
isMinder := review.GetUser().GetLogin() == stacklokBotUser && strings.HasPrefix(review.GetBody(), reviewBodyMagicComment)
Could we instead use:
isMinder := review.GetUser().GetLogin() == ghCli.GetAuthenticatedUser() && strings.HasPrefix(review.GetBody(), reviewBodyMagicComment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Haven't looked into that function. Do we expect ghCli.GetAuthenticatedUser()
to return stacklokbot
or the user who triggered the pipeline?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would return the user who enrolled that particular org or personal account, so in the case of stacklok's deployment, it would be stacklokbot.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
for _, r := range reviews { | ||
if strings.HasPrefix(r.GetBody(), reviewBodyMagicComment) && r.GetState() != "DISMISSED" { | ||
ra.minderReview = r | ||
for i := len(reviews) - 1; i >= 0; i-- { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is done because one of the calls returns reviews in reverse order, right? Could you add a comment here so that we remember the reason later?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes exactly. Very fair point. Will add a comment.
} | ||
|
||
func (ra *reviewPrHandler) findPreviousStatusComment(ctx context.Context) error { | ||
comments, err := ra.cli.ListComments(ctx, ra.pr.RepoOwner, ra.pr.RepoName, int(ra.pr.Number), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh so ListComments
allows you to set the sorting, but ListReviews
does not?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately this part of the GitHub API is a bit inconsistent
See https://docs.github.com/en/rest/pulls/reviews?apiVersion=2022-11-28#list-reviews-for-a-pull-request versus https://docs.github.com/en/rest/pulls/comments?apiVersion=2022-11-28#list-review-comments-in-a-repository
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I think this is the crux of the issues I was seeing. So the ra.cli.ListComments
method wraps c.client.PullRequests.ListComments
, its result is what the code iterates over and saves into ra.minderStatusReport
. But, the comments are created with ra.cli.CreateComment
which uses c.client.Issues.CreateComment
- it's a different API. The Issues
one is for what's usually referred to as comments, but the PullRequest
API is for iterating over review comments, those that are submitted either inline or as the review summary.
I know this work has been going on for some time, I wonder if there's a reason those two got mixed up?
fixHtmlEmoji = "🛠" | ||
warningHtmlEmoji = "⚠️" | ||
successHtmlEmoji = "✅" | ||
reviewHtmlEmoji = "📊" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Love the bling!
be86330
to
2a33343
Compare
Thanks for handling the comments! I've tested the PR and there seems to be a case that we don't handle correctly - when I ran minder as myself (meaning the same user who submitted the PR was reviewing) and pushed a new version of a patch (I started with vulnerable
and I didn't see any more updates from minder.. It seems like we regressed handling dismissing of comment reviews in this patch? |
Hi @jhrozek That is strange. So to clarify, one was previously able to dismiss reviews made by the same own author as the PR? Here is what I think is happening, and am not sure if these changes introduce this issue. The // if the user wants minder to request changes on a pull request, they need to
// be different identities
var failStatus *string
if pr.AuthorId == cliUser.GetID() {
failStatus = github.String("COMMENT")
logger.Debug().Msg("author is the same as the authenticated user, can only comment")
} else {
failStatus = github.String("REQUEST_CHANGES")
logger.Debug().Msg("author is different than the authenticated user, can request changes")
} So in the case of your PR, Also, are you sure you are using this branch version of |
I don't think it would dismiss the review, just create another comment. Here is a PR where I started with .20 and then went to .19 and saw a subsequent comment. This one was running main. Before that I tested with another PR running your branch where I pushed an in-between version, but saw no inline comment. I /think/ the summary was updated, but the formatting looks a bit off, too
I think this is pretty much what is happening. I'll try to poke either later today or tomorrow again to understand better what's causing the issue.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your patience, I apologise it took me so long to get back to this review. I think the primary issue is the mixup of the Issues and PullReques APIs, otherwise the code looks quite good to me.
Some minor comments inline.
const ( | ||
reviewBodyMagicComment = "<!-- minder: pr-review-body -->" | ||
statusBodyMagicComment = "<!-- minder: pr-status-body -->" | ||
separatorComment = "<!--[SEP]-->" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, but the constant separatorComment
is still unused, the "string constant is used verbatim in
minderTemplateString`.
} | ||
|
||
func (ra *reviewPrHandler) findPreviousStatusComment(ctx context.Context) error { | ||
comments, err := ra.cli.ListComments(ctx, ra.pr.RepoOwner, ra.pr.RepoName, int(ra.pr.Number), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I think this is the crux of the issues I was seeing. So the ra.cli.ListComments
method wraps c.client.PullRequests.ListComments
, its result is what the code iterates over and saves into ra.minderStatusReport
. But, the comments are created with ra.cli.CreateComment
which uses c.client.Issues.CreateComment
- it's a different API. The Issues
one is for what's usually referred to as comments, but the PullRequest
API is for iterating over review comments, those that are submitted either inline or as the review summary.
I know this work has been going on for some time, I wonder if there's a reason those two got mixed up?
<th>Patch</th> | ||
</tr> | ||
</thead> | ||
<tbody> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For reason I really don't understand the <tbody>
tag is not rendered correctly for me, see this example.
I wonder if we need the thead and tbody tags at all, can't we get away with just tr?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as I understand, using the Issues
API is not actually an issue since every PR is technically an issue (according to the GitHub docs).
You can use the REST API to create and manage comments on issues and pull requests. Every pull request is an issue, but not every issue is a pull request. For this reason, "shared" actions for both features, like managing assignees, labels, and milestones, are provided within the Issues endpoints. To manage pull request review comments, see "Pull request review comments."
With that said, I'll change it to the PullRequest API for correctness.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In order to properly test this, the account making the comments (the author) cannot be the same as the account submitting a review (reviewer
) because of the constraints outlined in #2171 (comment).
I'm also going to remove the section of the status comment that links the latest review -- since it's unnecessarily complicating things now that the status comment is required to be posted prior to the review comment.
} | ||
|
||
if err := ra.dismissReview(ctx); err != nil { | ||
return fmt.Errorf("could not dismiss previous review: %w", err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that previously error on dismiss review was not fatal, this was on purpose so that minder wouldn't error out trying to dismiss its own review running as the same use. I wonder if we could add this to body of dismissReview
:
+ if ra.pr.AuthorId == ra.authorizedUser.GetID() {
+ ra.logger.Debug().Msg("author is the same as the authenticated user, can't dismiss")
+ return nil
+ }
+
this is pretty much the same as ignoring the error, except we don't actually attempt the call so we save a token..
8a73286
to
efdee43
Compare
efdee43
to
7799097
Compare
Hi, I have two apologies - first for the delay, last week Stacklok met in person and not much development got done and so we're slow in PR reviews. Second, re-reading this PR and our chats on Discord, we must have confused you and sent you down the path of implementing something that's too complex. So I pushed another patch atop yours (we squash commits so you'd still be attributed for this PR!) that I think simplifies the flow in the sense that the inline reviews are still retained, but Minder replies with inline comments only and the whole summary including the CVE table is kept in the single summary comment you introduced. Would this approach work for you? |
Hi. No worries. Happy to go with the above approach and have these changes merged in 😄 |
7799097
to
2cf3dc8
Compare
TrackedDependencies []dependencyVulnerabilities | ||
} | ||
|
||
func (r *vulnSummaryReport) render() (string, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not to block this, but it feels odd to assemble this from three templates, rather than one larger template that takes r.TrackedDependencies
as the input, rather than looping outside the template over the tableVulnerabilitiesRows
repeatedly.
(Also, the header template doesn't seem to have any template stuff in it, so it could just be treated like the footer.)
@@ -305,52 +256,93 @@ func (ra *reviewPrHandler) setStatus() { | |||
ra.logger.Debug().Str("status", *ra.status).Msg("will set review status") | |||
} | |||
|
|||
func (ra *reviewPrHandler) findPreviousReview(ctx context.Context) error { | |||
reviews, err := ra.cli.ListReviews(ctx, ra.pr.RepoOwner, ra.pr.RepoName, int(ra.pr.Number), nil) | |||
func (ra *reviewPrHandler) findPreviousStatusComment(ctx context.Context) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be nice if this function didn't pass the found review as a struct field, but again, I'm not going to block this review on existing code styles.
return mci | ||
} | ||
|
||
func (ra *reviewPrHandler) submitReview(ctx context.Context, mci magicCommentInfo) (int64, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Future comment -- it would make sense to include a comment indicating what the returned int64 is. In this case, it appears to be the issue / PR number.
Thanks very much! Unfortunately, it looks like there are a couple of conflicts, so I can't merge. @jhrozek or @gregfurman if you could resolve the conflicts, I'm happy to merge this despite the small nits. |
This commit builds atop the previous one and moves further so that the message in the review reply (not to be confused with inline replies) are empty and all the feedback is kept in the single summary comment. Because we still need to dismiss the reviews, but now we don't have a review text body where we used to keep the magic comment with metadata, we keep it in the summary comment instead. We also keep the review ID there because we can't be sure which is the minder review now in case the same identity is used for a human identity and minder.
2cf3dc8
to
f213b73
Compare
Thank you for the review @evankanderson. If you don't mind I will address the comments you had as part of #1203 (which is getting more and more pressing as we have a fair bit of code duplication now between the reply machinery for trusty and OSV) |
Closes #1862
Overview
This PR allows for a single status comment to be repeatedly created and updated, illustrating an overview state of the minder evaluation. This summary comment is made in conjunction with the minder review comment.
Summary of changes
internal/engine/eval/vulncheck/report.go
that handles review and status comment templatingstacklokbot
-- preventing a user from abusing magic comment checksTo do
Example comments
Example Summary comment
Minder Vulnerability Report⚠️
Minder found vulnerable dependencies in this PR. Either push an updated version or accept the proposed changes. Note that accepting the changes will include Minder as a co-author of this PR.
📬 Have feedback? Share it here.
Example Review Comment
Summary of vulnerabilities found
Minder found the following vulnerabilities in this PR:
📬 Have feedback on the report? Share it here.