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

rfcbot should actually trigger FCPs, update comment text #86

Closed
anp opened this issue Oct 7, 2016 · 8 comments
Closed

rfcbot should actually trigger FCPs, update comment text #86

anp opened this issue Oct 7, 2016 · 8 comments

Comments

@anp
Copy link
Member

anp commented Oct 7, 2016

@aturon, #61 (comment):

Quick question/request: when the bot posts comments like rust-lang/rfcs#1682 (comment), AFAICT it's "going into FCP" in its own head (in that, according to the docs, it'll post a follow-up comment one week later). One problem is, it's not actually flagging the RFC as final-comment-period, and the comment it leaves is a little unclear.

After discussion with @nikomatsakis, we feel comfortable with the bot fully triggering FCP, given our experience so far. That would mean:

  • Tagging the issue as final-comment-period
  • Posting a very clear comment saying: "This RFC is now entering its final comment period, as per the review above", with a link back to the original checklist.

A couple other details:

The bot should post another comment, "The final comment period is now complete." after one "business week" has passed since going into FCP. Actually merging the RFC requires human intervention, since we want to review intervening comments.
...


Yes, it's "going into FCP" in its head. I didn't want the text to be too authoritative while we're trying it out, but I can definitely update that.

Tagging as final-comment-period is a little tricky, because I believe there are some permissions that need to be set up on any applicable repos. I can write the code such that it will fail gracefully if the tagging isn't permitted (allowing it to still work smoothly on non-permitted repos)?

How is a business week defined here? I'm just doing 7 days right now, which means that excepting three-day weekends there should always be 5 business days between FCP start and end.

@anp anp added the rfcbot label Oct 7, 2016
@aturon
Copy link
Member

aturon commented Oct 7, 2016

So the "business week" thing was mostly thinking about "This Week in Rust", which is where a lot of people find out about FCP. That usually goes out on Monday, sometimes Tuesday.

I think what I'm asking for is that FCP last until the nearest Sunday that is at least one week away... but that's a bit complex.

Probably a simpler approach would just be to say FCP lasts for 10 days, or something like that.

@anp
Copy link
Member Author

anp commented Oct 7, 2016

Gotcha. 10 days is totally doable -- and makes a lot more sense once I have time to generalize the dashboard. That should be a quick change to make.

@aturon
Copy link
Member

aturon commented Oct 7, 2016

Regarding tagging/permissions: I think this is worthwhile. We've had cases where we've forgotten to tag an RFC and thus delayed action by more than a week. Automating as much as we can is great.

If the tagging fails, it'd be great if the bot's comment can say something like "I was unable to tag final-comment-period; please do so". That'll also alert us to the repos where we need to set up these permissions.

@anp
Copy link
Member Author

anp commented Oct 7, 2016

Yes, failing gracefully-but-not-silently sounds great.

@aturon
Copy link
Member

aturon commented Oct 22, 2016

Applying the final-comment-period label is a high priority item.

@aturon aturon added the P-high label Oct 22, 2016
@anp anp added this to the Adam goes incognito milestone Oct 22, 2016
anp added a commit that referenced this issue Oct 25, 2016
@anp
Copy link
Member Author

anp commented Oct 25, 2016

@aturon this is pretty much ready to try out, but I haven't yet finished setting up the optional & variable-length FCPs. Which means that PRs which shouldn't have an FCP will still have the label applied. Should I wait to deploy this change until the command structure is built out?

@aturon
Copy link
Member

aturon commented Oct 25, 2016

Yay! I think we should go forward with this change -- better to get extra
final-comment-period labels than not enough, IMO.

Thanks!

On Mon, Oct 24, 2016 at 11:29 PM, Adam Perry notifications@github.com
wrote:

@aturon https://github.com/aturon this is pretty much ready to try out,
but I haven't yet finished setting up the optional & variable-length FCPs.
Which means that PRs which shouldn't have an FCP will still have the label
applied. Should I wait to deploy this change until the command structure is
built out?


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#86 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AArUr7dJFUJC9WS1tb20Guo41hC5jZJkks5q3aHSgaJpZM4KQsAF
.

@anp
Copy link
Member Author

anp commented Oct 26, 2016

This has been deployed, I'll keep an eye out for any breakage.

@anp anp closed this as completed Oct 26, 2016
anoadragon453 pushed a commit to matrix-org/mscbot that referenced this issue Jul 6, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants