Skip to content
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

Add a new users_on_vacation config which prevents PR assignment #1712

Merged
merged 1 commit into from
Jul 28, 2023

Conversation

jyn514
Copy link
Member

@jyn514 jyn514 commented Jul 26, 2023

People keep assigning me to PRs (usually new contributors who haven't seen my Zulip message). I very much do not want this. Prevent it in triagebot.

  • If r? jyn514 is posted as a comment after the PR is opened, post a useful error but otherwise do nothing
  • If r? jyn514 is posted in the PR body, fall back to assigning from the diff (as if r? wasn't present)
  • If r? bootstrap is posted anywhere, filter me out from the team. I am not actually on the review rotation currently, but this would be useful if I were (and maybe other people will find it useful?).

People keep assigning me to PRs (usually new contributors who haven't
seen my Zulip message). I very much do not want this. Prevent it in
triagebot.

- If `r? jyn514` is posted after a comment after the PR is opened, post a
  useful error but otherwise do nothing
- If `r? jyn514` is posted in the PR body, fall back to assigning from
  the diff (as if `r?` wasn't present)
- If `r? bootstrap` is posted anywhere, filter me out from the team. I
  am not actually on the review rotation currently, but this would be
  useful if I were (and maybe other people will find it useful?).
@jyn514
Copy link
Member Author

jyn514 commented Jul 26, 2023

also i have github notifications off, so if you have feedback get Pietro to dm me on discord or something, i probably won't see it here

@apiraino
Copy link
Contributor

Worth mentioning that there is an ongoing effort (lead by me) to solve this and other issues related to PR assignment. Here's my work in progress and a document detailing the reasoning and the global picture behind the idea.

@jyn514
Copy link
Member Author

jyn514 commented Jul 27, 2023

thanks. the two PRs are complimentary i think - your PR tries to do automatic load balancing of auto assigning PRs based on capacity, mine is about preventing manual assignment so that it is impossible for me to get assigned to PRs. right now the only way to prevent manual assignment is to leave all rust teams altogether so i am not part of the rust-lang org.

@apiraino
Copy link
Contributor

apiraino commented Jul 27, 2023

mine is about preventing manual assignment so that it is impossible for me to get assigned to PRs

that is true. By design I decided (after a first round of feedbacks) to allow the manual assignment (r? user) to override the time off settings of a contributor. But now I am thinking whether to rethink this and maybe have an additional ...

  • I am really REALLY outta here

... kind of flag.

I think in future it's better to have one single place to manage these preferences and these .toml files don't allow the granularity I am looking for.

EDIT: What I am trying to say is that personally I see this as a quick fix to solve your problem to be perhaps reverted when the new machinery rolls out.

@jyn514
Copy link
Member Author

jyn514 commented Jul 27, 2023

EDIT: What I am trying to say is that personally I see this as a quick fix to solve your problem to be perhaps reverted when the new machinery rolls out.

i'm fine with that yes, i just don't want to wait until your giant change merges.

Copy link
Contributor

@ehuss ehuss left a comment

Choose a reason for hiding this comment

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

Thanks! I'll go ahead and merge, with the general understanding that this may be a temporary solution that we may replace in the future. I haven't tested this, but it looks like it should be ok.

Another option to consider in the future is to read the "Busy" status of the user from GitHub. GitHub's own assignment algorithm uses that to skip people, and offers a nice lightweight way for someone to temporarily take themselves out of rotation.

@ehuss ehuss merged commit 82073e7 into rust-lang:master Jul 28, 2023
2 checks passed
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jul 28, 2023
Prevent people from assigning me as a PR reviewer

depends on rust-lang/triagebot#1712
@workingjubilee
Copy link
Member

that is true. By design I decided (after a first round of feedbacks) to allow the manual assignment (r? user) to override the time off settings of a contributor. But now I am thinking whether to rethink this and maybe have an additional ...

It is ultimately a matter of consent. In theory, we can say the r+ bit comes with the understanding that you can be assigned to do a PR, even on holidays, even if your dog died that day, even in the event of a natural disaster, et cetera, and thus not give it out without consent to that.

In practice, we may find it contributes more to confusion if a PR can be assigned to someone who, say, took a moment to set a flag on GitHub or via a .toml or whatever, because they are extremely conscientious and also know they are about to spend at least the next month or so dealing with a suddenly-ignited war or revolution in their home country. It may discourage people from being so conscientious if they know the impact would be minimal anyways and not prevent confusion.

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jul 29, 2023
Prevent people from assigning me as a PR reviewer

depends on rust-lang/triagebot#1712
@apiraino
Copy link
Contributor

In practice, we may find [that overriding a user's setting] contributes more to confusion if a PR can be assigned to someone who [...] took a moment to set a flag on GitHub or via a .toml or whatever [...]

Yeah, if someone has set themself as unavailable, it would be a saner assumption to ignore possible scenarios to override this explicit will (as explained on the HackMD doc, my reasoning was that a "r? user" implies a prev. conversation between the PR author and user somewhere else).

Thanks @workingjubilee for your thoughts!

@rustbot
Copy link
Collaborator

rustbot commented Jul 30, 2023

Error: Parsing assign command in comment failed: ...' user' | error: specify user to assign to at >| '" implies '...

Please file an issue on GitHub at triagebot if there's a problem with this bot, or reach out on #t-infra on Zulip.

@jyn514 jyn514 deleted the vacation branch December 19, 2023 05:32
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.

5 participants