Linting Errors as PR Comments

[SethFalco]

trafficstars

I'm primarily a user of GitLab, so as a maintainer (of small barely used projects XD), I'm not too familiar with Travis CI and GitHub Actions.

I just wanted to suggest, would it be a nice idea to try pull the linting failures from Travis CI and post them as a comment in the PR?

It seems to me lots of people don't know or don't bother to check the failed checks. And some that do, don't see what's wrong because they're only reading the bottom line, or just don't understand it. (Perhaps the terminal like look and monospace font is intimidating/confusing?)

It would probably massively reduce the workload of maintainers to push just the warnings to a PR comment so more users can hopefully fix these themselves.

I assume there is an API or maybe even a data forwarding feature on Travis already? If it's considered a nice idea, I'd gladly try look into it unless any of the current maintainers are already aware of an easy way to achieve this.

[SethFalco]

I've been playing around with it a fair bit and came up with this so far. :thinking:

.travis-ci.yml

language: shell
dist: xenial
os: linux

jobs:
  include:
    -
      language: node_js
      node_js:
        - 6
      before_script:
        - npm install -g free-programming-books-lint
      script:
        - set -o pipefail; fpb-lint . |& tee output.txt
      after_failure:
        - |
          message="The following failures occurred when linting on Travis CI:\n\n$(cat output.txt)"
          message_escaped=$(echo ${message} | sed -n ':a; $ ! {N;ba}; s/\n/\\n/g;p')
          
          if [ "${TRAVIS_PULL_REQUEST}" != "false" ] ; then
            curl -H "Authorization: token ${GITHUB_TOKEN}" -X POST -d "{\"body\":\"${message_escaped}\"}" "https://api.github.com/repos/${TRAVIS_REPO_SLUG}/issues/${TRAVIS_PULL_REQUEST}/comments"
          fi

Will need to add an environment variable to the GitHub access token to an account in order for it to post a comment.

• We first set -set -o pipefail so that when the exit code is still effective even when it's piped. Otherwise it won't have a exit code of 1 on failture since tee was succesful.
• Then we use !& tee output.txt so we keep the ouput of STDOUT and STDERR in console, but also out to a file as well.
• If the build fails, we build the message that we want to send and check if was a pull request.
• If it is a pull request, we drop a comment via the GitHub API with the linting failures that occured.

Result on Failure

Does nothing when the build succeeds, and only acts on Pull Requests. ^I was lazy, that is automated, I just used a personal access token on my personal account rather than creating a seperate bot account for it.

It needs a fair bit of tidying up, but I think it demonstrates the issue quite well, and gets most of the progress for it done already.

Known issues:

• Output could probably like nicer since the goal is to be as friendly as possible and easy to understand.
• The escaping of the JSON for the RESTful request is a bit shoddy, I just replace new lines. It needs to handle quotes etc as well. (I didn't do this fully in case we could use an NPM package to escape JSON instead.)
• Not sure if we really need to create output.txt, wasn't sure how to take output from both STDOUT and STDERR and keep both in an environment variable, while keeping output in the console as well.
• When storing things in environment variables, I'm removing line breaks.
• I'm a bit wary of there's a possibility of an SQL injection like attack, like if a file was named a certain way if the user could trigger it to do echo ${GITHUB_TOKEN}, I don't think this is possible due to file name constraints, but its noteworthy.

Edit: Actually one major problem this has, is that Travis CI will build PRs, even if the PR modifies the .travis-ci.yml file. This means someone could edit the file, and make use of the GitHub Token, at least for that particular pipeline, as far as I understand at least. :thinking: I would assume there is a protective mechanism against this, but not 100% sure what that'd be. (ie don't build pull requests if it modifies .travis-ci.yml)

[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:

• https://github.com/EbookFoundation/free-programming-books/pull/4569#issuecomment-714426400

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/[email protected]
          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/[email protected]
     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 https://github.com/cli/cli/issues/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.

[SethFalco]

name: free-programming-books-lint
on: [push, pull_request]
jobs:
  build:
    runs-on: ubuntu-latest
    steps:
    - uses: actions/checkout@v2
    - name: Use Node.js
      uses: actions/setup-node@v1
      with:
        node-version: '14.x'
    - 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 .
    - run: fpb-lint books casts courses more
      if: github.event_name == 'push'
    - run: |
        fpb-lint books casts courses more 2>&1 | tee output.log
        result_code=${PIPESTATUS[0]}

        if [ $result_code != 0 ]
        then
          gh pr review $PR --body "The build failed with the following output:
          \`\`\`
          $(cat output.log)
          \`\`\`" --request-changes
        else
          gh pr review $PR --approve
        fi

        exit $result_code
      if: github.event_name == 'pull_request'
      env:
        GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
        PR: ${{ github.event.pull_request.html_url }}

The updated GitHub workflow should look something like this. The first run will need to change when/if my PR to fpb-lint is merged and published, using my fork is temporary.

If linting fails, it'll request changes and post the output of the linter. If linting passes, it'll approve the changes (not merge) with no message. (We should probably add a message specifying it's only considering fpb-lint. It doesn't consider other human input or the URL checks.)

We can discuss the desired behavior.

Full example, including an intentional mistake and then fixing it after:

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.)

[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 .

:heart: 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 https://github.com/EbookFoundation/free-programming-books/issues/5176#issuecomment-890391432. 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 :arrow_forward: 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::

• [x] Separate push and pr workflows
• [x] 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 https://github.com/EbookFoundation/free-programming-books/issues/4416#issuecomment-1176287087, 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[bot]]

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

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 :heart: