Human friendly incantations + Auto Closing/Postponing RFCs + More labels #197

Centril · 2018-04-20T05:45:20Z

This PR aims to fix:

permit "rfcbot resolve foo" as an alias #193 by adding aliases and fault tolerant incantations (and a whole lot more in that direction)
Another label for when final-comment-period ends #102 (finished-final-comment-period + labels for disposition)

In addition, the PR improves the FCP-completed comment and also auto-closes FCP-completed RFCs with a disposition to close / postponed and applies those labels when doing so.

I haven't had time to test this on any repository, so please double check this logic =)

I also haven't added the new labels to the rfcs or rust repos. I'll do that before / if we merge this PR.
In total, these labels are added:

finished-final-comment-period
disposition-close
disposition-merge
disposition-postpone
closed

The postponed label already exists for the rfcs repo.

…re labels.

Centril · 2018-04-20T05:57:02Z

Hmm... The error arises from failing to compile diesel_cli v1.2.0.

Centril · 2018-04-20T06:36:13Z

Update: I updated the lockfile & bumped the nightly version to 2018-04-19; this seems to have fixed things.

aturon · 2018-04-20T15:58:01Z

These changes look fantastic to me!

I only have one concern: right now, once an RFC is in FCP, adding a concern doesn't take it back out of FCP. That'd be nice to improve in general, and it becomes even more important if the bot is going to automatically close RFCs.

anp

Thank you so much for this! Looking really great. I have some comments and a few requests. There are a couple of things I'd need to check to be sure on, but I'm short on time and will try to circle back soon. Really excited to see this land!

anp · 2018-04-20T18:10:46Z

src/github/nag.rs

@@ -143,6 +158,26 @@ fn update_proposal_review_status(proposal_id: i32) -> DashResult<()> {
    Ok(())
 }

+fn remove_issue_label(issue: &Issue, label: &str) {


I think these might be a bit clearer if they were methods on Issue, rather than free fns

anp · 2018-04-20T18:12:40Z

src/github/nag.rs

+    GH.add_label(&issue.repository, issue.number, label)
+}
+
+fn close_pull_request(issue: &Issue) {


This fn signature makes me sad that PRs are represented as Issues internally. At a minimum can you make a TODO or open an issue to make this type-safe and only apply to PRs but not issues? If GH.close_pr will correctly work on an issue even though it hits the pulls endpoint, can we call all of this close_issue and continue assuming issues and PRs are roughly the same?

It is good that you flagged this; I did some tests with curl on a dummy repo, https://github.com/Centril/test-gh-api, and closed both a PR and an issue with the issues endpoint. I also noticed a bug (s/status/state).

anp · 2018-04-20T18:14:11Z

src/github/nag.rs

+                  .first::<GitHubUser>(conn) {
+            Ok(i) => i,
+            Err(why) => {
+                error!("Unable to retrieve proposal initiator for proposal id {}: {:?}",


I need to double check the schema, but I'm pretty sure the database ensures this never happens and so this can probably be safely written as an expect.

For now, to be on the safe side I'll keep the match. I'll change later if you are sure. :)

anp · 2018-04-20T18:16:15Z

src/github/nag.rs

+        if is_rfc_repo(&issue) {
+            match disp {
+                FcpDisposition::Merge => {
+                    // TODO: This one will require a lot of work to


Can you start an issue for this? I'm not even sure what the "right" strategy is going to be and it will require some discussion.

Done :) #198

anp · 2018-04-20T18:17:15Z

src/github/nag.rs

+
+        // Execute FFCP actions:
+        if is_rfc_repo(&issue) {
+            match disp {


Can you make this match statement into a method on FcpDisposition?

I extracted the logic starting from the comment into a free function execute_ffcp_actions to reduce rightward drift. It didn't really feel like belonging to FcpDisposition given that it depends on an event :)

anp · 2018-04-20T18:18:51Z

src/github/nag.rs

@@ -16,6 +16,21 @@ use github::models::CommentFromJson;
 use teams::TEAMS;
 use super::GH;

+const LABEL_FFCP: &'static str = "finished-final-comment-period";


Can you make these into (an enum|a set of enums) that have as_str()?

anp · 2018-04-20T18:21:04Z

src/github/nag.rs

+    },
+}
+
+fn is_rfc_repo(issue: &Issue) -> bool {


Can you make this a part of teams.toml (maybe rename to rfcbot.toml and move current fields under a top-level teams key)? It would be very nice to not regress progress towards #92.

anp · 2018-04-20T18:22:35Z

src/github/nag.rs

@@ -898,6 +1003,14 @@ impl<'a> RfcBotComment<'a> {
        }
    }

+    fn couldnt_add_label<'b>(msg: &mut String, author: &'b GitHubUser, label: &str) {
+        msg.push_str("\n\n*psst @");


I'd be ok with this being a call to format! -- in hindsight I think it was a big mistake to be concerned about allocation performance here.

Hmm... can we fix this later as a more general refactoring of message formats and potential templating?

anp · 2018-04-20T18:25:13Z

src/github/nag.rs

+                    match disposition {
+                        FcpDisposition::Merge => {}
+                        FcpDisposition::Close => {
+                            msg.push_str("\n\nBy the power vested in me by Rust, I hereby close this RFC.");


Note that this won't be an accurate comment to post until rfcbot is given more permissions on the rust-lang org. Can we make this conditional on whether actually closing the issue succeeded? That might require a slight change to order of operations.

Then you have to close the issue first which looks a bit weird imo. The current ordering feels natural as how a human would do it :) We should just give the bot these permissions imo.

…e_ffcp_actions.

Centril · 2018-04-21T03:26:50Z

Did some refactoring per @anp's suggestions.
I also took a crack at fixing @aturon's concern in 13c38fd.

The team.toml / rfcbot.toml bit isn't fixed yet tho; still working on that.

anp · 2018-04-21T19:29:31Z

src/github/nag.rs

+                        if proposal.fcp_start.is_some() {
+                            // Update DB: FCP is not started anymore.
+                            proposal.fcp_start = None;
+                            match diesel::update(fcp_proposal.find(proposal.id))


Do you think the bot should comment when it's doing this? Could be confusing to do this without any visible status update?

cc @aturon

My thinking is that:

let _ = issue.add_label(Label::PFCP); issue.remove_label(Label::FCP);

serves as visible status update that the bot reacted to the concern :)

Yeah I think that suffices, given that the concern registration is already a comment.

anp · 2018-04-30T20:11:05Z

Hi @Centril! If I'm reading correctly, the only thing remaining is to configure the RFCs repo string via a configuration file. If that's the case, let's get this in and create a follow-up issue or add a comment on the issue I linked before?

Centril · 2018-04-30T20:20:35Z

@anp that works for me :) Sorry about not following up on this sooner.

One thing I'm a bit unsure about as to making this work for all repos is whether there is one instance of the rfcbot program running for rust-lang/rust and rust-lang/rfcs or if there are separate instances for each.

If there is just one instance, then you need to be able to configure close / postpone behavior separately for each repo.

For now I propose:

[fcp_behaviour."rust-lang/rfcs"]
close = true
postpone = true

[teams]

# other stuff....

anp · 2018-04-30T22:44:29Z

Thank you so much!

Centril added 2 commits April 20, 2018 04:22

improve rfcbot command parsing to be more human friendly.

a330887

refactor + auto-close/postpone FFCPed RFCs + better FFCP comment + mo…

e347795

…re labels.

update toolchain := 2018-04-19 + cargo update

6295b0e

anp reviewed Apr 20, 2018

View reviewed changes

Centril added 3 commits April 21, 2018 01:56

refactor consts into a Label enum.

544d05e

add/remove_label methods on Issue instead of free functions.

2a96c07

fix the GH.close_issue command.

dac6c55

Centril mentioned this pull request Apr 21, 2018

Auto-merge approved RFCs #198

Open

FCP + concern => PFCP per aturon's request; also refactor into execut…

13c38fd

…e_ffcp_actions.

anp reviewed Apr 21, 2018

View reviewed changes

anp mentioned this pull request Apr 25, 2018

new devtools team members #199

Merged

Centril mentioned this pull request Apr 30, 2018

Controllable auto-close/postpone behavior #201

Closed

anp merged commit 9f52efd into rust-lang:master Apr 30, 2018

Centril deleted the feature/human-friendly-incantations branch April 30, 2018 22:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Human friendly incantations + Auto Closing/Postponing RFCs + More labels #197

Human friendly incantations + Auto Closing/Postponing RFCs + More labels #197

Centril commented Apr 20, 2018

Centril commented Apr 20, 2018

Centril commented Apr 20, 2018

aturon commented Apr 20, 2018

anp left a comment

anp Apr 20, 2018

anp Apr 20, 2018

Centril Apr 21, 2018

anp Apr 20, 2018

Centril Apr 21, 2018

anp Apr 20, 2018

Centril Apr 21, 2018 •

edited

Loading

anp Apr 20, 2018

Centril Apr 21, 2018

anp Apr 20, 2018

anp Apr 20, 2018

anp Apr 20, 2018

Centril Apr 21, 2018

anp Apr 21, 2018

anp Apr 20, 2018

Centril Apr 21, 2018

Centril commented Apr 21, 2018

anp Apr 21, 2018

Centril Apr 22, 2018

aturon Apr 28, 2018

anp commented Apr 30, 2018

Centril commented Apr 30, 2018

anp commented Apr 30, 2018

Human friendly incantations + Auto Closing/Postponing RFCs + More labels #197

Human friendly incantations + Auto Closing/Postponing RFCs + More labels #197

Conversation

Centril commented Apr 20, 2018

Centril commented Apr 20, 2018

Centril commented Apr 20, 2018

aturon commented Apr 20, 2018

anp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Centril Apr 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Centril commented Apr 21, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anp commented Apr 30, 2018

Centril commented Apr 30, 2018

anp commented Apr 30, 2018

Centril Apr 21, 2018 •

edited

Loading