[issue40551] PRs should be rebased on top of master before running the build/tests

[Filipe Laíns]

Change by Filipe Laíns :


--
resolution:  -> works for me
stage:  -> resolved
status: open -> closed

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue40551] PRs should be rebased on top of master before running the build/tests

[Filipe Laíns]

Filipe Laíns  added the comment:

> Just like Travis, Github actions also automatically rebases pull requests 
> onto the latest master. As you can see from a recent workflow run, it uses 
> the special ref: "refs/remote/pull/20047/merge" 
> https://github.com/python/cpython/pull/20047/checks?check_run_id=665377507#step:2:898

Thanks for letting me know. I did search through the documentation but I hadn't 
found anything regarding this. Well, I guess [1] hinted it but I did not pay 
enough attention :D

> Unless there's real world examples of ongoing problems with PRs being merged 
> with outdated test runs, I don't think this is worth complicating our CIs for.

Although I understand your point, I think this is one of those cases where 
things can easily get out of hand. And when that happens, it could be very 
difficult to revert unless you catch it right away. There's too much noise for 
me to find examples, sorry.

> As Inada-san mentioned, the occasional slip through will eventually be caught 
> by the buildbots and promptly reverted. Re-triggering the CI for very stale 
> pull requests might be worth doing. I'm kinda opposed to an automated system 
> because this creates an undue burden on our CI providers, Travis graciously 
> provides us with extra time for our coverage builds to finish already. 
> Re-running pull requests that might never be merged is a lot of added work 
> for them.

Well, I was not really proposing that. I suggested to retrigger on approvals 
(and by this I mean approvals from core devs) or have a bot retriggering and 
merging automatically.

> Maybe we can get one of the bots to tag old pull requests and then have some 
> guidance in the core-dev guide to re-run the CIs manually when merging those 
> PRs.

Yes, this seems reasonable, and could be expanded in the future. I think we 
should document this and maybe add a bot command to allow devs to retrigger 
more easily (I was thinking they could just comment with /test).

[1] 
https://github.com/actions/checkout#Checkout-pull-request-HEAD-commit-instead-of-merge-commit

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue40551] PRs should be rebased on top of master before running the build/tests

[Ammar Askar]

Ammar Askar  added the comment:

Just like Travis, Github actions also automatically rebases pull requests onto 
the latest master. As you can see from a recent workflow run, it uses the 
special ref: "refs/remote/pull/20047/merge" 
https://github.com/python/cpython/pull/20047/checks?check_run_id=665377507#step:2:898

What is this ref that Github provides?

https://git-scm.com/book/en/v2/GitHub-Maintaining-a-Project
> There’s also a refs/pull/#/merge ref on the GitHub side,
> which represents the commit that would result if you push
> the “merge” button on the site. This can allow you to test
> the merge before even hitting the button


Unless there's real world examples of ongoing problems with PRs being merged 
with outdated test runs, I don't think this is worth complicating our CIs for.

As Inada-san mentioned, the occasional slip through will eventually be caught 
by the buildbots and promptly reverted. Re-triggering the CI for very stale 
pull requests might be worth doing. I'm kinda opposed to an automated system 
because this creates an undue burden on our CI providers, Travis graciously 
provides us with extra time for our coverage builds to finish already. 
Re-running pull requests that might never be merged is a lot of added work for 
them. Maybe we can get one of the bots to tag old pull requests and then have 
some guidance in the core-dev guide to re-run the CIs manually when merging 
those PRs.

--
nosy: +ammar2

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue40551] PRs should be rebased on top of master before running the build/tests

[Filipe Laíns]

Filipe Laíns  added the comment:

> re-trigger the CI, to make everything works correctly with master.

*re-trigger the CI, to make *sure* everything works correctly with master.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue40551] PRs should be rebased on top of master before running the build/tests

[Filipe Laíns]

Filipe Laíns  added the comment:

> "Rather than build the commits that have been pushed to the branch the
pull request is from, we build the merge between the source branch and
the upstream branch."

Okay, great. But this still only happens for Linux. And still has the same 
problem, the build needs to be re-triggered.

> Both are fine, but...

...

> Bot shouldn't use rebase, but merge master branch.
Rebase in pull request branch break the pull request sometime.
See this thread:
https://discuss.python.org/t/info-rebase-origin-master-push-f-workflow-corrupts-pull-request-rarely/2558/7

Okay, so maybe I was not clear enough here. The PR would be rebased locally in 
the CI runs before building and running the tests. Nothing would ever be 
pushed. We don't really want users have to pull their branch before making 
changes, also the users might not give us write access to their branch.

What I am proposing is to do here the same as Travis apparently does, and then 
maybe figure out a workflow where we (semi-/)automatically  re-trigger the CI, 
to make everything works correctly with master.

This is a low risk change. Rethinking the core workflow is the difficult part.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue40551] PRs should be rebased on top of master before running the build/tests

[Inada Naoki]

Inada Naoki  added the comment:

On Sat, May 9, 2020 at 6:08 PM Filipe Laíns  wrote:
>
> > * Travis-CI, at least, does test against "merge commit", not HEAD of PR 
> > branch.
>
> Where? https://github.com/python/cpython/blob/master/.travis.yml

https://docs.travis-ci.com/user/pull-requests/

"Rather than build the commits that have been pushed to the branch the
pull request is from, we build the merge between the source branch and
the upstream branch."

> * Automatically
> * Semi-automatically

Both are fine, but...

>
> Please note in both situations, automatically rebasing PRs *is* the first 
> step. After that, it's just figuring out a suitable model to re-trigger the 
> checks before merging.

Bot shouldn't use rebase, but merge master branch.
Rebase in pull request branch break the pull request sometime.
See this thread:
https://discuss.python.org/t/info-rebase-origin-master-push-f-workflow-corrupts-pull-request-rarely/2558/7

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue40551] PRs should be rebased on top of master before running the build/tests

[Filipe Laíns]

Filipe Laíns  added the comment:

> * Travis-CI, at least, does test against "merge commit", not HEAD of PR 
> branch.

Where? https://github.com/python/cpython/blob/master/.travis.yml

> * As you said already, master branch grow after PR is created anyway.

> This issue proposes " this only works at the time the pull request is 
> created."  It doesn't help that case.

Please note that the proposed fix is just that. I detailed that we should do 
something else, but this was the first step.

> * We run CI and buildbots against master branch too.  So we can find breakage 
> soon when something go wrong.

Right, but then it might not be trivial to add a fix or revert changes. If more 
changes are merged before someone deals with the breakage, it might become very 
difficult to fix, at this point you are playing whack-a-mole reverting 
patches...

So, I disagree with you. This is a real issue. It's not a matter of if, it's a 
matter of when. And a matter of, how bad will it be when it happens?

There are two ways we could go about this:

* Automatically
No-one merges anything manually, they add a tag or comment to trigger the build 
bot. The bot will restart the tests and merge if they pass.

* Semi-automatically
When someone approves the patch, the tests will automatically be re-triggered, 
it then is on the developer to make sure there are no major differences from 
master at the time of merging, to avoid any issues. The tests can be triggered 
manually, but perhaps it would be best to introduce a `/test` command in the 
bots to do this, avoiding the developer to go through a few UI jumps.
If this is chosen, all current developers should be notified, and this added to 
the dev wiki. It also should be noted during the onboard process for new 
developers.

Please note in both situations, automatically rebasing PRs *is* the first step. 
After that, it's just figuring out a suitable model to re-trigger the checks 
before merging.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue40551] PRs should be rebased on top of master before running the build/tests

[Inada Naoki]

Inada Naoki  added the comment:

>  See also 
> https://mail.python.org/archives/list/python-committ...@python.org/thread/WEU5CQKIA4LIHWHT53YA7HHNUY5H2FUT/.
>   This was a problem with other CI GitHub actions when a change had to be 
> merged to master with which all PRs need to be manually rebased to pass.

This is an example of "master branch grow after PR is created anyway."

This issue proposes " this only works at the time the pull request is created." 
 It doesn't help that case.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue40551] PRs should be rebased on top of master before running the build/tests

[Karthikeyan Singaravelan]

Karthikeyan Singaravelan  added the comment:

See also 
https://mail.python.org/archives/list/python-committ...@python.org/thread/WEU5CQKIA4LIHWHT53YA7HHNUY5H2FUT/.
  This was a problem with other CI GitHub actions when a change had to be 
merged to master with which all PRs need to be manually rebased to pass.

--
nosy: +xtreak

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue40551] PRs should be rebased on top of master before running the build/tests

[Inada Naoki]

Inada Naoki  added the comment:

I don't think this is a real issue we should solve:

* Travis-CI, at least, does test against "merge commit", not HEAD of PR branch.
* As you said already, master branch grow after PR is created anyway.
* We run CI and buildbots against master branch too.  So we can find breakage 
soon when something go wrong.

--
nosy: +inada.naoki

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue40551] PRs should be rebased on top of master before running the build/tests

[Filipe Laíns]

New submission from Filipe Laíns :

Let's imagine this scenario:

(master history)
abcd - some commit
hijk - change something

(pull request history)
abcd - some commit
defg - add feature


  master  pull request

abcd - some commit   ---  defg - add feature
hijk - change something


Currently in the CI we checkout defg and run the tests. If hijk introduces some 
change that breaks defg, we only know about it after defg is in master.

I propose pull requests to be automatically rebased on top of master, so that 
the resulting story becomes:

abcd - some commit
hijk - change something
defg - add feature

Now defg is properly tested :)

This may not be an issue in smaller projects, but in cpython where most things 
are a moving target, it should make a difference. Here I am referring to master 
but applies to all other maintained branches.

Github can't merge if the PR doesn't cleanly rebase to master, or rather, it 
will ask you to resolve conflicts, so I don't think there should be any problem.

Keep in mind that this only works at the time the pull request is created. To 
check against the latest master the build needs to be retriggered, which you 
should be able to do manually.
Even if we don't check against the latest master when the PR is merged, this is 
already an improvement. After this we could look into maybe automatically 
retriggering the CI on approvals, or better, managing merges with a bot.

--
components: Build
messages: 368371
nosy: FFY00
priority: normal
severity: normal
status: open
title: PRs should be rebased on top of master before running the build/tests
type: enhancement
versions: Python 3.9

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com