fcp merge: allow all known users to start team-specific FCPs#352
Conversation
| if all_team_members.iter().find(|&u| u == &author).is_none() { | ||
| // FIXME: post an error message in the issue. | ||
| info!("poll requester ({}) is not a known user", author.login); | ||
| return Ok(()); |
There was a problem hiding this comment.
Is it enough to just return Err(DashError(CommentableError(...))) here to get a proper error message?
There was a problem hiding this comment.
I don't think so -- not entirely sure, but my guess is that the right thing is to leave a comment inline here via:
let comment = RfcBotComment::new(&issue, CommentType::Error { message });
comment.post(None)?;
But it's not entirely obvious to me how that gets de-duped when rfcbot re-polls comments for additional processing. Maybe that's handled above this function (I sort of hope so...).
I'm personally OK merging without it, IIUC we don't currently return a comment error saying we failed to run the fcp because the person didn't have permissions to do so?
There was a problem hiding this comment.
we don't currently return a comment error saying we failed to run the fcp because the person didn't have permissions to do so?
Yeah we don't, and that has led to a lot of confusion over the years because the bot regularly thinks people don't have permission when they really should. This is the underlying cause of #312.
But indeed we don't have to fix that in this PR.
|
Just testing... @rust-rfcbot fcp merge T-lang |
|
And presumably this would work... @rust-rfcbot fcp merge T-infra |
|
@rust-rfcbot fcp merge infra |
|
Ah, I think I need the labels on the issue still? I'll avoid spamming further but in any case from the commands above it seems like it's at least not a regression to not give output as comments; the in-logs output from those is all "command author (Mark-Simulacrum) doesn't appear in any relevant subteams" -- probably because there are no relevant subteams right now (no labels on this PR). |
Mark-Simulacrum
left a comment
Mark-Simulacrum
left a comment
There was a problem hiding this comment.
I'll go ahead and approve this. I'll merge it when I have more time to debug any issues that come up (probably this weekend).
|
Seems to work: rust-lang/rust#156999 (comment) |
We should probably adjust that text.^^ |
2 participants
Helps with #312 for the case of
fcp merge team. The issue remains unfixed for a plainfcp merge.This PR allows everyone who is on any team to start FCP for any team with the "named team" syntax. IMO this is fine, we trust the people we have on teams so it's not worth checking whether the poster is on one of the teams included in the FCP. Note that already before this PR, anyone who is on a team that is mentioned in a label is allowed to start FCP for any team.
Note that I only build-tested this, I don't have a setup to actually run the bot.