diff --git a/config.toml b/config.toml index 48b154bdf..4fccb5e1a 100644 --- a/config.toml +++ b/config.toml @@ -83,6 +83,4 @@ members-without-zulip-id = [ "rust-timer", ] -disable-rulesets-repos = [ - "rust-lang/rust", -] +disable-rulesets-repos = [] diff --git a/repos/rust-lang/rust.toml b/repos/rust-lang/rust.toml index ca9deb742..a5fcc89f1 100644 --- a/repos/rust-lang/rust.toml +++ b/repos/rust-lang/rust.toml @@ -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]] +name = "main" pattern = "main" allowed-merge-apps = ["bors"] +prevent-update = true +# 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. +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 = "*" +allowed-merge-apps = ["promote-release"] +prevent-update = true [[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"] diff --git a/src/sync/github/mod.rs b/src/sync/github/mod.rs index 5f4dc2bf1..13fdc2bda 100644 --- a/src/sync/github/mod.rs +++ b/src/sync/github/mod.rs @@ -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}; @@ -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 { + let bypass_actors = self.bypass_actors(expected_repo, branch_protection).await?; + + Ok(construct_ruleset(branch_protection, bypass_actors)) + } + + async fn bypass_actors( + &self, + expected_repo: &rust_team_data::v1::Repo, + branch_protection: &rust_team_data::v1::BranchProtection, + ) -> Result, 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, 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, @@ -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); } } @@ -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 { @@ -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::Ruleset { use api::*; - let branch_protection_mode = get_branch_protection_mode(branch_protection); - // 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 = BTreeSet::new(); @@ -1214,14 +1292,14 @@ 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, }, @@ -1229,7 +1307,7 @@ pub fn construct_ruleset(branch_protection: &rust_team_data::v1::BranchProtectio } // 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(); @@ -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 = 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(); - api::Ruleset { id: None, name: branch_protection diff --git a/src/sync/github/tests/mod.rs b/src/sync/github/tests/mod.rs index e3a787005..765421396 100644 --- a/src/sync/github/tests/mod.rs +++ b/src/sync/github/tests/mod.rs @@ -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(); @@ -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(); diff --git a/src/sync/github/tests/test_utils.rs b/src/sync/github/tests/test_utils.rs index feb9522cd..58e290a50 100644 --- a/src/sync/github/tests/test_utils.rs +++ b/src/sync/github/tests/test_utils.rs @@ -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); diff --git a/src/validate.rs b/src/validate.rs index 1dfd9e00e..91c5ee17d 100644 --- a/src/validate.rs +++ b/src/validate.rs @@ -1189,12 +1189,22 @@ fn validate_branch_protections(data: &Data, errors: &mut Vec) { 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, @@ -1205,8 +1215,7 @@ fn validate_branch_protections(data: &Data, errors: &mut Vec) { 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"#, repo.name, protection.pattern, team @@ -1214,8 +1223,7 @@ but that team does not seem to exist"#, } 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