[10217] Run newsfragment checks based on the exact branch code, not the to-be-merged code.

[adiroiban]

Scope and purpose

The main reason for doing that is so that newsframent will be executed on the exact branch code and to avoid errors generated on the release branch.

The GitHub pull_request events are executed on a to-be-merged branch that does not has the same name as the tested branch, and the newsfragment checker fails to detect it as a release branch.

And when trunk contains features are are not to be released yet, the merge branch will contain those release fragments from trunk and the check fill fail as the release branch should have no release fragments.

Contributor Checklist:

• The associated ticket in Trac is here: https://twistedmatrix.com/trac/ticket/10217
• [NA] I ran tox -e lint to format my patch to meet the Twisted Coding Standard
• I have created a newsfragment in src/twisted/newsfragments/ (see: News files)
• The title of the PR starts with the associated Trac ticket number (without the # character).
• [NA] I have updated the automated tests and checked that all checks for the PR are green.
• I have submitted the associated Trac ticket for review by adding the word review to the keywords field in Trac, and putting a link to this PR in the comment; it shows up in https://twisted.reviews/ now.
• The merge commit will use the below format
  The first line is automatically generated by GitHub based on PR ID and branch name.
  The other lines generated by GitHub should be replaced.

Merge pull request #123 from twisted/4356-branch-name-with-trac-id

Author: adiroiban
Reviewer: 
Fixes: ticket:10217

Run  newsfragment checkers based on the branch head and not the merge code.

[adiroiban]

This is ready for review.

I am not sure what are the side effects for not running pyflakes check on the to-be-merged code.

Ideally for the release branch we want to run on push and for the other branches we want to run on pull_request (merge).

You can create something like

name: Lint

on:
  push:
    branches: [ 'trunk', 'release-*' ]
  pull_request:
    branches: [ trunk ]

but then the same commit will be execute for both push and pull_request ...with conflicting results.

---

You can see a failed PR test run for a release branch here https://github.com/twisted/twisted/runs/3000559343#step:5:89

branch name is release-21.7.0-10214

but newsfragmentcheckers thinks that this is not a release branch

Looking at these files:
NEWS.rst
README.rst
docs/core/development/policy/release-process.rst
src/twisted/_version.py
src/twisted/web/_stan.py
src/twisted/web/test/test_stan.py
----
No newsfragment found. Have you committed it?

[altendky]

Won't this not build for out-of-repo PRs? I mean yeah, it's a mess that GitHub defaults to checking out the automatic merge commit, but there's an option for that. Though it still uses the automatic merge commit for at least the initial (if not all) workflow definition.

[adiroiban]

Won't this not build for out-of-repo PRs? I mean yeah, it's a mess that GitHub defaults to checking out the automatic merge commit,

True. Thanks for the feedback.

Maybe have something like this

name: Lint

on:
  push:
    branches: [ 'trunk', 'release-*' ]
  pull_request:
    branches: [ trunk ]

and skip the pull_request run for a release branch.

I will see what can be done.

[adiroiban]

One crazy idea.

On GitHub Action detect that the job is executed for a pull_request and revert the last commit and run the newsfragment check.

[adiroiban]

Thanks for the review and feedback.

I reverted the change so that we will have the checks executed on a pull_request event... to have it executed for external forks.

But for a release branch (a branch starting with release- it will revert the merge)

You can see the merge reverting code here on a release branch: https://github.com/twisted/twisted/pull/1614/checks?check_run_id=3022301290#step:5:11

[altendky]

Are you sure you don't want to just always checkout the not-merged commit?

docs: https://github.com/actions/checkout#checkout-pull-request-head-commit-instead-of-merge-commit
example: https://github.com/python-qt-tools/qts/blob/a9f0da7cb6039eaf64ac7dc15d834742b7250ee9/.github/workflows/ci.yml#L39-L41

      - uses: actions/checkout@v1
        with:
          ref: ${{ github.event.pull_request.head.sha }}

You may also want a more complete checkout for the newsfragment check so that towncrier can actually see the other branch.

docs: https://github.com/actions/checkout#fetch-all-history-for-all-tags-and-branches
example: https://github.com/twisted/towncrier/blob/5dce0fad4973b859a68896b51ac87ec46c0f0312/.github/workflows/ci.yml#L148-L150

    - uses: actions/checkout@v2
      with:
        fetch-depth: 0

Hopefully I'm going down a useful path here...

[adiroiban]

Are you sure you don't want to just always checkout the not-merged commit?

Good question... but yes I want to run as many checks as possible based on the to-be-merged code.

And only run newsfragment checks on the non-merged code.

I think that for pyflakes and other checks we want to check the merged code as this is what will be committed and we don't want to risk introducing errors in trunk.

And I think that for a release branch, we also want to run pyflakes on the merge code just to prevent any strange errors to be triggered after the release branch is merged.

So, unless there are any major unwanted side-effects I would prefer to merge this code as it is.

If found out that this is a bad idea, we can always update it.

But for now, I don't see why the current code is a bad idea :)

Thanks for the review

[altendky]

I still prefer just directly checking out the pushed code as mentioned in #1617 (comment), but this seems like it will work.

To put some extra commentary here (I already discussed this with Adi on IRC), I consider GitHub's checking out of an automatically generated merge commit to be pretty darn bad. The common argument for it is that "then the PR build shows what it would do if merged". Except that that is untrue as soon as a new commit is added to the PR's target branch. Remember, the PR isn't rebuilt then, it just sits there claiming to be happy as long as an automatic merge can be done. So, you just have this idea in your head that the PR build is representative when it isn't. The actual solution to getting close to knowing the PR build is representative of what the post-merge build will be is to require that PRs be up to date with their target branches (which we do, I believe). The remaining differences are any build mechanisms that check the actual branch or trigger info etc.

[graingert]

I still prefer just directly checking out the pushed code as mentioned in #1617 (comment), but this seems like it will work.

To put some extra commentary here (I already discussed this with Adi on IRC), I consider GitHub's checking out of an automatically generated merge commit to be pretty darn bad. The common argument for it is that "then the PR build shows what it would do if merged". Except that that is untrue as soon as a new commit is added to the PR's target branch. Remember, the PR isn't rebuilt then, it just sits there claiming to be happy as long as an automatic merge can be done. So, you just have this idea in your head that the PR build is representative when it isn't. The actual solution to getting close to knowing the PR build is representative of what the post-merge build will be is to require that PRs be up to date with their target branches (which we do, I believe). The remaining differences are any build mechanisms that check the actual branch or trigger info etc.

Probably worth re-investigating if GHA ever supports merge trains

[adiroiban]

Thanks for your feedbacks. I have merged it as I wanted to have a green release branch.

I think that we can re-consider at any time the way we run the static checks.