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

Centril · Apr 20, 2018

This PR aims to fix:

#193 by adding aliases and fault tolerant incantations (and a whole lot more in that direction)
#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.

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

Centril · Apr 20, 2018

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

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

Centril · Apr 20, 2018

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

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.

aturon · Apr 20, 2018

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 · Apr 20, 2018

anp reviewed Apr 20, 2018

View changes

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!

src/github/nag.rs

anp · Apr 20, 2018

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. :)

src/github/nag.rs

anp · Apr 20, 2018

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.

src/github/nag.rs

anp · Apr 20, 2018

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.

@anp

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.

Centril · Apr 21, 2018

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 · Apr 21, 2018

anp reviewed Apr 21, 2018

View changes

anp · Apr 21, 2018

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.

@Centril

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?

anp · Apr 30, 2018

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?

@anp

@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....

Centril · Apr 30, 2018

@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....

Thank you so much!

anp · Apr 30, 2018

Thank you so much!

Centril added some commits Apr 20, 2018

improve rfcbot command parsing to be more human friendly.

a330887

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

e347795

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

6295b0e

Centril added some commits Apr 20, 2018

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 referenced this pull request Apr 21, 2018
Open
Auto-merge approved RFCs #198

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

13c38fd

anp referenced this pull request Apr 25, 2018
Merged
new devtools team members #199

Centril referenced this pull request Apr 30, 2018
Closed
Controllable auto-close/postpone behavior #201

anp merged commit 9f52efd into anp:master Apr 30, 2018
1 check passed

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

Centril deleted the Centril:feature/human-friendly-incantations branch Apr 30, 2018

anp/rfcbot-rs

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

Centril commented Apr 20, 2018

Centril added some commits Apr 20, 2018

This comment has been minimized.

Centril commented Apr 20, 2018

This comment has been minimized.

Centril commented Apr 20, 2018

This comment has been minimized.

aturon commented Apr 20, 2018

anp reviewed Apr 20, 2018

View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Centril added some commits Apr 20, 2018

Centril referenced this pull request Apr 21, 2018

Auto-merge approved RFCs #198

This comment has been minimized.

Centril commented Apr 21, 2018

anp reviewed Apr 21, 2018

View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

anp referenced this pull request Apr 25, 2018

new devtools team members #199

This comment has been minimized.

anp commented Apr 30, 2018

This comment has been minimized.

Centril commented Apr 30, 2018

Centril referenced this pull request Apr 30, 2018

Controllable auto-close/postpone behavior #201

anp merged commit `9f52efd` into anp:master Apr 30, 2018
1 check passed

1 check passed

This comment has been minimized.

anp commented Apr 30, 2018

Centril deleted the Centril:feature/human-friendly-incantations branch Apr 30, 2018

anp/rfcbot-rs

Join GitHub today

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

Conversation

Centril commented Apr 20, 2018

Centril added some commits Apr 20, 2018

This comment has been minimized.

Centril commented Apr 20, 2018

This comment has been minimized.

Centril commented Apr 20, 2018

This comment has been minimized.

aturon commented Apr 20, 2018

anp reviewed Apr 20, 2018 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Centril added some commits Apr 20, 2018

Centril referenced this pull request Apr 21, 2018

This comment has been minimized.

Centril commented Apr 21, 2018

anp reviewed Apr 21, 2018 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

anp referenced this pull request Apr 25, 2018

This comment has been minimized.

anp commented Apr 30, 2018

This comment has been minimized.

Centril commented Apr 30, 2018

Centril referenced this pull request Apr 30, 2018

Hide details View details anp merged commit 9f52efd into anp:master Apr 30, 2018 1 check passed

1 check passed

This comment has been minimized.

anp commented Apr 30, 2018

Centril deleted the Centril:feature/human-friendly-incantations branch Apr 30, 2018

anp reviewed Apr 20, 2018

View changes

anp reviewed Apr 21, 2018

View changes

anp merged commit `9f52efd` into anp:master Apr 30, 2018
1 check passed