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

strict check of pr-review write permission #1521

Merged
merged 26 commits into from Nov 23, 2023

Conversation

shogo82148
Copy link
Contributor

@shogo82148 shogo82148 commented Aug 9, 2023

There is currently no way to check the available scope of the tokens provided by GitHub; the tokens available for GitHub Actions vary in permissions in complex ways, depending on the repository, workflow settings, and how the workflow is launched.

The most reliable way is to actually post review comments.

  • Updated Unreleased section in CHANGELOG or it's not notable changes.

close #1210

There is currently no way to check the available scope of the tokens provided by GitHub; the tokens available for GitHub Actions vary in permissions in complex ways, depending on the repository, workflow settings, and how the workflow is launched.

The most reliable way is to actually post review comments.
@shogo82148 shogo82148 marked this pull request as draft August 9, 2023 16:16
service/github/github.go Outdated Show resolved Hide resolved
service/github/github.go Outdated Show resolved Hide resolved
@haya14busa
Copy link
Member

Is this pr review ready?

@shogo82148
Copy link
Contributor Author

No, I doesn't work expectly...

shogo82148 added a commit to shogo82148/reviewdog-test-pr1521 that referenced this pull request Sep 7, 2023
@shogo82148 shogo82148 marked this pull request as ready for review September 7, 2023 14:48
@shogo82148
Copy link
Contributor Author

@haya14busa it's ready, can you review it please.

Copy link
Member

@haya14busa haya14busa left a comment

Choose a reason for hiding this comment

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

I'm really sorry for the late review 🙇🙇🙇
It looks promising. Thank you for your work!
I left a few comments.

service/github/github.go Show resolved Hide resolved
service/github/github.go Outdated Show resolved Hide resolved
service/github/github.go Outdated Show resolved Hide resolved
service/github/github.go Outdated Show resolved Hide resolved
InDiffFile: true,
InDiffContext: true,
},
ToolName: "tool",
Copy link
Member

Choose a reason for hiding this comment

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

Can we use a bit more helpful tool name and message?

e.g. tool name: service/github/github_test.go
message: test message for TestGitHubPullRequest_Post_NoPermission

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed 0aa828d

Result: &filter.FilteredDiagnostic{
Diagnostic: &rdf.Diagnostic{
Location: &rdf.Location{
Path: "reviewdog.go",
Copy link
Member

Choose a reason for hiding this comment

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

Can we use a file under _testdata/ file or service/github/github_test.go if this test expect outputting the actual github action log comments?

continue
}

rawComments = append(rawComments, c)
if !c.Result.InDiffContext {
// If the result is outside of diff context, fallback to GitHub Review
// Comment API.
comment := buildPullRequestComment(c, body, g.sha)
Copy link
Member

Choose a reason for hiding this comment

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

[Unrelated to this pr]
I noticed that line (position) information is completely lost for these file comments and it must be confusing for most cases. 🤔
Probably we should add line (position) information in comment body.

(You don't need to address this comment in this pr. )

Copy link
Member

@haya14busa haya14busa left a comment

Choose a reason for hiding this comment

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

LGTM!

@haya14busa haya14busa merged commit 7544c2a into master Nov 23, 2023
32 checks passed
@haya14busa haya14busa deleted the strict-check-pr-review-write-permission branch November 23, 2023 09:36
@furushchev
Copy link

@haya14busa I'm glad the alternative way was found and merged. (Though, it's a bit sad that the PR #1406 was closed without any interaction after a long pending time)
When will it be released?

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.

reviewdog erroneously falls back to logging on forked repo even when it has write permissions
4 participants