Core developer workflow in the devguide

in the Core Developers / Responsibilities section, there’s this:

Should we specifically discourage core developers from committing to main or other branches directly (other than perhaps release managers during the run-up to their releases) and mandate always going through PRs? After all, core developers can accept their own PRs so it won’t hold anything up, but there will be potential opportunities for other committers to review PRs. Also, the CODEOWNERS seems only to be useful as a means of automatically inviting code owners to review a PR - direct commits to main would appear to bypass code owners’ opportunity to review completely. Allowing direct commits just seems to encourage an undesirable way of working. (Even if you can’t technically block direct commits at the Git level, proscribing them at the policy level will have some effect.)

3 Likes

PRs are a good idea, even if you merge it yourself, as the CI may catch something you missed.

And as you say, it gives others a chance to review.

Personally, I find useful the act of writing up and giving one last check before opening the PR, and sometimes spot something. A bit like rubber duck debugging.

1 Like

Should we specifically discourage core developers from committing to main or other branches directly (other than perhaps release managers during the run-up to their releases) and mandate always going through PRs?

This is already documented in Accepting pull requests (second bullet point). Maybe this section could be moved to or linked in the Responsibilities page.

I thought this was also enforced in the GitHub settings but it seems it isn’t (unless there’s a separate setting that prevents it):

Should we enable this branch protection rule?

Core developers should not be able to push directly to any of the cpython branches on GitHub (main, 3.11, etc) unless they are release managers or GitHub admins. If anyone can, it is a bug and please report it here! This should be enforced by another branch setting which is enabled for all of the branches:

Choose which status checks must pass before branches can be merged into a branch that matches this rule. When enabled, commits must first be pushed to another branch, then merged or pushed directly to a branch that matches this rule after status checks have passed.

P.S. And there is another branch protection rule that is enabled that even prevents release managers and admins from accidentally evading the above restrictions; to do so, they have to take special action.

3 Likes

IMO, yes. There isn’t much downside and lots of upside. Plus, doing so would codify best practices anyway. We can allow release manager overrides in dire situations, but my guess is that it would be only very rarely if ever invoked. @pablogsal - have you ever had to commit directly to main?

2 Likes

See above. This should already be the case.

1 Like

:partying_face:

Yes, normally to push the post release I commit to main to avoid having weird history around releases. That’s also why I block the branches, so o can always fast forward the commit.

3 Likes