-
Notifications
You must be signed in to change notification settings - Fork 34
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
vulnerability evaluator: Submit reviews with just comment if the author is the same as the mediator identity #952
Conversation
e1e0f3b
to
f2aca81
Compare
@@ -802,10 +802,16 @@ func getPullRequestInfoFromPayload( | |||
return nil, fmt.Errorf("error getting pull request number from payload: %w", err) | |||
} | |||
|
|||
prAuthorId, err := util.JQReadFrom[float64](ctx, ".pull_request.user.id", payload) |
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 reading a float64
and then storing it as an int64
. Can we extract this as an int64
? I doubt we'll lose precision, but it makes me nervous.
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 think that must by copy-pasta. Let me look into changing it.
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.
so this is indeed copy-paste, we use float64 everywhere in the Go accessor and also used float64 before we introduced the go-generics-powered JQReadFrom
.
However, just trying to convert to int64 gives me an error like:
could not type assert 1 to int64
which is coming from:
out, ok := outAny.(T)
if !ok {
return out, fmt.Errorf("could not type assert %v to %v", outAny, reflect.TypeOf(out))
}
I'll admit I don't know why doesn't Go like this type conversion, do you have some idea? I wonder if it's because the JQ library might be using encoding/json
under the hood which says in the docs that the default value for numerical types in float64
?
But at the same time, plain ol' int
seems to work fine, so maybe I'm just Go-impaired and don't see 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.
I'm fine with adding a TODO if it requires a lot of debugging. Just keeping track of where we have this debt will be the first step in cleaning it up in December.
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 I filed #965 and will move on with this PR.
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.
alongside evans points on types, looks good to me!
f2aca81
to
e044842
Compare
…or is the same as the mediator identity Because github doesn't let you review your own code, in case mediator is running as the same identity as the contributor submitting the PR, we can't request changes. In that case, let's just mark the review as commenting instead of changes requested. A follow-up patch would then set the commit status to failed to prevent merge. Fixes: #951
e044842
to
bf7f727
Compare
Because github doesn't let you review your own code, in case mediator is
running as the same identity as the contributor submitting the PR, we
can't request changes.
In that case, let's just mark the review as commenting instead of
changes requested. A follow-up patch would then set the commit status to
failed to prevent merge.
Fixes: #951