Skip to content

use rulesets by default#2397

Merged
marcoieni merged 1 commit intomainfrom
use-rulesets-by-default
Apr 9, 2026
Merged

use rulesets by default#2397
marcoieni merged 1 commit intomainfrom
use-rulesets-by-default

Conversation

@marcoieni
Copy link
Copy Markdown
Member

@marcoieni marcoieni commented Apr 9, 2026 •
edited
Loading

Switch from an allow approach to a deny approach.

@marcoieni marcoieni mentioned this pull request Apr 9, 2026
Open
marcoieni
marcoieni commented Apr 9, 2026
View reviewed changes
src/sync/github/mod.rs
fn should_use_rulesets(&self, repo: &rust_team_data::v1::Repo) -> bool {
let repo_full_name = format!("{}/{}", repo.org, repo.name);
self.config.enable_rulesets_repos.contains(&repo_full_name)
!self.config.disable_rulesets_repos.contains(&repo_full_name)
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.

here is where the logic changes

@marcoieni marcoieni force-pushed the use-rulesets-by-default branch from 6e2a394 to 1c14446 Compare April 9, 2026 11:00
marcoieni
marcoieni commented Apr 9, 2026
View reviewed changes
src/sync/github/mod.rs Outdated
settings_diff: (RepoSettings, RepoSettings),
permission_diffs: Vec<RepoPermissionAssignmentDiff>,
branch_protection_diffs: Vec<BranchProtectionDiff>,
branch_protection_diffs: BTreeSet<BranchProtectionDiff>,
Copy link
Copy Markdown
Member Author

@marcoieni marcoieni Apr 9, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

I had to change to a BTreeSet because the tests was flaky. Instead, a fixed order fixed it.

This change requires the additional derives you see in the PR

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.

This is fishy, Vec is ordered too. Using sorting based on some custom comparison, rather than on insertion order, seems like it hides a different bug. Maybe some of the input collections iterated in diff_branch_protections do not have a stable order?

Copy link
Copy Markdown
Member Author

@marcoieni marcoieni Apr 9, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

I think I addressed this comment, let me know

EDIT: I saw you upvoted my other change 👍

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.

Yes, thank you!

@marcoieni marcoieni force-pushed the use-rulesets-by-default branch 3 times, most recently from e61410a to d5c139f Compare April 9, 2026 11:05
@github-actions
Copy link
Copy Markdown

github-actions bot commented Apr 9, 2026

Dry-run check results 

[WARN  rust_team::sync] sync-team is running in dry mode, no changes will be applied.
[INFO  rust_team::sync] synchronizing crates-io
[INFO  rust_team::sync] synchronizing github

@marcoieni marcoieni force-pushed the use-rulesets-by-default branch from d5c139f to 3a00c5e Compare April 9, 2026 11:11
marcoieni
marcoieni commented Apr 9, 2026
View reviewed changes
src/sync/github/api/read.rs
org: &str,
repo: &str,
) -> anyhow::Result<HashMap<String, (String, BranchProtection)>>;
) -> anyhow::Result<BTreeMap<String, (String, BranchProtection)>>;
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.

changed this so that we order the branch protections when we read them from GitHub

@marcoieni marcoieni force-pushed the use-rulesets-by-default branch 3 times, most recently from 8bf5956 to b9c2e85 Compare April 9, 2026 11:44
@marcoieni marcoieni force-pushed the use-rulesets-by-default branch from 7d79b94 to 4913044 Compare April 9, 2026 11:47
@marcoieni marcoieni marked this pull request as ready for review April 9, 2026 11:55
ubiratansoares
ubiratansoares approved these changes Apr 9, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@ubiratansoares ubiratansoares left a comment

Choose a reason for hiding this comment

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

🚀🚀🚀

@marcoieni marcoieni added this pull request to the merge queue Apr 9, 2026
Merged via the queue into main with commit 3d23df5 Apr 9, 2026
3 checks passed
@rustbot rustbot mentioned this pull request Apr 9, 2026
Open
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ubiratansoares ubiratansoares ubiratansoares approved these changes

@Mark-Simulacrum Mark-Simulacrum Awaiting requested review from Mark-Simulacrum Mark-Simulacrum is a code owner

@emilyalbini emilyalbini Awaiting requested review from emilyalbini emilyalbini is a code owner

@jdno jdno Awaiting requested review from jdno jdno is a code owner

+1 more reviewer

@Kobzol Kobzol Kobzol left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@marcoieni @ubiratansoares @Kobzol