Mailman 3 February 2017 - core-workflow

self-approving pull requests
by Barry Warsaw 03 Mar '17

03 Mar '17

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

10 42

Travis is now required to be passing, reviews are not
by Brett Cannon 24 Feb '17

24 Feb '17

I didn't turn on Codecov requirements for the patch because we seem to still have variance in them where some module that are inconsistently being tested (there is a test for the whole project but I left that off as well). I think we should definitely work towards fixing the coverage variance as I would like to require no drop in code coverage at some point, but as of right now it's wonky enough to be too much of a showstopper to flip on. I have opened https://github.com/python/core-workflow/issues/38 to track this and any modules whose coverage is inconsistent so we can eventually fix this and require coverage doesn't decrease.

1 0

git notifications
by Ethan Furman 23 Feb '17

23 Feb '17

One thing that seems to be lacking: names of commenters. In the recent thread about [Provide guidance on editing PRs prior to merge (#129)] there are several entries, from at least two people, and I have no idea who they are or exactly which one said what because their names are not showing up anywhere that I can see. Granted I could go and look on GitHub directly, but this feels like a regression from our previous work flow. -- ~Ethan~

2 5

Can core developers bypass PR checks?
by Steve Dower 21 Feb '17

21 Feb '17

(I'm not subscribed to this list, so please keep me on CC) Was there any discussion about allowing core developers to bypass any of the PR checks? Or was there an assumption that we'd just push directly to the main repo? For example, most of my contributions are to do with the Windows installer. Due to the nature of installers, virtually nobody else is confident to review anything complex, or even simple ones (such as https://github.com/python/cpython/pull/160) due to not being able to predict the flow on effects. As a result, I make many small changes that would take a long time to get reviewed. Similarly, many of my changes are not touched by the automated builds. Particularly anything to do with the installer is certainly not going to be built on a Linux box, and most (probably all) Windows buildbots don't build the installer right now. I've seen some systems where tags can be applied to a PR to skip build validation - I would certainly find that valuable. But essentially, I believe it should be within the power of core developers to bypass the checkin requirements (review + builds) and force a merge. Currently this is not possible - could it be enabled? Obviously it's placing trust in our core dev team, but that's the way it has always been, and we don't lose any traceability from this (unlike allowing people to delete branches from the repo, which can permanently lose code). Cheers, Steve

5 9

codecov.io on PRs feedback
by Barry Warsaw 16 Feb '17

16 Feb '17

Hey cool, I've just submitted my first PR against GH, and it's *so* nice to be able to use git, I might have to do this more often <wink>. https://github.com/python/cpython/pull/138 But I have questions about the coverage reports. In bbdef4c, coverage failed because my diff wasn't 100% covered, and the module itself was missing some coverage in code I didn't touch. If you click on the red X next to that commit, you can see the codecov/patch report 94.73% of diff hit. It's expected that some lines may be missed, due to underlying platform support, so the natural thing would be to say so in the code. I.e. suppress some misses so coverage doesn't false positive. This leads to questions: * How is codecov.io actually performing coverage? It seems difficult to find out exactly based on their documentation, and python/cpython's .codecov.yml doesn't reveal any details. Over in #python-dev, nedbat guessed it would be coverage.py (big surprise! :), and that would both make sense and be great, since it's a well-known and well-loved tool. * On that guess, I figured I'd add a few #pragmas for lines which are okay not to be covered. I pushed the new commit and didn't see a codecov.io failure so I thought that did the trick, but I'm actually not so sure. If you look at the above PR, you can see that commit bbdef4c has a red X, clicking on which gives you details that codecov/patch failed but travis-ci/pr succeeded. However, if you hover over the green check next to ccefb3a, all you see is that the travis-ci/pr check succeeded. There's no indication that coverage actually ran at all on ccefb3a. That doesn't make sense to me. * How can you run the same coverage tests locally? It's hard to know, given the apparent dearth of details from codecov.io. The top-level Makefile does have a `coverage` target, but that's a different thing. It would be nice if you didn't have to push a new commit just to see if you've fixed your coverage issues. There needs to be a local way to run the same tests. As I'm writing this, suddenly ccefb3a gained another check! So it seems like codecov.io is quite delayed in reporting its coverage, and the PR results can be misleading in the meantime, because it only reports that there is 1 check until codecov.io completes. I would prefer to see a yellow dot (i.e. the test is still running). Cheers, -Barry

2 2

The migration has started
by Brett Cannon 12 Feb '17

12 Feb '17

We are hanging out in #python-dev on Freenode if you care to swing by while we work on things. I also created https://trello.com/b/S5WbJfjP/github-migration to track things that I would like to see happen today.

2 3

Migration is done!
by Brett Cannon 11 Feb '17

11 Feb '17

A massive thank you to everyone who helped with this. It has been a long road but we finally reached the end of it! As for what's next, I think getting Misc/NEWS straightened out and then getting cherry-picking PRs automated are the next important items. As always https://github.com/python/core-workflow/issues has the current ideas on how to improve our workflow.

3 2

Final chance to express opinion on history rewrite for issue #s
by Brett Cannon 11 Feb '17

11 Feb '17

+1: Nick, Senthil, Chris +0: ... -0: Martin, Brett -1: Naoki, Berker (Maciej was positive but didn't say +1 or +0; Martin said -0.5 which isn't a valid vote, so I rounded up for him; I'm personally on the fence so voting conservatively now but can switch that view) If you have an opinion please express them now so Senthil has a chance to test this today before we do the official move tomorrow. Otherwise Senthil and I will make the final decision ourselves.

15 37

Choosing a prefix/label for issue numbers
by Brett Cannon 09 Feb '17

09 Feb '17

Historically commit messages for CPython have had the form of "Issue #NNNN: did something". The problem is that Github automatically links "#NNNN" to GitHub issues (which includes pull requests). To prevent incorrect linking we need to change how we reference issue numbers. The current candidates are: issue NNNN (notice the lack of #) bug NNNN bpo NNNN ("bpo" stands for "bugs.python.org") Whatever choice we go with it will be how we reference issues in PR titles and comments to link the PR to the issue, and in commit messages to send a message to the issue about the commit. To start this off, I'm -1 on "issue" (because people will out of habit add the #), +0 on "bug" (it's different but not everything is a bug), and +1 on "bpo" (as it namespaces our issues).

16 65

Draft of letter announcing migration
by Brett Cannon 08 Feb '17

08 Feb '17

I have written up what I will say to python-committers once the migration is complete: https://paper.dropbox.com/doc/CPython-workflow-changes-mx1k8G6M0rg5JLy80F1r6 . Feel free to leave comments if you think there is something I missed (which can include thanking you; this has taken so long I'm sure I have forgotten someone who has done work towards making this happen).

2 2