lint: PR description linter

[fjahr]

This is a proof of concept of my idea briefly outlined here #18380 (comment). The code is partially based on the merge script.

The new linter uses the pull request number available in the Travis env and the Github API to retrieve the PR description and checks it for common errors, currently @-prefixed GH usernames and fractional left-overs of the pull request template (identified by HTML comments). Currently, users mostly have to be reminded here manually to fix these things or they are caught in the merge process. Obviously this linter can be expanded to check for more similar issues, those are just the ones I am familiar with. For example, usernames in ACK messages could be added as well, as mentioned here: bitcoin-core/bitcoin-maintainer-tools#51

If #18380 gets merged, the HTML comments part is useless but we could put a unique string in the bottom of the pull request template and then match against that. Alternatively, this PR could be a replacement for #18358

Open question: I was not sure if the script should be more forgiving. It expects to be run in Travis and otherwise fails. I think this makes sense because the user would have to provide the PR number manually if they want to run it locally. It also fails if there is an issue with the Github API which seems to have problems quite frequently. On the other hand, if Github is down, the CI checks should probably not be running anyway.

[fjahr]

Rate limiting of the Travis IPs seems to make the use of an authenticated request mandatory, added that.

[laanwj]

Concept ACK, thanks for working on this.

[practicalswift]

Concept ACK: Automation is good

[promag]

Concept ACK.

[theStack]

Concept ACK

Testing the script, I couldn't trigger a linting error with the script when run on a PR with HTML comments in the description. E.g. run on PR #18376
$ TRAVIS_PULL_REQUEST=18376 ./check-pr-description.py
didn't output anything when it should, see a421e0a for the commit message. That likely has to do with the search strings that have to be adapted (see my two code review comments plus change suggestions).

[maflcko]

Not sure if this is worth it. If it is extended to check for @ in any comment (as suggested in the OP), it is a DOS vector where anyone can block the travis run/make it fail by just posting a comment. In that case maintainers would have to run around and remove the @ to just get travis to run.

I think it should be sufficient to have the check in the merge script

[fjahr]

Not sure if this is worth it. If it is extended to check for @ in any comment (as suggested in the OP), it is a DOS vector where anyone can block the travis run/make it fail by just posting a comment. In that case maintainers would have to run around and remove the @ to just get travis to run.

I think it should be sufficient to have the check in the merge script

That makes a lot of sense, thanks!

[fjahr]

Thanks to @theStack 's feedback the regex should work properly now. I also simplified the GH username regex and added some documentation.

Here are some test vectors:

Fail due to HTML tags:

$ TRAVIS_PULL_REQUEST=16439 ./check-pr-description.py

Fail due to GH username:

$ TRAVIS_PULL_REQUEST=18376 ./check-pr-description.py

Since these are active the descriptions could be fixed any day so if it does not work withthese please check first if that is the case.

For the fun of it, you can test ranges of PRs like so but you will see 404 errors for each number that is an issue and not a pull:

COUNTER=18000; while [ $COUNTER -lt 18020 ]; do TRAVIS_PULL_REQUEST=$COUNTER test/lint/check-pr-description.py; let COUNTER+=1; done

[fjahr]

I thought there was a github access token available in the git config inside Travis but it appears their isn't and not running into rate limiting last time was a false positive. I am trying to find out the variable name if there is one available in the Travis environment, otherwise it would need to be added.

[theStack]

FWIW, with the help of a quick git log --grep="-->" and further manual inspection I could find two false positives, i.e. PRs that contain no HTML comments but still the linter would complain:

• rpc: improve getaddressinfo test coverage, help, code docs #17283 ($ TRAVIS_PULL_REQUEST=17283 ./check-pr-description.py)
• [Docs] Improve formatting of developer notes #12759 ($ TRAVIS_PULL_REQUEST=12759 ./check-pr-description.py)

[fjahr]

FWIW, with the help of a quick git log --grep="-->" and further manual inspection I could find two false positives, i.e. PRs that contain no HTML comments but still the linter would complain:

• rpc: improve getaddressinfo test coverage, help, code docs #17283 ($ TRAVIS_PULL_REQUEST=17283 ./check-pr-description.py)
• [Docs] Improve formatting of developer notes #12759 ($ TRAVIS_PULL_REQUEST=12759 ./check-pr-description.py)

Thanks for discovering these. So yes, the current version does give false positives any time someone wants to write something in the PR description that actually includes HTML comments. The only effective way I could think of to mitigate that would be to use the solution with the unique string in the template. It would work like this: at the bottom of the template, there is something like TEMPLEATE_END_PLEASE_REMOVE_ME or so and then this linter checks for that string. I am happy to hear feedback if that solution is preferred to prevent these false positives.

[fjahr]

Unfortunately, it looks like this is blocked. I was trying to get a Github Oauth Token into the Travis build environment as a secure (encrypted) variable but this variable can still not be accessed by pull requests triggered through forks since the variable would be exposed to unknown code (see travis docs) This makes sense and I currently only see this working if the pull body does get added to the standard Travis environment variables. There exists an issue for this travis-ci/travis-ci#9302 and a pull request is open to add the title at least travis-ci/travis-build#1319 but it has been open for 2 years.

I am happy to hear if there are other ideas on how to achieve this. Otherwise, I will close this and reopen when the pull body becomes available in Travis somehow. But I don't think there are any other obvious solutions as they should have come up in the Travis issues on this topic.