Skip to content

use rulesets in the rust repo #2327

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.

Sign up for GitHub

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

Open
marcoieni wants to merge 1 commit into
base: main
Choose a base branch
from
Open

use rulesets in the rust repo #2327

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 1 addition & 3 deletions config.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,4 @@ members-without-zulip-id = [
"rust-timer",
]

disable-rulesets-repos = [
"rust-lang/rust",
]
disable-rulesets-repos = []
72 changes: 66 additions & 6 deletions repos/rust-lang/rust.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,67 +26,127 @@ rust-analyzer = "write"
style = "write"
types = "write"
triage = "write"
# needed for the branch protection
rust-timer = "write"

# Only `bors` can push to `main`
[[branch-protections]]
Copy link
Copy Markdown
Member

@Kobzol Kobzol Mar 25, 2026

Choose a reason for hiding this comment

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

I don't understand why we need two separate branch protections to prevent force pushes. Force pushes should be disabled for everyone, even for bors.

Copy link
Copy Markdown
Member Author

@marcoieni marcoieni Mar 27, 2026

Choose a reason for hiding this comment

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

Force pushes should be disabled for everyone, even for bors.

That's what the main - force-push rule does. It prevents everyone (including bors) to force push.
Bors is in the main rule, so that rule doesn't apply to bors. To restrict bors, we need another rule where bors isn't in the bypass list.

Copy link
Copy Markdown
Member

@Kobzol Kobzol Mar 27, 2026

Choose a reason for hiding this comment

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

Ah, I see, I forgot about the other branch protection. That's quite stupid that rulesets work like this, tbh 😆 It would be better if we could bypass only specific properties.. but I understand that would make the options more complicated.

It's quite non-obvious though, and it seems hard to ensure that the other branch protection don't enable something that we don't want, hmm.

Copy link
Copy Markdown
Member Author

@marcoieni marcoieni Mar 27, 2026

Choose a reason for hiding this comment

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

It's quite non-obvious though, and it seems hard to ensure that the other branch protection don't enable something that we don't want, hmm.

Yeah, it requires a different mindset than before a bit.

Copy link
Copy Markdown
Member

@Kobzol Kobzol Mar 28, 2026

Choose a reason for hiding this comment

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

Wait, but if the force push ruleset requires a PR by default, doesn't that mean that it will require a PR also for bors?

Copy link
Copy Markdown
Member Author

@marcoieni marcoieni Mar 29, 2026

Choose a reason for hiding this comment

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

yes, you are right, in my test I didn't have "requires a PR" in the settings.
Probably we should add a third rule related to main 😅

Copy link
Copy Markdown
Member

@Kobzol Kobzol Mar 29, 2026

Choose a reason for hiding this comment

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

Not a third rule, just disable requiring PRs here.

Copy link
Copy Markdown
Member Author

@marcoieni marcoieni Mar 29, 2026

Choose a reason for hiding this comment

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

With "here" you mean in the "main - force push"?
I think you are right!
Because "main" already prevent everyone from opening PRs.
I'll do it.

Copy link
Copy Markdown
Member Author

@marcoieni marcoieni Mar 29, 2026

Choose a reason for hiding this comment

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

I did the same for stable and beta as well.

name = "main"
pattern = "main"
allowed-merge-apps = ["bors"]
prevent-update = true
Copy link
Copy Markdown
Member

@Kobzol Kobzol Mar 30, 2026

Choose a reason for hiding this comment

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

I realized that to mirror the previous behavior of branch protections, having a non-empty list of allowed-merge-apps should probably imply prevent-update = true. But that's non-blocking, we can do that later.


# No one can force push to main
[[branch-protections]]
name = "main - force-push"
pattern = "main"
# `main` already requires users to open PRs.
# This allows bors to push without opening a PR.
pr-required = false

# Only `bors` and `promote-release` can push to `stable`
[[branch-protections]]
name = "stable"
pattern = "stable"
allowed-merge-apps = ["bors"]
allowed-merge-apps = ["bors", "promote-release"]
prevent-update = true

# Only `promote-release` can force-push to `stable`
[[branch-protections]]
name = "stable - force-push"
pattern = "stable"
allowed-merge-apps = ["promote-release"]
# `stable` already requires users to open PRs.
# This allows bors to push without opening a PR.
Copy link
Copy Markdown
Member

@Kobzol Kobzol Mar 30, 2026

Choose a reason for hiding this comment

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

Suggested change
# This allows bors to push without opening a PR.
# This allows promote-release to push without opening a PR.

Copy link
Copy Markdown
Member Author

@marcoieni marcoieni Mar 30, 2026

Choose a reason for hiding this comment

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

no, I meant bors!
The stable - force-push branch protection is there to restrict the behavior of bors.
promote-release doesn't have any restrictions because it is in all the allowed lists, so it could even delete the branch.

pr-required = false

# No one create or delete the stable branch.
[[branch-protections]]
name = "stable - misc"
pattern = "stable"
pr-required = false
prevent-force-push = false

# Only `bors` and `promote-release` can push to `beta`
[[branch-protections]]
name = "beta"
pattern = "beta"
allowed-merge-apps = ["bors"]
allowed-merge-apps = ["bors", "promote-release"]
prevent-update = true

# Only `promote-release` can force-push to `beta`
[[branch-protections]]
name = "beta - force-push"
pattern = "beta"
allowed-merge-apps = ["promote-release"]
# `beta` already requires users to open PRs.
# This allows bors to push without opening a PR.
pr-required = false

# no one can create or delete the beta branch.
[[branch-protections]]
name = "beta - misc"
pattern = "beta"
pr-required = false
prevent-force-push = false

[[branch-protections]]
pattern = "*"
Copy link
Copy Markdown
Member

@Kobzol Kobzol Apr 7, 2026

Choose a reason for hiding this comment

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

We have to check whether this still protects tags (like 1.94.1).

Copy link
Copy Markdown
Member Author

@marcoieni marcoieni Apr 9, 2026

Choose a reason for hiding this comment

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

it doesn't. Let's do a followup PR

allowed-merge-apps = ["promote-release"]
prevent-update = true
Copy link
Copy Markdown
Member Author

@marcoieni marcoieni Apr 9, 2026

Choose a reason for hiding this comment

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

the classic branch protection allows deletion, while this doesn't.
Jakub here says he prefers to prevent deletion for this and "*/**/*"

Copy link
Copy Markdown
Member

@Kobzol Kobzol Apr 9, 2026

Choose a reason for hiding this comment

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

Hmm, I didn't realize deletion was allowed before. I don't think it's super important, but I guess we can try to be more strict and see if preventing deletion breaks anything.


[[branch-protections]]
pattern = "*/**/*"
pr-required = false
prevent-update = true

[[branch-protections]]
pattern = "cargo_update"
pr-required = false
prevent-deletion = false
prevent-force-push = false

# Required for running try builds created by bors.
# Must support force-pushes.
[[branch-protections]]
pattern = "automation/bors/try"
allowed-merge-apps = ["bors"]
prevent-update = true

# Required for running try builds created by bors.
# Must support force-pushes.
[[branch-protections]]
pattern = "automation/bors/try-merge"
allowed-merge-apps = ["bors"]
prevent-update = true

# Required for running auto builds created by bors.
# Must support force-pushes.
[[branch-protections]]
pattern = "automation/bors/auto"
allowed-merge-apps = ["bors"]
prevent-update = true

# Required for running auto builds created by bors.
# Must support force-pushes.
[[branch-protections]]
pattern = "automation/bors/auto-merge"
allowed-merge-apps = ["bors"]
prevent-update = true

# Required for unrolled PR builds created by perfbot.
# Must support force-pushes.
[[branch-protections]]
pattern = "try-perf"
allowed-merge-apps = ["rust-timer"]
pr-required = false
allowed-merge-teams = ["rust-timer"]
prevent-update = true

# Required for unrolled PR builds created by perfbot.
# Must support force-pushes.
[[branch-protections]]
pattern = "perf-tmp"
allowed-merge-apps = ["rust-timer"]
pr-required = false
allowed-merge-teams = ["rust-timer"]
prevent-update = true

[environments.bors]
branches = ["automation/bors/auto", "automation/bors/try", "try-perf"]
107 changes: 86 additions & 21 deletions src/sync/github/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ use crate::sync::Config;
use crate::sync::github::api::{
GithubRead, Login, PushAllowanceActor, RepoPermission, RepoSettings, Ruleset,
};
use anyhow::Context as _;
use futures_util::StreamExt;
use log::debug;
use rust_team_data::v1::{Bot, BranchProtectionMode, MergeBot, ProtectionTarget};
Expand Down Expand Up @@ -447,6 +448,78 @@ impl SyncGitHub {
!self.config.disable_rulesets_repos.contains(&repo_full_name)
}

async fn construct_ruleset(
&self,
expected_repo: &rust_team_data::v1::Repo,
branch_protection: &rust_team_data::v1::BranchProtection,
) -> anyhow::Result<api::Ruleset> {
let bypass_actors = self.bypass_actors(expected_repo, branch_protection).await?;

Ok(construct_ruleset(branch_protection, bypass_actors))
}
Copy link
Copy Markdown
Member Author

@marcoieni marcoieni Mar 24, 2026

Choose a reason for hiding this comment

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

this function is only two lines but I left it like this to minimize the git diff of the PR.
We could inline the construct_ruleset free function later


async fn bypass_actors(
&self,
expected_repo: &rust_team_data::v1::Repo,
branch_protection: &rust_team_data::v1::BranchProtection,
) -> Result<Vec<api::RulesetBypassActor>, anyhow::Error> {
use api::{RulesetActorType, RulesetBypassActor, RulesetBypassMode};

let mut bypass_actors = Vec::new();
let allowed_teams = self
.allowed_merge_teams(expected_repo, branch_protection)
.await?;
bypass_actors.extend(allowed_teams);
let allowed_apps = branch_protection
.allowed_merge_apps
.iter()
.filter_map(|app| {
app.app_id().map(|app_id| RulesetBypassActor {
actor_id: app_id,
actor_type: RulesetActorType::Integration,
bypass_mode: RulesetBypassMode::Always,
})
});
bypass_actors.extend(allowed_apps);
Ok(bypass_actors)
}

async fn allowed_merge_teams(
&self,
expected_repo: &rust_team_data::v1::Repo,
branch_protection: &rust_team_data::v1::BranchProtection,
) -> Result<Vec<api::RulesetBypassActor>, anyhow::Error> {
use api::{RulesetActorType, RulesetBypassActor, RulesetBypassMode};

let mut allowed = vec![];

for team_name in &branch_protection.allowed_merge_teams {
let github_team = self
.github
.team(&expected_repo.org, team_name)
.await?
.with_context(|| {
format!(
"failed to find GitHub team '{team_name}' in org '{}' for repo '{}/{}'",
expected_repo.org, expected_repo.org, expected_repo.name
)
})?;
let team_id = github_team.id.with_context(|| {
format!(
"GitHub team '{team_name}' in org '{}' is missing an ID",
expected_repo.org
)
})?;

allowed.push(RulesetBypassActor {
actor_id: team_id as i64,
actor_type: RulesetActorType::Team,
bypass_mode: RulesetBypassMode::Always,
});
}
Ok(allowed)
}

async fn diff_repo(
&self,
expected_repo: &rust_team_data::v1::Repo,
Expand Down Expand Up @@ -481,7 +554,9 @@ impl SyncGitHub {
let use_rulesets = self.should_use_rulesets(expected_repo);
if use_rulesets {
for branch_protection in &expected_repo.branch_protections {
let ruleset = construct_ruleset(branch_protection);
let ruleset = self
.construct_ruleset(expected_repo, branch_protection)
.await?;
rulesets.push(ruleset);
}
}
Expand Down Expand Up @@ -799,7 +874,9 @@ impl SyncGitHub {

// Process each branch protection as a potential ruleset
for branch_protection in &expected_repo.branch_protections {
let expected_ruleset = construct_ruleset(branch_protection);
let expected_ruleset = self
.construct_ruleset(expected_repo, branch_protection)
.await?;

if let Some(actual_ruleset) = rulesets_by_name.remove(&expected_ruleset.name) {
let Ruleset {
Expand Down Expand Up @@ -1179,11 +1256,12 @@ fn github_int(value: u32) -> i32 {
i32::try_from(value).unwrap_or_else(|_| panic!("Value {value} exceeds GitHub's Int range"))
}

pub fn construct_ruleset(branch_protection: &rust_team_data::v1::BranchProtection) -> api::Ruleset {
pub fn construct_ruleset(
branch_protection: &rust_team_data::v1::BranchProtection,
bypass_actors: Vec<api::RulesetBypassActor>,
) -> api::Ruleset {
use api::*;

let branch_protection_mode = get_branch_protection_mode(branch_protection);
Copy link
Copy Markdown
Member Author

@marcoieni marcoieni Apr 7, 2026

Choose a reason for hiding this comment

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

As noted by Kobzol:

we shouldn't turn off requiring PRs when bors is enabled, because that will turn off required PRs for everyone


// Use a BTreeSet to ensure a consistent order. This avoids unnecessary diffs when the order of rules changes,
// since GitHub does not guarantee any specific order for rules.
let mut rules: BTreeSet<RulesetRule> = BTreeSet::new();
Expand Down Expand Up @@ -1214,22 +1292,22 @@ pub fn construct_ruleset(branch_protection: &rust_team_data::v1::BranchProtectio
// Add pull request rule if PRs are required
if let BranchProtectionMode::PrRequired {
required_approvals, ..
} = &branch_protection_mode
} = branch_protection.mode
{
rules.insert(RulesetRule::PullRequest {
parameters: PullRequestParameters {
dismiss_stale_reviews_on_push: branch_protection.dismiss_stale_review,
require_code_owner_review: REQUIRE_CODE_OWNER_REVIEW_DEFAULT,
require_last_push_approval: REQUIRE_LAST_PUSH_APPROVAL_DEFAULT,
required_approving_review_count: github_int(*required_approvals),
required_approving_review_count: github_int(required_approvals),
required_review_thread_resolution: branch_protection
.require_conversation_resolution,
},
});
}

// Add required status checks if any
if let BranchProtectionMode::PrRequired { ci_checks, .. } = &branch_protection_mode
if let BranchProtectionMode::PrRequired { ci_checks, .. } = &branch_protection.mode
&& !ci_checks.is_empty()
{
let mut checks = ci_checks.clone();
Expand Down Expand Up @@ -1271,19 +1349,6 @@ pub fn construct_ruleset(branch_protection: &rust_team_data::v1::BranchProtectio
rules.insert(RulesetRule::MergeQueue { parameters });
}

// Build bypass actors from allowed merge apps
let bypass_actors: Vec<RulesetBypassActor> = branch_protection
.allowed_merge_apps
.iter()
.filter_map(|app| {
app.app_id().map(|app_id| RulesetBypassActor {
actor_id: app_id,
actor_type: RulesetActorType::Integration,
bypass_mode: RulesetBypassMode::Always,
})
})
.collect();
Copy link
Copy Markdown
Member Author

@marcoieni marcoieni Mar 24, 2026

Choose a reason for hiding this comment

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

this piece of code was moved up


api::Ruleset {
id: None,
name: branch_protection
Expand Down
10 changes: 6 additions & 4 deletions src/sync/github/tests/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1121,7 +1121,7 @@ fn ruleset_creation_logs_non_default_disabled_flags() {
protection.prevent_deletion = false;
protection.prevent_force_push = false;

let ruleset = construct_ruleset(&protection);
let ruleset = construct_ruleset(&protection, vec![]);
let mut rendered = String::new();
log_ruleset(&ruleset, None, &mut rendered).unwrap();

Expand All @@ -1133,15 +1133,17 @@ fn ruleset_creation_logs_non_default_disabled_flags() {

#[test]
fn ruleset_updates_log_disabled_toggle_rules_as_false() {
let old =
construct_ruleset(&BranchProtectionBuilder::pr_required("main", &["test"], 1).build());
let old = construct_ruleset(
&BranchProtectionBuilder::pr_required("main", &["test"], 1).build(),
vec![],
);

let mut new_protection = BranchProtectionBuilder::pr_required("main", &["test"], 1).build();

// Change default
new_protection.prevent_force_push = false;

let new = construct_ruleset(&new_protection);
let new = construct_ruleset(&new_protection, vec![]);
let mut rendered = String::new();
log_ruleset(&old, Some(&new), &mut rendered).unwrap();

Expand Down
2 changes: 1 addition & 1 deletion src/sync/github/tests/test_utils.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -182,7 +182,7 @@ impl DataModel {
.enumerate()
.map(|(idx, protection)| Ruleset {
id: Some(idx as i64),
..construct_ruleset(protection)
..construct_ruleset(protection, vec![])
})
.collect();
org.rulesets.insert(repo.name.clone(), protections);
Expand Down
22 changes: 15 additions & 7 deletions src/validate.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1189,12 +1189,22 @@ fn validate_branch_protections(data: &Data, errors: &mut Vec<String>) {

wrapper(data.repos(), errors, |repo, _| {
let bors_configured = repo.bots.iter().any(|b| matches!(b, Bot::Bors));
let mut patterns = HashSet::new();
let mut pattern_counts = HashMap::new();

for protection in &repo.branch_protections {
if !patterns.insert((protection.target, &protection.pattern)) {
*pattern_counts
.entry((protection.target, protection.pattern.as_str()))
.or_insert(0usize) += 1;
}

for protection in &repo.branch_protections {
let key = (protection.target, protection.pattern.as_str());
let occurrences = pattern_counts
.get(&key)
.with_context(|| format!("pattern_counts should contain the key {key:?}"))?;
if *occurrences > 1 && protection.name.is_none() {
bail!(
r#"repo '{}' uses multiple {:?} protections with the pattern `{}`"#,
r#"repo '{}' uses multiple {:?} protections with the pattern `{}`; when multiple protections share the same target and pattern, each protection must specify `name`"#,
repo.name,
protection.target,
protection.pattern,
Expand All @@ -1205,17 +1215,15 @@ fn validate_branch_protections(data: &Data, errors: &mut Vec<String>) {
let key = (repo.org.clone(), team.clone());
if !github_teams.contains(&key) {
bail!(
r#"repo '{}' uses a branch protection for {} that mentions the '{}' github team;
but that team does not seem to exist"#,
r#"repo '{}' uses a branch protection for {} that mentions the '{}' github team; but that team does not seem to exist"#,
Copy link
Copy Markdown
Member Author

@marcoieni marcoieni Mar 24, 2026

Choose a reason for hiding this comment

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

I put this error and the next one on the same line to improve how they look in github actions.

repo.name,
protection.pattern,
team
);
}
if !repo.access.teams.contains_key(team) {
bail!(
r#"repo '{}' uses a branch protection for {} that has an allowed merge team '{}',
but that team is not mentioned in [access.teams]"#,
r#"repo '{}' uses a branch protection for {} that has an allowed merge team '{}', but that team is not mentioned in [access.teams]"#,
repo.name,
protection.pattern,
team
Expand Down