Allow automatic triggering of autofixing of Pull Request linting errors with a comment

[rtibbles]

Expected behavior

A Github action that is triggered by comments on open Pull Requests.

If the pull request comment contains the text "autofixme!" then the workflow should run pre-commit run --all-files or equivalent on the code of the pull request. If there are any unstaged changes after this (i.e. autofixes have been generated), then they should be committed and pushed to the pull request branch.

This may be most easily accomplished by simply triggering the pre-commit-ci lite action in the case that autofixme! is detected https://pre-commit.ci/lite.html

[im-NL]

I've spent too many hours and this script detects the comment but the workflow seems to be skipping the pre-commit-ci/[email protected] step every time after the previous step exits with this error -> https://pastebin.com/XcNBmTKx

name: autofix linting errors
on:
  issue_comment:
    types: [created, edited, deleted]
jobs:
  autofix:
    if: contains(github.event.comment.body, 'autofixme!')
    runs-on: [ubuntu-latest]
    steps:
      - uses: actions/checkout@v3
      - uses: actions/setup-python@v4
        with:
          python-version: 3.x
      - uses: pre-commit/[email protected]
      - uses: pre-commit-ci/[email protected]
        if: always()

I feel like pre-commit.ci doesn't work well with comments. It would be simpler to just run a lint check on every pull request as shown in the docs

[MisRob]

Thank you for looking into this @im-NL. @rtibbles has a good experience with GH actions - any recommendations on how to proceed?

[rtibbles]

The error there seems to be coming from the incorrect version of node being used. If you look at our current pre-commit action: https://github.com/learningequality/kolibri/blob/develop/.github/workflows/pre-commit.yml#L36 the version of node needs to be properly set.

The reason that pre-commit-ci/lite-action is not doing anything is because there is nothing to commit. The error is coming from the incorrect node version, not any linting failure.

[im-NL]

Yes thank you @rtibbles, I hadn't been reading the error message properly before.

When pre-commit-ci lite is triggered by comments, it says skip: not a pull request. It looks like autofixing doesn't work in this case (?)
This is the autofix file

name: autofix linting errors
on:
  issue_comment:
    types: [created]
jobs:
  autofix:
    if: contains(github.event.comment.body, 'autofixme!')
    runs-on: [ubuntu-latest]
    steps:
      - uses: actions/checkout@v3
      - uses: actions/setup-python@v4
        with:
          python-version: 3.x
      - uses: actions/setup-node@v3
        with:
          node-version: '16.x'

      - name: Get yarn cache directory path
        id: yarn-cache-dir-path
        run: echo "dir=$(yarn cache dir)" >> $GITHUB_OUTPUT
      - name: Cache Node.js modules
        uses: actions/cache@v3
        with:
          path: ${{ steps.yarn-cache-dir-path.outputs.dir }}
          key: ${{ runner.os }}-yarn-${{ hashFiles('**/yarn.lock') }}
          restore-keys: |
            ${{ runner.os }}-yarn-
      - name: Install dependencies
        run: |
          yarn --frozen-lockfile
          npm rebuild node-sass

      - uses: pre-commit/[email protected]
      - uses: pre-commit-ci/[email protected]
        if: always()

Instead, should we just run pre-commit-ci lite on every pull request? That would be a very simple fix

[thesujai]

I'm interested in this. Just a couple of clarifications:

1. Currently, the pre-commit process warns about certain issues like "print" or "console". Should I leave these warnings as they are, or would you like me to address them as part of this fix?

2. Additionally, there's an existing workflow for updating the AUTHORS.md file. I noticed that there may be some issues with this workflow. For instance, when I created a pull request to my fork of Kolibri, the workflow triggered a commit updating my branch with my name in AUTHORS.md. However, I haven't observed this behavior when creating a pull request to the main repository. The solution to this issue shall also include the checkout action to push into origin branch.
   Maybe this behavior is due to GH action bot not having permission to commit into contributer's branch

Could you please provide some insights on how you'd like me to proceed with these aspects? Thank you! Also, could I be assigned to this issue?

[rtibbles]

1. No need to make any edits here, the only purpose is to make anything that is currently autofixable, autofixed.
2. Yes - there definitely appears to be a permissions issue at play here. If you could discern what would need to be done to make this work as intended, this would be helpful. One thing to consider here is the work that is being done on KDS to automatically update the changelog. Rather than updating it within the pull request, instead the action is triggered after the PR has been merged. This might be a better solution and would also lead to fewer merge conflicts for pull requests.

I will assign you - I would recommend doing tests of this in your own test repository to have full control and permissions over the action (if you add the action via a pull request directly in the Kolibri repo, there will be no way to test it on pull requests).

[thesujai]

Please check this: https://github.com/thesujai/kolibri/blob/release-v0.16.x/.github/workflows/precommit-autofix.yml
Not using any precommit ci lite or the heavy version, i am doing it manually, running -> pre-commit run --all-files and then simply committing to the target branch after the PR is merged.
Check this commit it made after the workflow ran thesujai@c2c488d

What I am not sure is about if there will be security compromises with this?

if yes:
The other work around i can think of is(But slow): PR merged-->Autofix Starts-->create-newbranch(main repo by the bot)->create a pr with lint fixes
We can use https://github.com/peter-evans/create-pull-request

[rtibbles]

The main issue I can see with the action you've got in there, is that it is running on the main branch post-merge - which wouldn't allow for linting to be fixed before merge. As you noted above, there are some linting issues that can't be fixed automatically, so we would want to do the autofixing, but still confirm that everything is fixed before we merge to account for these issues.

[rtibbles]

In terms of the permissions we would need, Peter Evans has a lot of documentation for how to achieve this properly, which is definitely worth engaging with to understand how to do this best.

[thesujai]

Thank you for you suggestions. After a day working on this, i got two solutions that will auto fix pre-merge

1. Using Pre-commit.ci lite: But the catch here is it can only be used on event pull_request, so there will be no way we will be able to use this on issue_comment unless GITHUB adds a comment key on pull_request events.
   Their code is only designed to only take pull_request event https://github.com/pre-commit-ci/lite-action/blob/main/main.mjs#L8
   So I can add

 - name: Run pre-commit-ci-lite
   uses: pre-commit-ci/[email protected]
   if: always()

at the last here

This will fix automatically on every other commit in the PR, all we need to do is install pre-commit.ci lite in this REPO.

But if we are some how able to override the github.event_name to 'pull_request' even when the event is 'issue_comment', we surely can achieve an autofix on comment. I tried many methods, but all failed.

2. Use pre-commit.ci(non lite): we will just need to install it this repo and we can configure out current .pre-commit.config.yml by adding

ci:
 autofix_prs: false

I have tested both the approach, both works fine only.

[rtibbles]

Let's give the first option a go then - it's not the completely ideal solution, but would cover most use cases. If the commit is unwanted and the developer prefers to fix the linting in another way, then they always have the option to update locally and force push to override the commit.