Mailman 3 self-approving pull requests - core-workflow

newer
My new, asynchronous GitHub library

self-approving pull requests

Barry Warsaw

17 Feb 2017 17 Feb '17

10:39 p.m.

I submitted PR#138 on bpo-22807. I got a nice review from a community member and made a small change. All checks have passed. But now I'm stuck and I'm impatient. ;) The change is small enough and I'm happy with it, and I could patiently wait for another core dev to approve it, but in the likely case that no one else is interested in the bug, I'd like to be able to self-approve and accept it. This of course is described as my right in the devguide, along with the responsibility to watch the buildbots and revert the change if there are any problems with it. But it seems like I cannot do that through the GH UI. Right now I'm seeing (X) Review required (✓) All checks have passed (X) Merge is blocked There seems to be no way to self-approve the PR. If I go to 'Files changed' tab and click on 'Review changes', both Approve and Request changes is dimmed. I can only comment on my own PR. I can understand why we might want to prohibit self-approvals, but that's also a regression in the permissions and rights of core developers. I also think it's counterproductive since without that I might have to go begging for a review approval before the branch can land. Is this intentional or an oversight? (As an owner of the project, I took a quick look at the project settings and couldn't find a control for this, but I know other GH projects I contribute to support self-approval.) Cheers, -Barry

Attachments:

attachment.sig (application/pgp-signature — 801 bytes)

Show replies by date

Donald Stufft

17 Feb 17 Feb

11:24 p.m.

We turned on require code review for the PR, though at the time I *thought* it still allowed you to approve your own PR. Apparently that was wrong. Brett was the one to actually make the decision to do it and turned it on, so I don’t know if he knew that it didn’t allow people to self-approve or not. The impetus to do this was to make sure that all changes went through the PR process and folks didn’t directly push to `master`. So our choices at this time are: 1) Leave it requiring a review from someone else with write or admin permissions on the repository, which will continue to require a PR. 2) Turn it off, but turn on requiring status checks which will still effectively require a PR (there is one way around this, but it is so convoluted nobody would be able to do it by accident, and things still get tested anyways). 3) Turn them both off, and just tell people not to push to `master` directly and always go through the PR process. Personally I think that (2) is a good option if our test suite is stable on Travis now. We *want* to make core contributors to generally go through the same process as non-core contributors in order to better expose pain points introduced by our process so we can find them and fix them. Although arguably reviewer bandwidth is a pain point in our process I think that it might be special enough to just switch to the required status check instead.

...

On Feb 17, 2017, at 5:39 PM, Barry Warsaw wrote:

I submitted PR#138 on bpo-22807. I got a nice review from a community member and made a small change. All checks have passed.

But now I'm stuck and I'm impatient. ;)

The change is small enough and I'm happy with it, and I could patiently wait for another core dev to approve it, but in the likely case that no one else is interested in the bug, I'd like to be able to self-approve and accept it. This of course is described as my right in the devguide, along with the responsibility to watch the buildbots and revert the change if there are any problems with it.

But it seems like I cannot do that through the GH UI. Right now I'm seeing

(X) Review required (✓) All checks have passed (X) Merge is blocked

There seems to be no way to self-approve the PR. If I go to 'Files changed' tab and click on 'Review changes', both Approve and Request changes is dimmed. I can only comment on my own PR.

I can understand why we might want to prohibit self-approvals, but that's also a regression in the permissions and rights of core developers. I also think it's counterproductive since without that I might have to go begging for a review approval before the branch can land.

Is this intentional or an oversight?

(As an owner of the project, I took a quick look at the project settings and couldn't find a control for this, but I know other GH projects I contribute to support self-approval.)

Cheers, -Barry

_______________________________________________ core-workflow mailing list core-workflow@python.org https://mail.python.org/mailman/listinfo/core-workflow This list is governed by the PSF Code of Conduct: https://www.python.org/psf/codeofconduct

— Donald Stufft

Barry Warsaw

11:32 p.m.

On Feb 17, 2017, at 06:24 PM, Donald Stufft wrote:

...

2) Turn it off, but turn on requiring status checks which will still effectively require a PR (there is one way around this, but it is so convoluted nobody would be able to do it by accident, and things still get tested anyways).

I do like requiring PRs, prohibiting direct pushes to master, feeling non-core contributor pain points, and getting reviews. :) And sometimes it makes sense to actually get an ack from another core contributor in order to land a change, but I think we need to mostly leave that to the discretion of the core contributor. As you say, reviewer bandwidth is a bottleneck (something that I'm personally going to try to help with now that we're on GH), so at least for now, I agree that #2 makes for a good choice. Cheers, -Barry

Senthil Kumaran

11:42 p.m.

On Fri, Feb 17, 2017 at 3:32 PM, Barry Warsaw wrote:

...

As you say, reviewer bandwidth is a bottleneck

Would anyone consider "Requiring a review from another core-developer" is actually a helpful thing? The positives I see are: 1) Forcing other developers with commit rights to act soon and review each other's code more often. 2) As a result of 1, the developers are incentivized to stay involved either in reviews, by reading code and subsequently will be fast in contributing code. For me, I am +1 in staying this way and I don't mind if we think that 'review bottleneck' needs to be tackled by allowing core-dev to commit his own code without another view. -- Senthil

Nick Coghlan

22 Feb 22 Feb

7:42 a.m.

On 18 February 2017 at 05:12, Senthil Kumaran wrote:

...

On Fri, Feb 17, 2017 at 3:32 PM, Barry Warsaw wrote:

...
As you say, reviewer bandwidth is a bottleneck

Would anyone consider "Requiring a review from another core-developer" is actually a helpful thing?

No, because we don't have anything close to the reviewer bandwidth needed to keep the previous "single review needed" process running smoothly, let alone a "double review needed (even on backports)" process.

...

The positives I see are:

1) Forcing other developers with commit rights to act soon and review each other's code more often.

Are you offering to pay everyone for the time spent on these additional reviews? Remember, disallowing self-review will come close to doubling the aggregate review bandwidth required for the core development process (especially with backports requiring a PR-per-branch).

...

2) As a result of 1, the developers are incentivized to stay involved either in reviews, by reading code and subsequently will be fast in contributing code.

What incentive? I'm *less* inclined to contribute right now, and less inclined to mentor people, since I can get very little done on my own, and instead even the simplest of tasks can drag on for 24 or 48 hours (There are relatively few other core developers in my time zone, so folks in the US and Europe saying "I don't have any trouble finding extra reviewers" doesn't carry a lot of weight with me on this point). I'm +1 for turning on required status checks though - giving us a strong incentive to get the test suite stable and keep it that way is an unequivocally good thing, and I do want to keep the "PR required, even for core developers" experiment going for at least another few weeks. Cheers, Nick. -- Nick Coghlan | ncoghlan@gmail.com | Brisbane, Australia

Nick Coghlan

7:51 a.m.

On 22 February 2017 at 13:12, Nick Coghlan wrote:

...

I'm +1 for turning on required status checks though - giving us a strong incentive to get the test suite stable and keep it that way is an unequivocally good thing, and I do want to keep the "PR required, even for core developers" experiment going for at least another few weeks.

To be clear: my preference would be to have the setup be "Review required, but self-review is permitted", as that lets us decided whether or not it makes sense to wait for the CI to run. But if GitHub doesn't allow that, then I think "Successful CI run required" would be a better near term gate than preventing developers from merging our own changes (even when we're importing someone else's patch from bugs.python.org or just fixing a typo in the docs) Cheers, Nick. -- Nick Coghlan | ncoghlan@gmail.com | Brisbane, Australia

Maciej Szulik

12:24 p.m.

On Wed, Feb 22, 2017 at 8:51 AM, Nick Coghlan wrote:

...

On 22 February 2017 at 13:12, Nick Coghlan wrote:

...
I'm +1 for turning on required status checks though - giving us a strong incentive to get the test suite stable and keep it that way is an unequivocally good thing, and I do want to keep the "PR required, even for core developers" experiment going for at least another few weeks.

To be clear: my preference would be to have the setup be "Review required, but self-review is permitted", as that lets us decided whether or not it makes sense to wait for the CI to run.

If I'm correct, gh will grey-out (but completly block) merge when all the checks are not green. Green CI should be helpful here. Eventually we could add some additional checks for self-approval awareness in our bot.

...

But if GitHub doesn't allow that, then I think "Successful CI run required" would be a better near term gate than preventing developers from merging our own changes (even when we're importing someone else's patch from bugs.python.org or just fixing a typo in the docs)

Donald Stufft

4:12 p.m.

...

On Feb 22, 2017, at 2:42 AM, Nick Coghlan wrote:

I'm +1 for turning on required status checks though - giving us a strong incentive to get the test suite stable and keep it that way is an unequivocally good thing, and I do want to keep the "PR required, even for core developers" experiment going for at least another few weeks.

I’m happy to switch this around, but I don’t know whose call it is. Does Brett need to make this call? I dunno. — Donald Stufft

Barry Warsaw

4:26 p.m.

On Feb 22, 2017, at 11:12 AM, Donald Stufft wrote:

...

I’m happy to switch this around, but I don’t know whose call it is. Does Brett need to make this call? I dunno.

Brett should at least get a chance to weigh in, so let's wait for him to return. -Barry

Senthil Kumaran

4:35 p.m.

On Wed, Feb 22, 2017 at 8:12 AM, Donald Stufft wrote:

...

I’m happy to switch this around, but I don’t know whose call it is. Does Brett need to make this call? I dunno.

I am +1 on turning on the required status check before the merge is enabled. It was briefly discussed here: https://mail.python.org/pipermail/python-committers/2017-February/004255.htm... We can use that thread to bring it to a closure. I understand Nick's point of view in this thread. It might be that given my time zone or chance, I could get reviewers easily. And, I could pull myself to review small patches. With our previous VCS, we have trusted core-developers to take appropriate actions. So, for any significant changes, I assume, developers will involve another developer for the review. And for non-controversial, straightforward improvements, they would be free to commit stuff (as it was the case with hg). Github allows such a provision, if we decide to go for it, we should know that no extra/special tooling should be required. Thanks, Senthil

Donald Stufft

17 Feb 17 Feb

11:50 p.m.

https://github.com/python/core-workflow/issues/32 might help as well. Sent from my iPhone

...

On Feb 17, 2017, at 6:32 PM, Barry Warsaw wrote:

As you say, reviewer bandwidth is a bottleneck (something that I'm personally going to try to help with now that we're on GH), so at least for now, I agree that #2 makes for a good choice.

Nick Coghlan

22 Feb 22 Feb

3:22 p.m.

On 18 February 2017 at 09:24, Donald Stufft wrote:

...

We turned on require code review for the PR, though at the time I *thought* it still allowed you to approve your own PR. Apparently that was wrong. Brett was the one to actually make the decision to do it and turned it on, so I don’t know if he knew that it didn’t allow people to self-approve or not. The impetus to do this was to make sure that all changes went through the PR process and folks didn’t directly push to `master`.

While this is a good ideal to strive for, I'm starting to think it isn't practical with our current development set up. The trigger for this change in my thinking was going to merge https://github.com/python/cpython/pull/171 which is basically fine, but needs a few clean-ups prior to committing: - Misc/ACKS update - Misc/NEWS entry - What's New entry - (and probably a rebase due to Misc/NEWS) It's generally easier to just add those myself rather than asking the contributor to do it, and then the contributor can look at the final commit to see what these additional bookkeeping entries look like. However, without the ability to commit locally and then push to GitHub from there, you end up with one of two options: - attempt to amend the original PR (see https://github.com/python/devguide/issues/129 ). That may not work depending on how the contributor has set up their fork and PR branch. - create a new PR and close the original one (with the corresponding subpar experience for the original contributor) So as much as I'd like to keep it, it probably makes sense to turn off branch protection for now and instead start making a list of the things that would be needed to turn it back on, starting with: - a way of reliably editing PRs that doesn't require closing them and creating new ones - conflict-free NEWS entry definition - a way for core developers to handle minor changes autonomously rather than always needing someone else to be online and responsive Cheers, Nick. -- Nick Coghlan | ncoghlan@gmail.com | Brisbane, Australia

Maciej Szulik

3:59 p.m.

On Wed, Feb 22, 2017 at 4:22 PM, Nick Coghlan wrote:

...

...
We turned on require code review for the PR, though at the time I *thought* it still allowed you to approve your own PR. Apparently that was wrong. Brett was the one to actually make the decision to do it and turned it on, so I don’t know if he knew that it didn’t allow people to self-approve or not. The impetus to do this was to make sure that all changes went

On 18 February 2017 at 09:24, Donald Stufft wrote: through

...
the PR process and folks didn’t directly push to `master`.

While this is a good ideal to strive for, I'm starting to think it isn't practical with our current development set up.

The trigger for this change in my thinking was going to merge https://github.com/python/cpython/pull/171 which is basically fine, but needs a few clean-ups prior to committing:

- Misc/ACKS update - Misc/NEWS entry - What's New entry - (and probably a rebase due to Misc/NEWS)

It's generally easier to just add those myself rather than asking the contributor to do it, and then the contributor can look at the final commit to see what these additional bookkeeping entries look like.

However, without the ability to commit locally and then push to GitHub from there, you end up with one of two options:

- attempt to amend the original PR (see https://github.com/python/devguide/issues/129 ). That may not work depending on how the contributor has set up their fork and PR branch. - create a new PR and close the original one (with the corresponding subpar experience for the original contributor)

You should be able to cherry-pick his commit. For that it's worth setting your repo to be able to checkout any PR submitted to a repo: https://gist.github.com/piscisaureus/3342247 with that you can basically do git checkout pr/XXX and from that you can easily get the commit id and be able to cherry-pick it, amend it or add new thing on top of it. In the end submitting a new PR, replacing the original one.

...

So as much as I'd like to keep it, it probably makes sense to turn off branch protection for now and instead start making a list of the things that would be needed to turn it back on, starting with:

- a way of reliably editing PRs that doesn't require closing them and creating new ones - conflict-free NEWS entry definition - a way for core developers to handle minor changes autonomously rather than always needing someone else to be online and responsive

Cheers, Nick.

-- Nick Coghlan | ncoghlan@gmail.com | Brisbane, Australia _______________________________________________ core-workflow mailing list core-workflow@python.org https://mail.python.org/mailman/listinfo/core-workflow This list is governed by the PSF Code of Conduct: https://www.python.org/psf/codeofconduct

Donald Stufft

4:10 p.m.

...

On Feb 22, 2017, at 10:22 AM, Nick Coghlan wrote:

- attempt to amend the original PR (see https://github.com/python/devguide/issues/129 https://github.com/python/devguide/issues/129 ). That may not work depending on how the contributor has set up their fork and PR branch. - create a new PR and close the original one (with the corresponding subpar experience for the original contributor)

FWIW, I don’t think that creating a new PR and closing the original one is a subpar experience for contributors, particularly if we turn off the bit that requires reviews and just turn on the thing that requires passing tests. Having been in that situation it has never once bothered me to have someone cherry pick my change and amend it. — Donald Stufft

Barry Warsaw

4:25 p.m.

On Feb 22, 2017, at 11:10 AM, Donald Stufft wrote:

...

FWIW, I don’t think that creating a new PR and closing the original one is a subpar experience for contributors, particularly if we turn off the bit that requires reviews and just turn on the thing that requires passing tests. Having been in that situation it has never once bothered me to have someone cherry pick my change and amend it.

Since we're squashing commits wouldn't that obliterate the original author's credit for the work? Cheers, -Barry

Donald Stufft

4:39 p.m.

...

On Feb 22, 2017, at 11:25 AM, Barry Warsaw wrote:

Since we're squashing commits wouldn't that obliterate the original author's credit for the work?

Yes, github will credit the person who opened the PR, but the person who made the person who made the PR can check the box (which I *believe* is checked by default) to allow maintainers to edit their PR. If that is checked, then maintainers can edit their branch on their fork directly, in which case no credit gets lost. So just make your changes directly in their branch, and things will continue to work. — Donald Stufft

Ethan Furman

7:16 p.m.

On 02/22/2017 08:39 AM, Donald Stufft wrote:

...

...
On Feb 22, 2017, at 11:25 AM, Barry Warsaw wrote:

...

...
Since we're squashing commits wouldn't that obliterate the original author's credit for the work?

Yes, github will credit the person who opened the PR, but the person who made the person who made the PR can check the box (which I *believe* is checked by default) to allow maintainers to edit their PR. If that is checked, then maintainers can edit their branch on their fork directly, in which case no credit gets lost.

So just make your changes directly in their branch, and things will continue to work.

Is there a list of instructions somewhere on how to do all that? -- ~Ethan~

Donald Stufft

7:26 p.m.

You can clone their repository and make changes directly to their branch and push to it I believe, assuming they allowed that when they made the PR. You should also be able to add their fork as a remote on your current branch using 'git remote add foobar githuburl' then checkout the got branch from them. Sent from my iPhone

...

On Feb 22, 2017, at 2:16 PM, Ethan Furman wrote:

On 02/22/2017 08:39 AM, Donald Stufft wrote:

...
...
On Feb 22, 2017, at 11:25 AM, Barry Warsaw wrote:

...
...
Since we're squashing commits wouldn't that obliterate the original author's credit for the work?

Yes, github will credit the person who opened the PR, but the person who made the person who made the PR can check the box (which I *believe* is checked by default) to allow maintainers to edit their PR. If that is checked, then maintainers can edit their branch on their fork directly, in which case no credit gets lost.

So just make your changes directly in their branch, and things will continue to work.

Is there a list of instructions somewhere on how to do all that?

-- ~Ethan~ _______________________________________________ core-workflow mailing list core-workflow@python.org https://mail.python.org/mailman/listinfo/core-workflow This list is governed by the PSF Code of Conduct: https://www.python.org/psf/codeofconduct

Nick Coghlan

23 Feb 23 Feb

4:29 a.m.

On 23 February 2017 at 05:26, Donald Stufft wrote:

...

You can clone their repository and make changes directly to their branch and push to it I believe, assuming they allowed that when they made the PR.

This is the part that GitHub hasn't documented very well from the maintainer side yet, but I think I just worked out the appropriate incantation: https://github.com/python/devguide/issues/129#issuecomment-281891204 The trick is that you have to use the "git@github.com:contributor_name/cpython.git" spelling when defining the remote or GitHub won't allow SSH-based pushes back to their repo, even if they've granted maintainer access to their PR branch. Cheers, Nick. -- Nick Coghlan | ncoghlan@gmail.com | Brisbane, Australia

Nick Coghlan

4:17 a.m.

On 23 February 2017 at 02:25, Barry Warsaw wrote:

...

On Feb 22, 2017, at 11:10 AM, Donald Stufft wrote:

...
FWIW, I don’t think that creating a new PR and closing the original one is a subpar experience for contributors, particularly if we turn off the bit that requires reviews and just turn on the thing that requires passing tests. Having been in that situation it has never once bothered me to have someone cherry pick my change and amend it.

Since we're squashing commits wouldn't that obliterate the original author's credit for the work?

You can set "--author" when making the new squash commit (we should document that somewhere, since it's also useful when importing patches from bugs.python.org). Although looking at https://github.com/python/cpython/pull/169 (where I set the author to Serhiy, since I was importing a patch he wrote), it seems GitHub doesn't actually *show* the Author information anywhere that I can find. Cheers, Nick. -- Nick Coghlan | ncoghlan@gmail.com | Brisbane, Australia

Donald Stufft

4:18 a.m.

...

On Feb 22, 2017, at 11:17 PM, Nick Coghlan wrote:

On 23 February 2017 at 02:25, Barry Warsaw wrote:

...
On Feb 22, 2017, at 11:10 AM, Donald Stufft wrote:

...
FWIW, I don’t think that creating a new PR and closing the original one is a subpar experience for contributors, particularly if we turn off the bit that requires reviews and just turn on the thing that requires passing tests. Having been in that situation it has never once bothered me to have someone cherry pick my change and amend it.

Since we're squashing commits wouldn't that obliterate the original author's credit for the work?

You can set "--author" when making the new squash commit (we should document that somewhere, since it's also useful when importing patches from bugs.python.org).

Although looking at https://github.com/python/cpython/pull/169 (where I set the author to Serhiy, since I was importing a patch he wrote), it seems GitHub doesn't actually *show* the Author information anywhere that I can find.

Github automatically sets author of the squash commit based on who opened the PR. — Donald Stufft

Brett Cannon

25 Feb 25 Feb

12:17 a.m.

Two things. One, has someone verified that if a core dev edits a PR that the squash commit still gives the PR creator the author credit in the git metadata? I remember doing an edit like this once and GitHub didn't show any author credit in the web UI because I assume GitHub refused to guess who the author credit should go to. So if someone could test this in a checkout that would be great as that means https://github.com/python/core-workflow/issues/7 can be easily solved and we can automate Misc/ACKS. Two, we are going to review the new workflow in two weeks after having been using it for a month (I can't believe it's only been two weeks since the migration!). Since the sign-off requirement has generated the most discussion, what I will do is swap the requirement for PR merging to be no required review but to require all-green status checks (in a previous email Donald alluded to the fact that I thought self-approval would be possible in "emergencies" but that obviously doesn't hold). This will give us basically 2 weeks with both approaches when we review the process so we can make an informed decision. On Wed, 22 Feb 2017 at 20:19 Donald Stufft wrote:

...

On Feb 22, 2017, at 11:17 PM, Nick Coghlan wrote:

On 23 February 2017 at 02:25, Barry Warsaw wrote:

On Feb 22, 2017, at 11:10 AM, Donald Stufft wrote:

FWIW, I don’t think that creating a new PR and closing the original one is a subpar experience for contributors, particularly if we turn off the bit that requires reviews and just turn on the thing that requires passing tests. Having been in that situation it has never once bothered me to have someone cherry pick my change and amend it.

Since we're squashing commits wouldn't that obliterate the original author's credit for the work?

You can set "--author" when making the new squash commit (we should document that somewhere, since it's also useful when importing patches from bugs.python.org).

Although looking at https://github.com/python/cpython/pull/169 (where I set the author to Serhiy, since I was importing a patch he wrote), it seems GitHub doesn't actually *show* the Author information anywhere that I can find.

Github automatically sets author of the squash commit based on who opened the PR.

—

Donald Stufft _______________________________________________ core-workflow mailing list core-workflow@python.org https://mail.python.org/mailman/listinfo/core-workflow This list is governed by the PSF Code of Conduct: https://www.python.org/psf/codeofconduct

Nick Coghlan

5:21 a.m.

On 25 February 2017 at 10:17, Brett Cannon wrote:

...

Two things. One, has someone verified that if a core dev edits a PR that the squash commit still gives the PR creator the author credit in the git metadata? I remember doing an edit like this once and GitHub didn't show any author credit in the web UI because I assume GitHub refused to guess who the author credit should go to. So if someone could test this in a checkout that would be great as that means https://github.com/ python/core-workflow/issues/7 can be easily solved and we can automate Misc/ACKS.

Two, we are going to review the new workflow in two weeks after having been using it for a month (I can't believe it's only been two weeks since the migration!). Since the sign-off requirement has generated the most discussion, what I will do is swap the requirement for PR merging to be no required review but to require all-green status checks (in a previous email Donald alluded to the fact that I thought self-approval would be possible in "emergencies" but that obviously doesn't hold). This will give us basically 2 weeks with both approaches when we review the process so we can make an informed decision.

Thanks, the new setup where Travis is required and codecov is advisory is looking pretty good to me so far (I just accepted a PR with bad codecov stats as the branches it was complaining lacked coverage will only trigger on Mac OS X). Cheers, Nick. -- Nick Coghlan | ncoghlan@gmail.com | Brisbane, Australia

Senthil Kumaran

17 Feb 17 Feb

11:45 p.m.

On Fri, Feb 17, 2017 at 2:39 PM, Barry Warsaw wrote:

...

But now I'm stuck and I'm impatient. ;)

No more. :) I spent time reading the bug, understanding the comments, reviewing the code, and then toggled my approval. The later action was a helpful thing for "me", in addition to being helpful to you to commit the code. If you had already committed, perhaps I might not have taken those steps until I had a need to. -- Senthil

Barry Warsaw

18 Feb 18 Feb

8:26 p.m.

On Feb 17, 2017, at 03:45 PM, Senthil Kumaran wrote:

...

On Fri, Feb 17, 2017 at 2:39 PM, Barry Warsaw wrote:

...
But now I'm stuck and I'm impatient. ;)

No more. :)

Thanks Senthil! And thanks Berker who also reviewed the branch.

...

I spent time reading the bug, understanding the comments, reviewing the code, and then toggled my approval.

The later action was a helpful thing for "me", in addition to being helpful to you to commit the code.

If you had already committed, perhaps I might not have taken those steps until I had a need to.

It's a fair point. Maybe we shouldn't change the setting just yet. (As I said, I was being impatient. :) Brett's argued for leaving the current workflow alone for a month to see how things shake out, and it wouldn't be unreasonable to do that here too. Maybe having things on GH will eventually improve the review bottleneck, but if we're too eager to turn off that requirement, we may not find out. Maybe we should bring back MvL's 5-for-1 offer. :) Cheers, -Barry

Senthil Kumaran

17 Feb 17 Feb

11:45 p.m.

On Fri, Feb 17, 2017 at 2:39 PM, Barry Warsaw wrote:

...

But now I'm stuck and I'm impatient. ;)

Yury Selivanov

2 Mar 2 Mar

11:31 p.m.

I only started to use the new workflow today. And I think that the new rules are too restrictive. Enforcing checking and reviews and approvals is a right thing, but what we have now feels like too much. I feel like a lot of bug fixes will have to be backported. With the current rules I have to open a new PR and ask someones approval *and* wait until Travis completes the build before I can cherry-pick and push. I like GH, git, and all other tools we now have. But current workflow needs to be loosen up a little IMHO. Yury On 2017-02-17 5:39 PM, Barry Warsaw wrote:

...

I submitted PR#138 on bpo-22807. I got a nice review from a community member and made a small change. All checks have passed.

But now I'm stuck and I'm impatient. ;)

The change is small enough and I'm happy with it, and I could patiently wait for another core dev to approve it, but in the likely case that no one else is interested in the bug, I'd like to be able to self-approve and accept it. This of course is described as my right in the devguide, along with the responsibility to watch the buildbots and revert the change if there are any problems with it.

But it seems like I cannot do that through the GH UI. Right now I'm seeing

(X) Review required (✓) All checks have passed (X) Merge is blocked

There seems to be no way to self-approve the PR. If I go to 'Files changed' tab and click on 'Review changes', both Approve and Request changes is dimmed. I can only comment on my own PR.

I can understand why we might want to prohibit self-approvals, but that's also a regression in the permissions and rights of core developers. I also think it's counterproductive since without that I might have to go begging for a review approval before the branch can land.

Is this intentional or an oversight?

(As an owner of the project, I took a quick look at the project settings and couldn't find a control for this, but I know other GH projects I contribute to support self-approval.)

Cheers, -Barry

_______________________________________________ core-workflow mailing list core-workflow@python.org https://mail.python.org/mailman/listinfo/core-workflow This list is governed by the PSF Code of Conduct: https://www.python.org/psf/codeofconduct

Donald Stufft

11:36 p.m.

You no longer need approval from someone else and you can open a cherry-pick PR prior to merging if you want. Sent from my iPhone

...

On Mar 2, 2017, at 6:31 PM, Yury Selivanov wrote:

I feel like a lot of bug fixes will have to be backported. With the current rules I have to open a new PR and ask someones approval *and* wait until Travis completes the build before I can cherry-pick and push.

Yury Selivanov

11:40 p.m.

On 2017-03-02 6:36 PM, Donald Stufft wrote:

...

You no longer need approval from someone else and you can open a cherry-pick PR prior to merging if you want.

OK, thanks for the clarification! Will try it later today. Thank you, Yury

Yury Selivanov

3 Mar 3 Mar

midnight

On 2017-03-02 6:36 PM, Donald Stufft wrote:

...

You no longer need approval from someone else and you can open a cherry-pick PR prior to merging if you want.

But I still can't push a cherry-picked commit without a PR for it? Yury

Donald Stufft

12:01 a.m.

Correct. Everything goes through a PR now. Ideally we will automate PRs for backports. Sent from my iPhone

...

On Mar 2, 2017, at 7:00 PM, Yury Selivanov wrote:

...
On 2017-03-02 6:36 PM, Donald Stufft wrote: You no longer need approval from someone else and you can open a cherry-pick PR prior to merging if you want.

But I still can't push a cherry-picked commit without a PR for it?

Yury

Brett Cannon

12:06 a.m.

The discussion of automating the creation of cherry-pick PRs is at https://github.com/python/core-workflow/issues/8. And the requirement of the PR is to force people to go through CI to verify the cherry-pick took cleanly (and the Travis config is structured to only run the test suite if the PR changes things outside of Doc/; same goes for building the docs and code coverage is never waited on). On Thu, 2 Mar 2017 at 16:02 Donald Stufft wrote:

...

Correct. Everything goes through a PR now. Ideally we will automate PRs for backports.

Sent from my iPhone

...
On Mar 2, 2017, at 7:00 PM, Yury Selivanov wrote:

...
On 2017-03-02 6:36 PM, Donald Stufft wrote: You no longer need approval from someone else and you can open a cherry-pick PR prior to merging if you want.

But I still can't push a cherry-picked commit without a PR for it?

Yury

_______________________________________________ core-workflow mailing list core-workflow@python.org https://mail.python.org/mailman/listinfo/core-workflow This list is governed by the PSF Code of Conduct: https://www.python.org/psf/codeofconduct

Yury Selivanov

12:21 a.m.

On 2017-03-02 7:06 PM, Brett Cannon wrote:

...

The discussion of automating the creation of cherry-pick PRs is at https://github.com/python/core-workflow/issues/8. And the requirement of the PR is to force people to go through CI to verify the cherry-pick took cleanly (and the Travis config is structured to only run the test suite if the PR changes things outside of Doc/; same goes for building the docs and code coverage is never waited on).

The issue here is that we now don't seem to trust core devs to check if cherry-picks are good and working. In an ideal world where online tooling spends as much time as running the test suite locally it would work flawlessly. In the reality, it simply takes a lot of time *waiting* and is quite distracting. Anyways, this is just my 2 cents. Maybe I just need more time to get accustomed to the new process. Thanks, Yury

Yury Selivanov

12:07 a.m.

On 2017-03-02 7:01 PM, Donald Stufft wrote:

...

Correct. Everything goes through a PR now. Ideally we will automate PRs for backports.

Got it. Well, I guess my only complaint about this is that Travis is extremely slow. I've been waiting a couple of hours for my PR to pass the check (and it's not yet done). When Travis is green, I'll push commits to master, create new PRs for cherry-picks and will have to wait another 1-2-N hours until I can push them. With old workflow we trusted core devs to run full test suite locally, and while buildbots were failing sometimes it wasn't a very frequent case. Yury

Donald Stufft

12:11 a.m.

I'm on my phone but unless Travis is backed up it sounds like something got lost somewhere. Multi hour waits is not typical. Sent from my iPhone

...

On Mar 2, 2017, at 7:07 PM, Yury Selivanov wrote:

Well, I guess my only complaint about this is that Travis is extremely slow. I've been waiting a couple of hours for my PR to pass the check (and it's not yet done).

Yury Selivanov

12:13 a.m.

On 2017-03-02 7:11 PM, Donald Stufft wrote:

...

I'm on my phone but unless Travis is backed up it sounds like something got lost somewhere. Multi hour waits is not typical.

Travis is overloaded a little bit at the moment. It looks like it takes it 20-30 minutes to fully test one PR. And it looks like it tests PRs one by one, so if you have many PRs in the queue it takes it a while to go through all of them. Yury

Brett Cannon

12:51 a.m.

On Thu, 2 Mar 2017 at 16:16 Yury Selivanov wrote:

...

On 2017-03-02 7:11 PM, Donald Stufft wrote:

...
I'm on my phone but unless Travis is backed up it sounds like something got lost somewhere. Multi hour waits is not typical.

Travis is overloaded a little bit at the moment. It looks like it takes it 20-30 minutes to fully test one PR. And it looks like it tests PRs one by one, so if you have many PRs in the queue it takes it a while to go through all of them.

Yeah, something is going on with Travis as it should typically only take 30 minutes. It might be due to backup they have from the S3 outage yesterday.

Tres Seaver

10:53 p.m.

-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 On 03/02/2017 07:51 PM, Brett Cannon wrote:

...

On Thu, 2 Mar 2017 at 16:16 Yury Selivanov wrote:

...
On 2017-03-02 7:11 PM, Donald Stufft wrote:

...
I'm on my phone but unless Travis is backed up it sounds like something got lost somewhere. Multi hour waits is not typical.

Travis is overloaded a little bit at the moment. It looks like it takes it 20-30 minutes to fully test one PR. And it looks like it tests PRs one by one, so if you have many PRs in the queue it takes it a while to go through all of them.

Yeah, something is going on with Travis as it should typically only take 30 minutes. It might be due to backup they have from the S3 outage yesterday.

Travis also throttles organizations with lots of builds. Tres. - -- =================================================================== Tres Seaver +1 540-429-0999 tseaver@palladion.com Palladion Software "Excellence by Design" http://palladion.com -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJYufPvAAoJEPKpaDSJE9HYSJAP/0tkNd2CiA2zECc91v5D2+7w 32ZgLOPmq4uBQRbkHVUTpi9hll5BzGX4CsQCoy03V2MJz/YU9styHN8s9NC0ugfq BcQn5xGVCr2ZploAeCfaSNe1pG6PjnnFmege9vc2Q2cgrft65SsIHKs3Vngws56X KHupE4/cNLApLVBrYO/O8rbP7rFhm35sGzpaRl2MUY86DuBp0PK6p8wgNkIY3mkl u3LQiCwILuPxzVtWgbEZUO3GtHpbI0AM+DOdQ/82qCVGRWVmN3woopJsdVKMKtvH /VldxKARpVjQcaVKJ3ERayNOlrQS2AssoGQ5PV9QmJxYy1ZZL9ni6fwfBpFKR3RY Zi2jbGloyy0fZmmDHWtV4E9ThULgVOExXbp4sWxqSx9g4ZmH0aKxewVLMrzV+GCT dbRyHSDuYC0tEKWzvp6v8E5/M30623iN9GARN3+lc9WmQ8MN0iPvM99vOs1hj5wD 894/UTMw/l/YV4bfcSsc8UpYxfw7q6qedXYYtRULy01Qw1X7aAbz/nPRj++mAOtP +RxkTgGFm0PmrBPLYVpT8vuJHMMH9mskBoBGa8pDMHSMCfVIg/aBPuXMQDc9Juso bj6mLY907y0xObSZ3tauzGDkTvig9jCfcKlqTjjjKRDLvil+9efZoZTvXDVOZpyU WbEITgoPjYSOGscxyWOp =GazO -----END PGP SIGNATURE-----

Donald Stufft

10:54 p.m.

...

On Mar 3, 2017, at 5:53 PM, Tres Seaver wrote:

Travis also throttles organizations with lots of builds.

I’m seeing if I can get our default limit increased FWIW. — Donald Stufft

Ryan Gonzalez

4 Mar 4 Mar

1:05 a.m.

I just checked the Travis CI status page, and it's showing a rather large backlog (~120-140 jobs) earlier today. Their site says: Our database provider has asked to make some changes to the existing primary logs DB that require we stop processing new jobs temporarily. They also mentioned that the S3 issue was causing them trouble, too, and they ALSO had a short API outage. I'm maybe it was just really bad luck that caused a bit of a backlog? AFAIK OSS projects are supposed to get up to 5 builds running concurrently. Maybe it would be worthwhile to attempt to parallel-ize the tests that don't depend on each other in any way? Also, this has nothing to do with this post, but when I saw this email, GMail showed the senders as "Barry .. Tres, Donald", and I thought it said "Trump, Donald". On Fri, Mar 3, 2017 at 4:54 PM, Donald Stufft wrote:

...

On Mar 3, 2017, at 5:53 PM, Tres Seaver wrote:

Travis also throttles organizations with lots of builds.

I’m seeing if I can get our default limit increased FWIW.

— Donald Stufft

_______________________________________________ core-workflow mailing list core-workflow@python.org https://mail.python.org/mailman/listinfo/core-workflow This list is governed by the PSF Code of Conduct: https://www.python.org/psf/codeofconduct

-- Ryan (ライアン) Yoko Shimomura > ryo (supercell/EGOIST) > Hiroyuki Sawano >> everyone else http://refi64.com/

Senthil Kumaran

2 Mar 2 Mar

11:40 p.m.

Hi Yuri, On Thu, Mar 2, 2017 at 3:31 PM, Yury Selivanov wrote:

...

I only started to use the new workflow today. And I think that the new rules are too restrictive. Enforcing checking and reviews and approvals is a right thing, but what we have now feels like too much.

You are actually experiencing an "improved" process since it was more more restrictive 7 days ago. So, you could actually describe what you felt was cumbersome with this "new" process and perhaps that could be tackled next. -- Senthil

Yury Selivanov

11:44 p.m.

On 2017-03-02 6:40 PM, Senthil Kumaran wrote:

...

Hi Yuri,

On Thu, Mar 2, 2017 at 3:31 PM, Yury Selivanov wrote:

...
I only started to use the new workflow today. And I think that the new rules are too restrictive. Enforcing checking and reviews and approvals is a right thing, but what we have now feels like too much. You are actually experiencing an "improved" process since it was more more restrictive 7 days ago. So, you could actually describe what you felt was cumbersome with this "new" process and perhaps that could be tackled next.

Thank you Senthil. Donald already clarified that my concerns about pushing cherry-picks have been already addressed. I'm going to be pushing some changes today, and will report if something comes up. Thanks, Yury

Donald Stufft

11:52 p.m.

One thing I forgot to mention is that cherry picking before a merge might result in more overall work if someone does review your original PR and it ends up causing changes since you'll need to pull those changes into each backport PR too. Sent from my iPhone

...

On Mar 2, 2017, at 6:44 PM, Yury Selivanov wrote:

Donald already clarified that my concerns about pushing cherry-picks have been already addressed.

2608

Age (days ago)

2623

Last active (days ago)

List overview

Download

42 comments

10 participants

participants (10)

Barry Warsaw
Brett Cannon
Donald Stufft
Ethan Furman
Maciej Szulik
Nick Coghlan
Ryan Gonzalez
Senthil Kumaran
Tres Seaver
Yury Selivanov

self-approving pull requests

tags

participants (10)