Feature: provide automated PR reviews for linting related changes

[OnkarRuikar]

• Alternative to feat(ci): Add Reviewdog for Prettier formatting #28228

Purpose of this PR is to provide an alternative approach. This PR involves all the linters/formatters (markdown, prettier, front-matter) for automated review using reviewdog.

Summary

Current Prettier implementation in our CI is being annoying because the formatter tells contributors that there is something wrong with their changes without telling what is it and how to fix it. This is forcing contributors to fix those issues using command line tools or online Prettier playground.
Using reviewdog it is possible to instead suggest those changes as review comments on PRs. With this contributors will simply have to accept these changes and commit them in a batch.

While we are at it we can involve markdownlint and front-matter linters too, i.e we can suggest solutions to markdown and front-matter related issues as well.

Implementation

reviewdog uses two inputs to provide review comments:
a) git diff generated after running the linters with --fix or --write options
b) logs generated by the linters along with error message matching pattern

The implementation performs following steps to provide the review comments:

1. Run all the linters/formatter to fix the code and collect logs along with exit statuses.
2. Use git diff to suggest changes done by the linters using reviewdog -f=diff command. Basically we are suggesting the same changes done by the linters.
   Note: The diff is collected after running all the linters because we always run Prettier the last in order to avoid clashes with markdownlint. Also providing reviews for each linter separately will confuse the contributors. The combined linting is being presented as "mdn-linter".
3. If there are unfixable issues reported by markdownlint then put them as review comments using reviewdog -efm command.
4. Initially the front-matter linter had been implemented as a yari tool, so the logs generated by it are multi line. Till we make those logs single line just log the front-matter output and fail the workflow.

[github-actions]

Preview URLs

• /en-US/docs/Games

(comment last updated: 2023-09-29 18:29:46)

[OnkarRuikar]

We'll need a token with Pull request write permissions. Getting following error:

reviewdog: This GitHub token doesn't have write permission of Review API [1],

For demo output see OnkarRuikar#15

[yin1999]

One nit :)

[yin1999]

Hi @OnkarRuikar, should we make the "add lint reviews to PRs" workflow failed when there does have some formats need to be done (maybe we should rename this workflow). Just as what I said in another PR, this could help the maintainer to check if it's ready to merge PRs (if the contributer just mark the suggestions as resolved, the maintainer may not realize that the PR is not ready to be merged yet.

[OnkarRuikar]

if the contributer just mark the suggestions as resolved, the maintainer may not realize that the PR is not ready to be merged yet

Good point! Now the job fails when there are any suggestions by the linters or raised issues.

[OnkarRuikar]

We are not able to make it work for the PRs submitted from forks of guest users. The initial PR showed those review comments because the PR author has direct push permissions to the repo so their GITHUB_TOKEN in the workflow runs got write permissions. But PRs from guest users are not getting the write permissions. We've tried to use a repo wide secret personal access token with PR write permissions but it is not visible in PRs from fork.

[yin1999]

Hi @OnkarRuikar, is it possible that we could run the reviewdog in another workflow which could be triggered by "Add lint reviews to PRs" workflow. So we could use mdn-bot token (or other token which has write permission, maybe in this way, the GITHUB_TOKEN does have write permission) to report suggestions (This should keep the repository secure). But the reviewdog seems to only works with pull request event.

So I just check how the reviewdog collects enough info before submitting suggestions. The entry should be here. And all the information seems to be provided by the event file, and it's location is provided by GITHUB_EVENT_PATH environment, see: https://github.com/reviewdog/reviewdog/blob/439aec2bbeb833f510426822a9db01e7edbf910a/cienv/github_actions.go#L69-L74, and https://docs.github.com/en/actions/learn-github-actions/variables#default-environment-variables

So, I'm wondering is it possible that we could upload the event file and git diff content, and in another PR, we just download those files, and set up the GITHUB_EVENT_PATH to simulate the pull request event.

[yin1999]

Hi @OnkarRuikar, is it possible that we could run the reviewdog in another workflow which could be triggered by "Add lint reviews to PRs" workflow. So we could use mdn-bot token (or other token which has write permission, maybe in this way, the GITHUB_TOKEN does have write permission) to report suggestions (This should keep the repository secure). But the reviewdog seems to only works with pull request event.

So I just check how the reviewdog collects enough info before submitting suggestions. The entry should be here. And all the information seems to be provided by the event file, and it's location is provided by GITHUB_EVENT_PATH environment, see: https://github.com/reviewdog/reviewdog/blob/439aec2bbeb833f510426822a9db01e7edbf910a/cienv/github_actions.go#L69-L74, and https://docs.github.com/en/actions/learn-github-actions/variables#default-environment-variables

So, I'm wondering is it possible that we could upload the event file and git diff content, and in another PR, we just download those files, and set up the GITHUB_EVENT_PATH to simulate the pull request event.

I just setup the pr-review-lint-companion workflow, add modified the pr review lint workflow (yin1999/translated-content#17), the reviewdog just submmitted suggestions successfully (check yin1999/translated-content#17 (comment)) :)

/cc @LeoMcA I'm not sure if it's secure enough for us, do you have any thought?

[yin1999]

Hi @OnkarRuikar, one more question, I'm not really familiar with markdown-lint and front-matter-lint, do I just need to redirect stderr to a log file? or we need to combine stdout with stderr, and then redirect to a log file?

[LeoMcA]

Chatted with Onkar on Slack about this, but documenting here for everyone to see:

I think pull_request_target is the way forward here - reviewdog seems to support it without any weird workarounds, but we do still need to be careful about the permissions we gain in that workflow, see: https://securitylab.github.com/research/github-actions-preventing-pwn-requests/

I believe we'll be alright here if we:

• lock down the permissions in that job to only the scopes which reviewdog requires: https://docs.github.com/en/actions/using-jobs/assigning-permissions-to-jobs
• only run executables from the main branch, and only pull in content from the PR's branch, in practice that would look something like:
  1. checkout main branch
  2. checkout the PR branch in a subfolder
  3. remove content from the main branch, replace it with content from the subfolder
  4. remove the subfolder
  5. hopefully reviewdog is happy and has the correct context to run?

[OnkarRuikar]

I think pull_request_target is the way forward here...

I'll try this approach in a new draft PR. This one has a lot of commits and history accrued.

[github-actions]

[mdn-linter] _{reported by reviewdog 🐶}

Suggested change

	body { background-color: aqua; }
	body {
	background-color: aqua;
	}

[github-actions]

[mdn-linter] _{reported by reviewdog 🐶}

Suggested change

	* item 1
	+ item 2
	- item 1
	- item 2

[github-actions]

[mdn-linter] _{reported by reviewdog 🐶}

Suggested change

	## Port native games to the Web
	## Port native games to the Web

[github-actions]

[markdownlint] _{reported by reviewdog 🐶}
MD025/single-title/single-h1 Multiple top-level headings in the same document [Context: # Develop web games]

[Josh-Cena]

Looks like a duplicate of #28363