Remove "needs backport to 3.6" label


(Ned Deily) #21

@Mariatta, Yes, I am aware of that :slight_smile: However, in this case, the merger did not have any special privs, AFAICT.


#22

PR 11477 https://github.com/python/cpython/pull/11477 was merged by @Senthil, who appears to have Admin access to python/cpython.

51%20AM


(Ned Deily) #23

@Mariatta, oh, thanks! I was looking in the wrong place.


(Brett Cannon) #24

Admin access probably needs to be cleaned up as it’s currently broader than RMs and those of us maintaining a webhook. Once 3.4 hits EOL in March I plan to talk it over with Ernest about a reasonable criteria for who gets admin access for security – and now branch access – reasons (and my guess it will simply tighten to those maintaining an active webhook or RMs :smile:).


(Victor Stinner) #25

Does GitHub support different permissions for different group of people?


(Brett Cannon) #26

There’s read, write, and admin. They can be set at the individual or GitHub team level (e.g. Python Core has write access while Release Managers has admin access).


(Senthil) #27

I do. When we migrated from hg to git, I needed that access, it was left like that. I may not need it any longer, but I can ask again if I need it or we leave at status quo too, and I can use for helping others when required.

  • PR11477 merging was a mistake, which has been corrected now by revert.

    • The version in bpo was having 3.6 set, and I didn’t realize we have 3.6 in security only fix mode.
    • I didn’t know that if the bot was not auto merging on purpose and thought, it needed manual intervention.

I hope the discussion on this topic: Removal of “needs backport to 3.6” can be separated from the above incident can be continued.


(Senthil) #28

I read this discussion completely.

I’d go with Ned (RM for 3.6) having the authority on if we should remove “Needs backport to 3.6” label on not.

  • Just we do not remove the selection of 3.6 in bugs.python.org it can be argued that we do not remove that label in github.com/python/cpython project too. (Side note: the issues themselves needs to be pruned for correct selection now that 3.6 is in security fix only mode).

  • Now that the previously set labels are removed from PRs. Keeping the label itself seems harmless to me, and the label will help us if we really want to backport security fixes automatically PRs automatically. (Imagine doing the backport via the computer in your pocket standing in the trains, labels,github, ui etc are helpful here).

  • There were 2 mistakes so far, and it can be corrected as we realize 3.6 is security fixes only. And current automation provided by miss-islington bot is actually very good IMO.


(Nathaniel J. Smith) #29

Would it be helpful for the bot to somehow signal the issue, e.g. posting a comment saying “only release managers can backport to this branch”?


(Brett Cannon) #30

I think fixing the permissions issue is the easier solution. That doesn’t require custom code and for security purposes we should do it anyway.


(Ernest W. Durbin III) #31

In following up here, new policies have been implemented that limit GitHub Organization Owners and python/cpython Repository Admins to help mitigate these kinds of things in the future.

You can view the initial policies here: https://github.com/python/devguide/pull/448.

Moving to least privilege model is a first step, we should further assess how to handle more granular situations as necessary.


(Donald Stufft) #32

This is a bit delayed, but I think fixing permissions is something that should be done regardless. Principle of least authority and all that.

However, I think we should also have one of the bots active on backport branches and should have a status check that ensure the RM of that branch has signed off on the PR. This would be similar to the check for a news file, except it would check to see if the RM for that branch has approved the PR (or something similar).

Effectively, reducing the people who have owner on the permission narrows down the list of people who can possibly make a mistake, but it doesn’t prevent those mistakes from happening. Adding controls so that the RM has to approve PRs for security only branches does prevent them (or well, makes it so it’s the RMs fault in that case :wink: ).


#33

I think we can add that in. If there is no rush in wanting this feature, I can see to it worked by a newcomer during Python US sprint. So essentially, we’ll add a “approved by RM” status check to the protected branches?


(Donald Stufft) #34

Yea, I think an Approved By RM status check to protected branches would be ideal. I also don’t think there is any rush, the number of people who can merge now is small and if someone does accidentally do it, a revert can solve it in the interim.


(Ned Deily) #35

With the changes that @EWDurbin has announced, I don’t think any further action is needed. It should now be the case that only the RM for a security-only branch can merge things (or an admin) to the branch and, even so, I think there is little chance that something would slip by. A bot would be overkill at this point.