Closed ehuss closed 1 year ago
Thanks for the review!
It would be great to get the documentation you've written for the PR description somewhere in the repo itself.
I plan to update the wiki documentation after this merges. Here is a draft of what I plan to post: https://gist.github.com/ehuss/5745d8b8d979a68c167f3b700fbb4be5
I'm concerned that moving the configuration from the highfive repo to rust-lang/rust will make changes more painful. Is the team repo another possible place?
This was addressed in the original proposal at https://hackmd.io/@ehuss/rJ_MLj-Ji. Overall I think it will be easier to configure it in one place (triagebot.toml
) instead of multiple places. Or, placing codeowners in the teams repo also seems like the wrong way to approach it (since the file layout could change over time). I also think it will be easier for different repos to be able to update and adjust these on their own instead of relying on someone to merge something in the teams repo which can take a long time. Dealing with the ad-hoc groups would also be complicated.
I understand this makes some things a little more complicated, such as removing a user from a team. However, I don't expect that to be too much of a problem (and I'll update the forge docs as well).
I'm unsure if it's wise to allow r? to anyone. I suppose we can see if this ever becomes a problem in practice, but it's relatively simple to prevent I wonder, why not?
This was also addressed in the proposal document. The original logic was somewhat awkward (using "collaborators" which on rust-lang/rust is a odd assortment of people). I'm not too worried about abuse, and it is easy to undo any abuse that occurs. It can also be added later if it seems necessary.
I think the comment editing "bug" is indeed a bug and should be fixed. Comment editing is very common (especially for checking boxes), so I imagine this will get encountered fairly often.
OK. I changed it to ignore edits. It is a somewhat tricky issue, because I have observed users trying to edit their r?
comments, particularly when they don't work. highfive would ignore something like r? rust-lang/foobar
and then the user would try to edit it to try to fix it, and get frustrated when nothing happens. It could in theory respond to edits on the last comment (or the last comment with an r?
), but that seems like it is more complicated than it is worth.
I think r? on closed PRs should be ignored.
OK, I added a check for that. I don't like to ignore commands, as it can be confusing when a bot does not respond, so I had it post a comment.
Modulo the two implementation comments I'm personally very excited to see this land, thanks for putting it together.
This adds the functionality from highfive into triagebot. The motivation is to hopefully have something more maintainable, and to reduce the overhead of managing another service.
Overview of what this does:
new_pr
setting to the[autolabel]
config. This makes triagebot automatically add that label to new PRs. This takes over thenew_pr_labels
setting from highfive (usually used forS-waiting-on-review
).r?
commands in PR comments to set assignment. This is only enabled when the[assign.owners]
config is set to support a gradual transition away from highfive (so the two bots don't interfere with one another, see "Migration" below).Adds configuration to the
[assign]
table for handling auto-assignment of PRs:See https://hackmd.io/@ehuss/rJ_MLj-Ji for the original proposal which includes some discussion of alternatives considered.
Migration
To migrate a repo, the process is roughly:
triagebot.toml
configuration.triagebot.toml
file is merged, merge the highfive change (or just remove the highfive webhook from the repo) to avoid conflicting behavior.Differences
For various reasons, there are several differences in this implementation versus highfive:
owners
paths now use gitignore style patterns instead ofstarts_with
.r?
now allows referring to Rust teams (liker? rust-lang/cargo
)r?
to the PR author, the current assignee, or a repo collaborator. It now has no restriction on who can postr?
.all
group which would be included as candidates to assign all PRs. In practice this was used whendirs
was empty. This can instead be expressed with something like:fallback
group, which can be used to select a reviewer if none of theowners
paths matched any modified files. This replaces thecore
team fallback.r? group
so that it doesn't allow assigning the same reviewer again (it always picks someone new).r? whatever/group
. triagebot now requires it to berust-lang
(to match a team) or whatever org the PR is opened on (to match a group).:
or-
afterr?
as inr?:@user
orr?-@user
. I have not seen these used in practice, and don't seem particularly helpful to me.@user
orcompiler
(must start with@
or alphanumeric or dash). Now it allows anything to followr?
such asr? !@$#!
, and will give an error if it can't figure out what was meant.@
inr? @rust-lang/@user
r? @
with nothing following the@
is now an error (previously was ignored)r?
at the end of the message is now an error (previously was ignored)r? user
is now allowed to refer to the given user. Previously it required the@
symbol. This seems like a relatively arbitrary requirement. This would require some effort to restrict since triagebot now supports rust teams, and I want to allow referring to a team without the@
to avoid subscribing a whole team.r? @user$
previously matcheduser
and now matchesuser$
. The current token boundaries seem like they should handle all expected cases (.
,,
,?
,!
, etc.)compiler/rustc_target/src/spec
. This can be replaced with[mentions]
.r?
user).Now responds toNo longer allowsr?
on closed PRs (highfive ignores these). Should these be ignored or generate an error?r?
on closed PRs in https://github.com/rust-lang/triagebot/pull/1656/commits/210178d5118bc80f941d81fc2efbab95a9825900.