Linting Errors as PR Comments

[SethFalco]

I think it'd be worth pulling the lint summary from CI and posting them as a comment in the PR?

Many contributors either don't know how or don't bother to check the failed checks. And some that do, don't understand what's wrong because they're only reading the bottom line, or just don't know what to focus on.

It'd massively reduce the workload of maintainers to push the warnings to a PR comment, so the relevant information that contributors need is readily available.

[eshellman]

That would be useful I think. Might be helpful to look at history https://github.com/EbookFoundation/free-programming-books/issues?q=is%3Aissue+linter

[SethFalco]

It should be feasible to migrate from Travis to GitHub Actions. Is there any preference on this? Are we using Travis CI for a reason, or is it just what the repository had before?

Not only to show user's warnings, but to provide more context in specific scenarios.

For example, in:

• Updated #4569 (comment)

It'd be nice to wrap some context around it. If the warnings refer to files not changed in the PR, a failing PR may have accidentally been merged prior. We can declare this explicitly in the comment.

It'd also be possible to make suggestions that users can click to commit, or dispute if they believe it's a false positive or a problem with the linter.

[eshellman]

I think @borgified looked at some of these issues.

[borgified]

hi @SethiPandi i see you noticed we use both travisci and github actions. there's no real reason why we couldn't consolidate to one. i only added the github actions cuz i wanted to learn how to use it. im more familiar with travisci but i think github actions is better integrated to github for obvious reasons.

to your point of wanting to bring the output of the build (in this case, the reasons why a PR failed the checks), if you want to do some digging, there's a way to do it that should already work. just need to implement it/test it to make sure. i'll drop some links for you to get you started in the right direction.

so for example, let's take a recent PR's check: https://github.com/EbookFoundation/free-programming-books/pull/4873/checks?check_run_id=1325849656#step:6:10

we want to surface this output to a more convenient location in the PR, perhaps as a comment. here's one way that this can be done:

see this section about integrating Danger to awesome_bot : https://github.com/dkhamsing/awesome_bot#danger

if i remember correctly, we're already generating the json files (see artifacts) so all we have to do is hook this up (create the Dangerfile and modify github actions to use Danger) and we'll get the nice looking report automatically.

as for the fpb-lint currently running in travisci, i'd say the best way would be to migrate that over to github actions, then employ the same method with Danger to put that in the report too. (prob just need to figure out the format of the json file and generate one)

[SethFalco]

@borgified Understood, and thank you for the links.

The only unfortunate part is, I was hoping we could actually have it automatically offer suggestions, rather than just drop a comment.

- * [My Link] (https://example.com)
+ * [My Link](https://example.com)

(\[.+\])\s+(\(.+?\)) -> $1$2

- * [My Link](https://example.com/)
+ * [My Link](https://example.com)

(?<=\()(https?:\/\/[a-zA-Z\d.-]+)\/(?=\)) -> $1

- * [My Link](https://example.com)  - Seth
+ * [My Link](https://example.com) - Seth

\s+-\s+ -> -

The examples above are fairly simple, but would make for a solid proof of concept. This would maximize convenience for contributors, give them the exact change that needs to be made if they aren't sure, as well as a "Commit" button.

[davorpa]

@SethFalco I found this both snippets that could help using github actions:

• https://rammusxu.github.io/toolkit/snippets/github-action/#get-other-step-output-as-enviroment-in-github-script

  jobs:
    debug:
      runs-on: ubuntu-latest
      steps:
        - id: update
          run: echo ::set-output name=message::okok
        - name: js
          uses: actions/github-script@0.8.0
          env:
            MESSAGE: ${{ steps.update.outputs.message }}
          with:
            github-token: ${{ secrets.GITHUB_TOKEN }}
            script: |
              var message = process.env.MESSAGE
              if (message !== undefined){
                message == 'in'
              } else {
                message == 'else'
              }
              console.log(message)

• https://rammusxu.github.io/toolkit/snippets/github-action/#create-a-comment-in-pull-request

   - name: Notify Results in Pull Request
     if: steps.generate-mirror-list.outputs.images
     uses: actions/github-script@0.3.0
     with:
       github-token: ${{ secrets.GITHUB_TOKEN }}
       images: ${{ steps.generate-mirror-list.outputs.images }}
       script: |
         var body = "## I mirrored something for you"
         process.env.INPUT_IMAGES.split(' ').forEach(function(image){
           let [source, target] = image.split('@')
           body = `${body}\r\n- \`${source}\` -> \`${target}\``
         })
  
         github.issues.createComment({
           ...context.repo,
           issue_number: context.payload.number,
           body: body,
         })

Basically

• Monitor exitcode of linting step.
• If distinct than 0 then there are errors. Save error stream output as action env-var
• Submit a comment to PR using action/github-script (javascript as language) loading that variable as body in next step

In github actions its nice do atomic steps rather all in the same. It aims to isolate errors either each step can have an id and then export outputs/envs as seen in example

[SethFalco]

I think my favorite solution would be split into 2 steps.

There can be a bot on GitHub for Free Ebook Foundation, probably something like ebook-foundation-bot. This can be achieved with GitHub Actions and GitHub CLI, which is pre-installed in all cloud hosted runners.

Step 1

We can extend the fpb-lint GitHub Action to post a comment if linting fails. What it sends is up for discussion.

• Tell the user that the build failed and prompt them to check the output?
• Forward the output of the build as a comment?
• Send diffs with the changes the user should make to get it passing? (Not suggestions!)

This can be done in CLI with gh pr review --body {} --request-changes.

Step 2

Once cli/cli#4056 has been resolved, we'll be able to make suggestions directly from CLI.

To me, it doesn't seem worth creating a custom solution for this from scratch for now. It'll be easier when this is addressed.

[eshellman]

Ok, so what do I need to do?

[SethFalco]

It looks like when using GitHub Actions, there's actually no need to even go through a bot user. 🤔

We can make the pipeline approve or request changes on behalf of fpb-lint as is, it'll be sent by the default github-actions bot user.

With this in mind, I don't think you'd need to do anything:

• We need to remove Travis. (.travis.yml)
• Update all docs to describe GitHub Actions (Translations will have to be done overtime?)
• Deciding what the GitHub Action should post on success and failure.

[davorpa]

    - run: |
        git clone https://github.com/SethFalco/free-programming-books-lint.git
        cd free-programming-books-lint
        git checkout origin/multiple-dirs
        npm i
        npm i -g .

❤️ vhf/free-programming-books-lint#29 Nice improve to handle all files and not only the first directory.

There are other feature pending to be reintegrated: links with pipes in title #5176 (comment). It would be a nice moment to recover it too?

If desirable, I can use sed to clean up the output before posting. (For example removing /home/runner/work/free-programming-books/, etc. In the end I think most tidying up can be done in fpb-lint itself though, like an argument to output relative paths instead of absolute paths.)

I opt for keep project name free-programming-books. Also variable $GITHUB_REPOSITORY has format <owner>/<repo> and then, use it as sed step to get a generalistic common pattern.

Other option would be use remark output formats configuration parameter (docs ▶️ unified-args:reporter), but might be too extensive, so then leave it as desired debt.

[eshellman]

Can we have this working well before Friday?

[SethFalco]

Can we have this working well before Friday?

I think so, the only flaw I've found is that it can be spammy if someone does many small commits.

For example, if one merges suggestions (by humans) one at a time or applies the fix incorrectly multiple times, then the action will put a review for every push.

[eshellman]

Let's try it. Make sure we can revert if needed.

[LuigiImVector]

Based on @SethFalco's workflow I created one that can work without changing the free-programming-books-lint, you can see it working here: LuigiImVector#6 (here is the code)

The things that are missing to be done are::

• Separate push and pr workflows
• Clean up the output (?)

tell me what you think and if you want you can help me develop it

[eshellman]

That looks pretty good! Is it all contained in the github actions config?

[LuigiImVector]

That looks pretty good! Is it all contained in the github actions config?

yes, everything is contained in the gh actions config without using external scripts

[eshellman]

OK to merge, but do it when you can test right away.

…

On Jul 6, 2022, at 10:23 AM, ImVector ***@***.***> wrote: That looks pretty good! Is it all contained in the github actions config? yes, everything in one file without using external scripts — Reply to this email directly, view it on GitHub <#4416 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAHCGMPZF7FAYJVOVVYH5CTVSWJGZANCNFSM4SVZSHOQ>. You are receiving this because you commented.

[LuigiImVector]

(LuigiImVector#7)

I separated the push/pr controls and cleaned up the output, if it is okay I will create a PR in this repo

[davorpa]

@LuigiImVector Great work❤️

If could be possible, I'd prefer clean only the filename path, and leave the lines reported by linter untouched. The linter give more info than a simple message. e.g. line and columns, affected remark plugin...

free-programming-books/books/free-programming-books-langs.md
107:1-131:93    warning    Alphabetical ordering: swap l.109 and l.108    alphabetize-lists    remark-lint

Could be the cleaning script moved to workflow instead of use a Python file? Maybe it's a good moment to use an actions/github-script since provide a context to work with github using nodejs scripts

It'd be great if there are some link to workflow run as part of message. I think could be easy since we have the runid as envvar

[LuigiImVector]

It'd be great if there are some link to workflow run as part of message. I think could be easy since we have the runid as envvar

The workflow logs are the same as the message, so I don't think it's useful to link them.

Maybe it's a good moment to use an actions/github-script since provide a context to work with github using nodejs scripts

I will study how it works and I'll use it.

[github-actions]

This issue has been automatically marked as stale because it has not had recent activity during last 60 days 😴

It will be closed in 30 days if no further activity occurs. To unstale this issue, remove stale label or add a comment with a detailed explanation.

There can be many reasons why some specific issue has no activity. The most probable cause is lack of time, not lack of interest.

Thank you for your patience ❤️