Skip to content

Add minimal triagebot config #956

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.

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

Merged
merged 1 commit into from
Jun 14, 2025
Merged

Conversation

Urgau
Copy link
Member

@Urgau Urgau commented Jun 14, 2025

This PR adds a minimal triagebot.toml config to make contributions to this repository respect upstream rust-lang/rust conventions and avoid issues when syncing this subtree.

If you're interested in more triagebot feature, I highly recommand looking at miri's triagebot.toml config.

Accompanying team PR: rust-lang/team#1876

cf. rust-lang/rust#142489 (comment)
cc @tgross35

@tgross35
Copy link
Contributor

Adding it LGTM, but I believe this needs check-commits = false since we pretty often need to link to r-l/rust issues

@Urgau
Copy link
Member Author

Urgau commented Jun 14, 2025

Then they should be put in the PR description and not in the commit description, we don't want to spam the issues with references to the commit. It also creates issues with subtrees, especially if someone puts Fixes #123 in the commit description. It's explained in the warning message.

@tgross35
Copy link
Contributor

tgross35 commented Jun 14, 2025

I don't really follow that logic - if links are in the PR description on a repo that uses merge commits (r-l/r), then the links wind up in those merge commits and still make noise from forks on issues. In fact, that's actually worse because of how Josh does syncs - it (horribly) needs to recreate every totally unrelated merge commit for every merge from the same rollup, e.g. the last sync #946 wound up mentioned in all these totally unrelated issues:

And then in a repo that doesn't use merge commits (this one), the links are just lost and you have no useful crossrefs in the log/blame.

It would make sense to disallow Fixes:/Closes:/etc links that can accidentally close issues in other repos, but not allowing any links in history seems pretty strong.

@Urgau
Copy link
Member Author

Urgau commented Jun 14, 2025

if links are in the PR description on a repo that uses merge commits (r-l/r), then the links wind up in those merge commits and still make noise from forks on issues

Unfortunately, we currently can't do much about them. We unfortunately can't change GitHub default merge commit description and the one bors generates is parsed by some tooling making it difficult to change.

And then in a repo that doesn't use merge commits (this one), the links are just lost and you have no useful crossrefs in the log/blame.

That's a good point. When there isn't a merge commit the PR is not cross-referenced in a commit.
There is still a reference on the UI tough.
image

It would make sense to disallow Fixes:/Closes:/etc links that can accidentally close issues in other repos, but not allowing any links in history seems pretty strong.

That's what we were doing at the start, but the same issue about spam and wrongly referenced issue exist without the Fixes/..., the solution for the wrongly referenced one is to use canonicalized issue links (ie prefixed with the org and repo name), but that won't do anything for the spam issue, furthermore by moving the issue links to the PR description triagebot can automatically update the issue links with canonicalized version, reducing a bit the contributor frustration.

but not allowing any links in history seems pretty strong.

Technically it's a warning, it's not blocking in anyway.


Anyway, added check-commits = false to [issue-links].

I will see how we can better accommodate repos without merge commits.

@Urgau Urgau force-pushed the add-triagebot-config branch from b03bdf2 to dafb383 Compare June 14, 2025 22:07
@tgross35
Copy link
Contributor

I wish github would just allow you to filter mentions by comment vs. commit and by organization so this wasn't a concern. One of the things I miss most from GitLab 🙃

Hopefully bors2 will help clean up the merge messages a bit. With any luck it will also be able to use octopus merges for rollups so the merge commits don't get so out of hand...

In any case, thank you for getting this set up! I don't think there is any harm in merging before the team PR merges.

@tgross35 tgross35 enabled auto-merge (squash) June 14, 2025 22:33
@tgross35 tgross35 force-pushed the add-triagebot-config branch from dafb383 to cbb5af5 Compare June 14, 2025 22:33
@tgross35 tgross35 merged commit 41b5e34 into rust-lang:master Jun 14, 2025
35 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants