Skip to content

Add a new users_on_vacation config which prevents PR assignment - #1712

Merged
ehuss merged 1 commit into
rust-lang:masterfrom
jyn514:vacation
Jul 28, 2023
Merged

ehuss merged 1 commit into
rust-lang:masterfrom
jyn514:vacation

Conversation

@jyn514

@jyn514 jyn514 commented Jul 26, 2023

Copy link
Copy Markdown
Member

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

jyn514 commented Jul 26, 2023

Copy link
Copy Markdown
Member Author

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
Copy Markdown
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

jyn514 commented Jul 27, 2023

Copy link
Copy Markdown
Member Author

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

apiraino commented Jul 27, 2023

Copy link
Copy Markdown
Contributor

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

jyn514 commented Jul 27, 2023

Copy link
Copy Markdown
Member Author

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.

@ehuss ehuss left a comment

Copy link
Copy Markdown
Contributor

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
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
Copy Markdown
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
Copy Markdown
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

rustbot commented Jul 30, 2023

Copy link
Copy Markdown
Collaborator

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