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

Add the option to not require a PR for protected branches #1359

Merged
merged 1 commit into from
Sep 27, 2024

Conversation

Kobzol
Copy link
Contributor

@Kobzol Kobzol commented Feb 28, 2024

This PR adds a pr-required = <true/false> option to branch protections. There are some repositories that use branch protections, but that do not require a PR.

I also refactored the branch protection mode to make the data representation clearer/make it impossible to represent invalid states (such as PR not required, but a required approval count is set). This is a compatibility break for the v1 data format, but branch protections are only read by sync-team anyway, so it shouldn't be a big deal.

An interesting thing is that it does not actually make sense to require any CI status checks if a PR is not required, because we do not enable anyone to bypass the branch protections. Because if you set a CI check, and try to push to the branch without a PR, it will be rejected.

Without a required PR, the branch protection still disallows force pushes and deletion, and allows us to set push actor allowances.

Relevant repos:

@Kobzol Kobzol force-pushed the branch-protection-mode branch from 132af34 to 0c68f45 Compare February 28, 2024 21:05
docs/toml-schema.md Show resolved Hide resolved
docs/toml-schema.md Show resolved Hide resolved
@Kobzol
Copy link
Contributor Author

Kobzol commented Mar 19, 2024

The sync-team side of this change is here.

@Kobzol Kobzol requested a review from rylev March 26, 2024 19:24
@@ -43,11 +47,15 @@
"branch_protections": [
{
"pattern": "master",
"ci_checks": [
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change breaks anything (namely sync-team) that relies on the current schema?

Don't think we can do this (at least not immediately).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think that there's anything else than sync-team that read the branch protections, and the sync-team change is ready. I already did some breaking changes recently that broke sync-team, it doesn't seem like a big deal.

Although, it would be bad if this broke other tools that just reuse the schema, even if they don't read the branch protections at all. Not sure if there are any of these though. Is there even any other tool apart from sync-team that reads the repos endpoint?

@MarcoIeni
Copy link
Member

Looks good to me. I'm not concerned about the breaking change because we control sync-team so we can redeploy it after changing the format 👍

@Kobzol
Copy link
Contributor Author

Kobzol commented Sep 24, 2024

@Turbo87 @jackh726 @rylev Apart from the breaking change to sync-team (which we expect and which will be resolved soon after merging this), do you have any other concerns about this change?

@Turbo87
Copy link
Member

Turbo87 commented Sep 24, 2024

Without a required PR, the branch protection still disallows force pushes and deletion, and allows us to set push actor allowances.

FWIW we need force pushes for the index repository too, since we regularly squash the history to keep the download size reasonable, but I guess that is unrelated to this PR?

@Kobzol
Copy link
Contributor Author

Kobzol commented Sep 24, 2024

Yes, the index repo will need some additional special treatment I think, that is tangential (although it will also need the ability to avoid PRs on branches, I suppose).

@jdno
Copy link
Member

jdno commented Sep 25, 2024

I'm not concerned about the breaking change because we control sync-team so we can redeploy it after changing the format

Theoretically, this is a public API and we're not necessarily the only consumers. But a quick code search doesn't show any uses of the Repo struct outside of sync-team. And it's definitely not worth the effort to bump the API version for this...

@Turbo87
Copy link
Member

Turbo87 commented Sep 25, 2024

But a quick code search doesn't show any uses of the Repo struct outside of sync-team.

btw not everyone is necessarily using this as a git dependency... (https://github.com/rust-lang/crates.io/blob/e922d6806cfcb90a4eb8838ec3a29029b65fe9e0/src/team_repo.rs#L63 😉)

@Kobzol
Copy link
Contributor Author

Kobzol commented Sep 25, 2024

I was specifically looking for usage of the repos endpoint and the required_approvals attribute before, which didn't seem to show anything.

@Kobzol Kobzol force-pushed the branch-protection-mode branch from 81651f2 to 1eb0177 Compare September 26, 2024 07:22
@MarcoIeni MarcoIeni merged commit 5646ad1 into rust-lang:master Sep 27, 2024
1 check passed
@Kobzol Kobzol deleted the branch-protection-mode branch September 27, 2024 08:18
@MarcoIeni
Copy link
Member

merged as asked by Kobzol

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants