Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Automatically set Branch protection rules #191

Open
icemac opened this issue Jan 27, 2023 · 12 comments
Open

Automatically set Branch protection rules #191

icemac opened this issue Jan 27, 2023 · 12 comments
Assignees
Labels
enhancement New feature or request

Comments

@icemac
Copy link
Member

icemac commented Jan 27, 2023

Currently I set the "required status checks to pass before merging" by hand:

  • docs (if applicable)
  • lint
  • coverage
  • py37 (as oldest supported python version)
  • py311 (as newest supported python version)
  • pypy3 (if applicable as slowest running test)

Repositories containing C code or testing against Windows or MacOS have other status check names.

The actual list of status check names depends on the configuration via meta/config so it should be set when running it.

Update:

@mgedmin
Copy link
Member

mgedmin commented Jan 27, 2023

Note that branch protection rules interfere with people using zest.releaser to make releases. The failure mode is pretty annoying: the git push is rejected after the release is already pushed to PyPI, and so you end up with a release that doesn't have the corresponding source available on GitHub, unless people notice and take steps to remedy the situation (e.g. by creating a pull request containing the release tag and the post-release version bump commit).

When this has happened in the past I would go into GH repository settings and turn off branch protection.

Do we have a better solution?

@mgedmin
Copy link
Member

mgedmin commented Jan 27, 2023

(I forgot to mention that people with admin rights to the GitHub ZopeFoundation org are exempt from branch protection rules and can use zest.releaser freely, without noticing that there is a problem for other maintainers.)

@dataflake
Copy link
Member

I don't use zest.releaser at all. I'd have a problem with turning off branch protection just because one specific tool can't deal with it.

@d-maurer
Copy link

I don't use zest.releaser at all. I'd have a problem with turning off branch protection just because one specific tool can't deal with it.

As @mgedmin has pointed out, the problem would go away if the people doing releases were admins.

If this would give them too many right, maybe "Personal Access Tokens" ("PAT") might help. Apparently, they can give fine grained rights - maybe there is a right "ignore branch protection". In this case, releasers potentially could get a PAT with this right to do their release work.

@mauritsvanrees
Copy link
Member

I don't use zest.releaser at all. I'd have a problem with turning off branch protection just because one specific tool can't deal with it.

This is not because of the use of zest.releaser. The problem is that a simple git push fails.

@icemac
Copy link
Member Author

icemac commented Jan 30, 2023

This is not because of the use of zest.releaser. The problem is that a simple git push fails.

This failing git push may be not recognised by the person cutting the release. Could there be an ability to create a branch for the release commits so they can be merged via a pull request?

The other question would be: Do we have too many people who are able to cut releases? (Some having PyPI rights have left Zope or even Python years ago. Some care just for a specific package where they should be able to cut releases.) From a security and quality standpoint I'd like to see successful pull request before changes in master. I see protected branches and required successful test runs as a valid tool to keep the quality.

Maybe we need a more clean policy regarding releases. (We could even create a release team and allow them to push to master via Restrict who can push to matching branches.)

What do you think?

@icemac
Copy link
Member Author

icemac commented Feb 2, 2023

Another thought: We are not enabling Require a pull request before merging for master branches so the scenario After-a-release-I-cannot-push-my-changes-to-master should not happen at all.

@dataflake
Copy link
Member

There's a setting "Allow specified actors to bypass required pull requests" that can be applied to people or teams. We could set up a new team specifically including the intersection of those who are not admins but who make releases, and then set that team as allowed to bypass PRs. I would bet that's a pretty small group.

@dataflake
Copy link
Member

I have created a new team "Release Managers" at https://github.com/orgs/zopefoundation/teams/release-managers/members. Feel free to add members there.

The group needs to be added to each affected repository in the repo's "Settings" | "Collaborators and Teams" as well before it can be selected for "Allow specified actors to bypass required pull requests" when editing a branch protection rule. Adding a team to a repo can also be done at https://github.com/orgs/zopefoundation/teams/release-managers/repositories by searching for and selecting a repository. There doesn't seem to be a way to just force-add a new team everywhere.

@icemac
Copy link
Member Author

icemac commented Mar 24, 2023

There doesn't seem to be a way to just force-add a new team everywhere.

I hope that this can be done via the API, so it could be included when running meta/config scripts.

@dataflake
Copy link
Member

There doesn't seem to be a way to just force-add a new team everywhere.

I hope that this can be done via the API, so it could be included when running meta/config scripts.

It can be done with the API, and I just did that. I added the "Release Manager" team to all non-archived repositories within the zopefoundation organization and gave it the maintain role. Here's the script I wrote: https://gist.github.com/dataflake/f33932efa8fa6423ff2907708664f936

The team doesn't necessarily make sense in all of those repositories, but it won't break anything and can be deleted easily.

@icemac icemac removed their assignment Jun 28, 2023
@icemac icemac self-assigned this Sep 21, 2023
@icemac
Copy link
Member Author

icemac commented Apr 26, 2024

Open issues once #207 is merged: Allow release managers to push to master without needing a PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Status: No status
Development

No branches or pull requests

5 participants