prepare for moving cargo from branch protections to rulesets#2333
prepare for moving cargo from branch protections to rulesets#2333
Conversation
Dry-run check results |
2758c92 to
3473afa
Compare
repos/rust-lang/cargo.toml
Outdated
| [[branch-protections]] | ||
| pattern = "master" | ||
| ci-checks = ["conclusion"] | ||
| merge-queue = true |
There was a problem hiding this comment.
merge queues in branch protections were enabled from the UI because the API only support merge queues in rulesets.
| pr-required = false | ||
| allowed-merge-apps = ["promote-release"] | ||
| prevent-update = true | ||
|
|
There was a problem hiding this comment.
this is the ruleset that already existed. I just imported the settings here.
rust_team_data/src/v1.rs
Outdated
| pub allowed_merge_apps: Vec<MergeBot>, | ||
| pub merge_queue: bool, | ||
| #[serde(default = "default_true")] | ||
| #[serde(default = "branch_protection_default_prevent_creation")] |
There was a problem hiding this comment.
the fact that prevent_creation "was true by default" was encoded in too many places in the codebase. So I created functions to be reused across the codebase.
Similar with the other fields.
There was a problem hiding this comment.
Wait, this is the v1 API, not GitHub API models, why do we need a default at all? Once triagebot has been updated, the field should always be present, right?
| default = "branch_protection_default_prevent_update", | ||
| skip_serializing_if = "is_false" | ||
| )] | ||
| pub prevent_update: bool, |
There was a problem hiding this comment.
I had to add this field because cargo needs it.
There was a problem hiding this comment.
I edited this file to encode the default of various fields, so that we know when to skip logging a rule
| pub struct BranchProtection { | ||
| pub pattern: String, | ||
| #[serde(default, skip_serializing_if = "is_branch_target")] | ||
| pub target: ProtectionTarget, |
There was a problem hiding this comment.
I had to add this field because the existing cargo ruleset targets a tag
f1ca0f1 to
7b72488
Compare
| r#"repo '{}' uses multiple branch protections with the pattern `{}`"#, | ||
| r#"repo '{}' uses multiple {:?} protections with the pattern `{}`"#, | ||
| repo.name, | ||
| protection.target, |
There was a problem hiding this comment.
A repo can have the same pattern for branch and tag
There was a problem hiding this comment.
Maybe we could create some BranchProtectionId to encode what is the unique identifier of the protection (target + pattern, maybe plus name?). But if it's only used here, it's fine.
e82a97d to
d8e7dba
Compare
src/schema.rs
Outdated
| #[serde(default)] | ||
| pub merge_queue: bool, | ||
| #[serde(default = "default_true")] | ||
| #[serde(default = "rust_team_data::v1::branch_protection_default_prevent_creation")] |
There was a problem hiding this comment.
The defaults should be chosen here, not in the v1 API.
There was a problem hiding this comment.
the issue is that the rust_team_data crate doesn't import this crate. So it can't use the default.
There was a problem hiding this comment.
oh, I saw your other comment.
| r#"repo '{}' uses multiple branch protections with the pattern `{}`"#, | ||
| r#"repo '{}' uses multiple {:?} protections with the pattern `{}`"#, | ||
| repo.name, | ||
| protection.target, |
There was a problem hiding this comment.
Maybe we could create some BranchProtectionId to encode what is the unique identifier of the protection (target + pattern, maybe plus name?). But if it's only used here, it's fine.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
4e2357c to
83c18a9
Compare
| RustTimer, | ||
| Bors, | ||
| WorkflowsCratesIo, | ||
| PromoteRelease, |
There was a problem hiding this comment.
We don't have deny unknown fields on the branch protection, so adding the new fields there should be fine for downstream. But this enum variant will brick triagebot, because it won't know it. So we will again have to first land just the infra change, then update triagebot, and then land the cargo repo config change itself.
There was a problem hiding this comment.
yes, this is what I wrote in the PR body, right?
This comment has been minimized.
This comment has been minimized.
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
b009059 to
a6ed73b
Compare
a6ed73b to
4a5dc0b
Compare
I ran a script to detect existing rulesets in our orgs.
rust-lang/cargois the only repository that has a ruleset not tracked in this repo.This PR:
"Only allow the release process to publish tags") to theteamrepository. You can verify that the dry run doesn't mention this repo, so we are just writing down without editing the ruleset in githubcargofrom classic branch protections to rulesetsIn the following, I post the screenshots of the resources this PR touches:
as a backup in case something goes wrong
to better review the dry run
For triagebot retrocompatibility we should first merge the code changes, update triagebot and then edit
cargo.toml. After this PR is approved, I will comment the changes tocargo.tomlbefore merging this.Existing branch protections
Masterrust-1.*(download to see the full picture)
Existing ruleset