Conversation
Dry-run check results |
11472fe to
ccd8f31
Compare
|
Backup (taken automatically with https://github.com/marcoieni/multicheese in case you are curious!)
|
a4540e6 to
83f09a2
Compare
| protection.pattern, | ||
| ); | ||
| } | ||
|
|
There was a problem hiding this comment.
Unlike protection rules, multiple rulesets can apply at the same time, so you can be confident that every rule targeting a branch in your repository will be evaluated when someone interacts with that branch
I removed this because we need multiple rules to interact with the same branch (different bypass list for normal push and force push).
There was a problem hiding this comment.
In that case we should make the name required when there are multiple identical patterns, because we used the pattern as an identifier.
6f35480 to
c3d0cbd
Compare
|
Here's how I tested the "stable" and "stable - force-pushes" rules.
The user in the |
6e4c63c to
c67ec1b
Compare
|
There's one issue: |
0dee3f1 to
f614447
Compare
|
We are relatively close to making the unrolled PRs by bors, instead of rustc-perf. If this can wait a few weeks, rust-timer will no longer need that access. |
f614447 to
dbe30bf
Compare
#2343 is a small change that can be reverted easily. Since this is the only blocker to have rulesets in the rust I'm in favor of creating the team. What do you think? 🤔 |
|
Sure, go ahead, it's a small change 👍 |
1b14c6d to
c62adff
Compare
| let bypass_actors = self.bypass_actors(expected_repo, branch_protection).await?; | ||
|
|
||
| Ok(construct_ruleset(branch_protection, bypass_actors)) | ||
| } |
There was a problem hiding this comment.
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
| bypass_mode: RulesetBypassMode::Always, | ||
| }) | ||
| }) | ||
| .collect(); |
There was a problem hiding this comment.
this piece of code was moved up
c62adff to
8c81777
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
eafb1f6 to
2146114
Compare
ubiratansoares
left a comment
There was a problem hiding this comment.
I've checked all screenshots and compared side-by-side with the new config. Looks Ok to me. Perhaps @Kobzol wants to confirm whether we are good to go from his side as well.
I think it'd be nice spreading the word about this PR on T-infra/annoucements.
| name = "main" | ||
| pattern = "main" | ||
| allowed-merge-apps = ["bors"] | ||
| prevent-update = true |
There was a problem hiding this comment.
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.
| pattern = "stable" | ||
| allowed-merge-apps = ["promote-release"] | ||
| # `stable` already requires users to open PRs. | ||
| # This allows bors to push without opening a PR. |
There was a problem hiding this comment.
| # This allows bors to push without opening a PR. | |
| # This allows promote-release to push without opening a PR. |
There was a problem hiding this comment.
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.
2146114 to
90ef019
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
b563c85 to
f1b0898
Compare
This comment has been minimized.
This comment has been minimized.
21063a4 to
d9ec7fa
Compare
| ) -> api::Ruleset { | ||
| use api::*; | ||
|
|
||
| let branch_protection_mode = get_branch_protection_mode(branch_protection); |
There was a problem hiding this comment.
As noted by Kobzol:
we shouldn't turn off requiring PRs when bors is enabled, because that will turn off required PRs for everyone
repos/rust-lang/rust.toml
Outdated
| pattern = "try-perf" | ||
| allowed-merge-apps = ["rust-timer"] | ||
| allowed-merge-teams = ["rust-timer"] | ||
| pr-required = false |
There was a problem hiding this comment.
| pr-required = false |
Shouldn't be needed, as rust-timer bypasses this ruleset anyway (same below).
| prevent-force-push = false | ||
|
|
||
| [[branch-protections]] | ||
| pattern = "*" |
There was a problem hiding this comment.
We have to check whether this still protects tags (like 1.94.1).
There was a problem hiding this comment.
it doesn't. Let's do a followup PR
95f7537 to
af4704b
Compare
This comment has been minimized.
This comment has been minimized.
| [[branch-protections]] | ||
| pattern = "*" | ||
| allowed-merge-apps = ["promote-release"] | ||
| prevent-update = true |
There was a problem hiding this comment.
the classic branch protection allows deletion, while this doesn't.
Jakub here says he prefers to prevent deletion for this and "*/**/*"
There was a problem hiding this comment.
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.
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. |
a1caee4 to
37806ed
Compare
37806ed to
ab71fb6
Compare














Is there a preferred moment when we want to merge this?