Linting Errors as PR Comments #4416

SethFalco · 2020-10-19T08:16:15Z

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 · 2020-10-19T15:27:08Z

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 · 2020-10-22T11:33:16Z

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 · 2020-10-24T18:07:25Z

I think @borgified looked at some of these issues.

borgified · 2020-10-29T10:14:45Z

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 · 2020-10-29T10:49:52Z

@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 · 2021-07-27T21:42:42Z

@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 · 2021-09-27T00:21:37Z

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 · 2021-09-27T01:04:55Z

Ok, so what do I need to do?

SethFalco · 2021-09-27T10:42:52Z

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 · 2021-09-27T13:48:44Z

    - 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 · 2021-09-27T14:55:56Z

Can we have this working well before Friday?

SethFalco · 2021-09-27T15:02:01Z

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 · 2021-09-27T15:55:54Z

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

LuigiImVector · 2022-07-06T13:28:07Z

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 · 2022-07-06T14:06:49Z

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

LuigiImVector · 2022-07-06T14:23:21Z

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 · 2022-07-06T22:25:26Z

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 · 2022-07-07T17:45:20Z

(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 · 2022-07-10T19:57:22Z

@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 · 2022-07-11T11:02:03Z

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.

…ion#4416

github-actions · 2022-09-13T18:57:33Z

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 ❤️

SethFalco mentioned this issue Dec 9, 2020

Fix Minor Formatting Issues #5212

Merged

davorpa mentioned this issue Aug 8, 2021

lint: Broken links reported from urlchecker #5470

Closed

18 tasks

davorpa added the New Feature New feature / enhancement / translation... label Sep 28, 2021

eshellman changed the title ~~Travis Linting Errors as PR Comments~~ Linting Errors as PR Comments Nov 15, 2021

davorpa mentioned this issue Jan 31, 2022

Make script python to sort the name of courses or books #6690

Open

LuigiImVector added a commit to LuigiImVector/free-programming-books that referenced this issue Jul 6, 2022

build(fpb-lint): linting errors as pr comments EbookFoundation#4416

cc4441d

LuigiImVector added a commit to LuigiImVector/free-programming-books that referenced this issue Jul 13, 2022

build(fpb-lint): linting errors as PR comments, resolved EbookFoundat…

7568a64

…ion#4416

LuigiImVector mentioned this issue Jul 13, 2022

build(fpb-lint): linting errors as PR comments, resolved #4416 #6914

Closed

1 task

This was linked to pull requests Jul 13, 2022

ci(workflow): post build result as comment #5564

Closed

build(fpb-lint): linting errors as PR comments, resolved #4416 #6914

Closed

github-actions bot added the stale Requests that have not had recent interaction (Out-of-Date) label Sep 13, 2022

LuigiImVector added keep Requests exempt from being punctually stale and removed stale Requests that have not had recent interaction (Out-of-Date) labels Sep 13, 2022

davorpa added 👥 discussion This Repo is guided by its community! Let's talk! stale Requests that have not had recent interaction (Out-of-Date) and removed keep Requests exempt from being punctually stale stale Requests that have not had recent interaction (Out-of-Date) labels Sep 13, 2022

SethFalco mentioned this issue Oct 21, 2023

ci(workflow): post build result as comment #10618

Merged

2 tasks

eshellman closed this as completed in #10618 Oct 25, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Linting Errors as PR Comments #4416

Linting Errors as PR Comments #4416

SethFalco commented Oct 19, 2020 •

edited

Loading

eshellman commented Oct 19, 2020

SethFalco commented Oct 22, 2020 •

edited

Loading

eshellman commented Oct 24, 2020

borgified commented Oct 29, 2020

SethFalco commented Oct 29, 2020 •

edited

Loading

davorpa commented Jul 27, 2021 •

edited

Loading

SethFalco commented Sep 27, 2021 •

edited

Loading

eshellman commented Sep 27, 2021

SethFalco commented Sep 27, 2021 •

edited

Loading

davorpa commented Sep 27, 2021

eshellman commented Sep 27, 2021

SethFalco commented Sep 27, 2021 •

edited

Loading

eshellman commented Sep 27, 2021

LuigiImVector commented Jul 6, 2022 •

edited

Loading

eshellman commented Jul 6, 2022

LuigiImVector commented Jul 6, 2022 •

edited

Loading

eshellman commented Jul 6, 2022 via email

LuigiImVector commented Jul 7, 2022 •

edited

Loading

davorpa commented Jul 10, 2022 •

edited

Loading

LuigiImVector commented Jul 11, 2022

github-actions bot commented Sep 13, 2022

Linting Errors as PR Comments #4416

Linting Errors as PR Comments #4416

Comments

SethFalco commented Oct 19, 2020 • edited Loading

eshellman commented Oct 19, 2020

SethFalco commented Oct 22, 2020 • edited Loading

eshellman commented Oct 24, 2020

borgified commented Oct 29, 2020

SethFalco commented Oct 29, 2020 • edited Loading

davorpa commented Jul 27, 2021 • edited Loading

SethFalco commented Sep 27, 2021 • edited Loading

Step 1

Step 2

eshellman commented Sep 27, 2021

SethFalco commented Sep 27, 2021 • edited Loading

davorpa commented Sep 27, 2021

eshellman commented Sep 27, 2021

SethFalco commented Sep 27, 2021 • edited Loading

eshellman commented Sep 27, 2021

LuigiImVector commented Jul 6, 2022 • edited Loading

eshellman commented Jul 6, 2022

LuigiImVector commented Jul 6, 2022 • edited Loading

eshellman commented Jul 6, 2022 via email

LuigiImVector commented Jul 7, 2022 • edited Loading

davorpa commented Jul 10, 2022 • edited Loading

LuigiImVector commented Jul 11, 2022

github-actions bot commented Sep 13, 2022

SethFalco commented Oct 19, 2020 •

edited

Loading

SethFalco commented Oct 22, 2020 •

edited

Loading

SethFalco commented Oct 29, 2020 •

edited

Loading

davorpa commented Jul 27, 2021 •

edited

Loading

SethFalco commented Sep 27, 2021 •

edited

Loading

SethFalco commented Sep 27, 2021 •

edited

Loading

SethFalco commented Sep 27, 2021 •

edited

Loading

LuigiImVector commented Jul 6, 2022 •

edited

Loading

LuigiImVector commented Jul 6, 2022 •

edited

Loading

LuigiImVector commented Jul 7, 2022 •

edited

Loading

davorpa commented Jul 10, 2022 •

edited

Loading