Add `glacier` command to track ICEs #526

Elinvynia · 2020-05-11T22:09:33Z

This is building on the work done in #234, huge props to @hellow554 for the initial effort!

I have no idea how to run the bot, but the tests I added pass.

hellow554 · 2020-05-12T07:13:01Z

parser/src/command/glacier.rs

+            match toks.next_token()? {
+                Some(Token::Quote(s)) => {
+                    let source = s.to_owned();
+                    let playground_url = Regex::new("https://play.rust-lang.org/.*").unwrap();


Why not using starts_with? I don't like using a regex for this case, because you pull in the crate just for this piece and you could circumvent it.

That's a very good suggestion! It didn't cross my mind, heh.

Elinvynia · 2020-05-21T14:02:57Z

Status update: Currently waiting on XAMPPRocky/octocrab#12 to be merged to allow me to do this cleanly.

LeSeulArtichaut · 2020-05-21T17:18:35Z

@Elinvynia XAMPPRocky/octocrab#12 has been merged

LeSeulArtichaut

Looks promising overall :)

LeSeulArtichaut · 2020-05-21T21:25:50Z

src/handlers/glacier.rs

+    let number = event.issue().unwrap().number;
+    let user = event.user();
+
+    let octocrab = octocrab::instance();


As asked by @Mark-Simulacrum, we should not use the static instance here. Let's store it in Context alongside the GithubClient

LeSeulArtichaut · 2020-05-21T21:27:11Z

src/handlers/glacier.rs

+
+    let octocrab = octocrab::instance();
+
+    let fork = octocrab.repos("rust-lang", "glacier");


I assume we @Mark-Simulacrum will create a fork for the bot, so to not pollute the main rust-lang/glacier repository? In that case you just need to set the owner to "rustbot" instead of "rust-lang"

LeSeulArtichaut · 2020-05-21T21:30:24Z

src/handlers/glacier.rs

+    };
+
+    fork.create_ref(&Reference::Branch(format!("triagebot-ice-{}", number)), master).await?;
+    fork.create_file(format!("ices/{}.rs", number), format!("This PR was created by {} on issue {}.", user.login, number), body)


This message is actually the commit message. You probably want to change that, to something like "Add ICE reproduction for issue #{}"?

LeSeulArtichaut · 2020-05-21T21:32:45Z

src/handlers/glacier.rs

+
+    fork.create_ref(&Reference::Branch(format!("triagebot-ice-{}", number)), master).await?;
+    fork.create_file(format!("ices/{}.rs", number), format!("This PR was created by {} on issue {}.", user.login, number), body)
+        .branch("triagebot-ice")


Use the branch you created earlier.

Suggested change

.branch("triagebot-ice")

.branch(format!("triagebot-ice-{}", number))

LeSeulArtichaut · 2020-05-21T21:34:05Z

src/handlers/glacier.rs

+
+    octocrab.pulls("rust-lang", "glacier")
+        .create(format!("ICE - {}", number), format!("triagebot-ice-{}", number), "master")
+        .body("This is a fake new catastrophic avalanche of ICE!")


This is the body (description) of the PR. Let's change it to something like "Automatically created by @{} in issue #{}"

LeSeulArtichaut · 2020-05-21T21:34:48Z

src/handlers/glacier.rs

+        .await?;
+
+    octocrab.pulls("rust-lang", "glacier")
+        .create(format!("ICE - {}", number), format!("triagebot-ice-{}", number), "master")


If we actually create a fork for rustbot, you'll need to set this to "rustbot:triagebot-ice-{}"

LeSeulArtichaut · 2020-05-21T21:37:18Z

src/handlers/glacier.rs

+    };
+
+    let url = cmd.source;
+    let response = reqwest::get(&format!("{}{}", url.replace("github", "githubusercontent"), "/playground.rs")).await?;


Use the reqwest::Client inside of the GithubClient using GithubClient::raw

Mark-Simulacrum · 2020-05-22T10:59:52Z

parser/src/command/glacier.rs

+    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+        match self {
+            Self::NoLink => write!(f, "no link provided"),
+            Self::InvalidLink => write!(f, "invalid link"),


Let's note here that it should be a link to a gist.

Mark-Simulacrum · 2020-05-22T11:00:26Z

parser/src/command/glacier.rs

+impl fmt::Display for ParseError {
+    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+        match self {
+            Self::NoLink => write!(f, "no link provided"),


We should note the requirement for quotes around the link.

Mark-Simulacrum · 2020-05-22T11:01:51Z

src/handlers/glacier.rs

+
+        match event {
+            Event::IssueComment(_) => (),
+            _ => {return Ok(None);}


We probably want to run on newly opened issues too, not just comments?

Mark-Simulacrum · 2020-05-22T11:02:30Z

src/handlers/glacier.rs

+            Command::Glacier(Ok(command)) => Ok(Some(command)),
+            Command::Glacier(Err(err)) => {
+                return Err(format!(
+                    "Parsing assign command in [comment]({}) failed: {}",


nit: glacier, not assign command.

Mark-Simulacrum · 2020-05-22T11:05:47Z

src/handlers/glacier.rs

+    let master = if let Object::Commit { sha, ..} = master {
+        sha
+    } else {
+        panic!()


Please add the actual value of master here so that we can debug if it does occur.

Well I think this should be unreachable!() instead.

Regardless, we should note what we actually encountered.

Mark-Simulacrum · 2020-05-22T11:06:49Z

src/handlers/glacier.rs

+        panic!()
+    };
+
+    fork.create_ref(&Reference::Branch(format!("triagebot-ice-{}", number)), master).await?;


Will this fail if the branch already exists? Sometimes we have multiple distinct examples for a single ICE.

Honesly I am not sure what happens here, how should we proceed? Append a random string and hope for no conflict?

Seems fine to leave as is for now.

Mark-Simulacrum

I would like to see octocrab moved to a crates.io dependency before merging, and I've also asked @XAMPPRocky over zulip to see if we can use a subset of tokio's feature flags rather than full.

Other than that, this looks good!

src/handlers/glacier.rs

Co-authored-by: XAMPPRocky <[email protected]>

Cargo.toml

hellow554 mentioned this pull request May 12, 2020

Add glacier command for adding ICE snippets easily #234

Closed

hellow554 reviewed May 12, 2020

View reviewed changes

Elinvynia added 5 commits May 21, 2020 11:28

Add glacier command to track ICEs

15f46b0

Fix docs

f8b1526

Remove regex dependency

5f91822

Handler skeleton

539e138

Fix tests

7e4cfd1

First implementation proposal

c017219

LeSeulArtichaut suggested changes May 21, 2020

View reviewed changes

Address review requests

30e671f

Mark-Simulacrum reviewed May 22, 2020

View reviewed changes

Address most review requests

8a3668e

Mark-Simulacrum reviewed May 22, 2020

View reviewed changes

XAMPPRocky reviewed May 22, 2020

View reviewed changes

src/handlers/glacier.rs Outdated Show resolved Hide resolved

Update src/handlers/glacier.rs

b2d14bc

Co-authored-by: XAMPPRocky <[email protected]>

XAMPPRocky reviewed May 22, 2020

View reviewed changes

Cargo.toml Outdated Show resolved Hide resolved

Replace octocrab git dependency

f0af1c6

Mark-Simulacrum merged commit fcbcc52 into rust-lang:master May 22, 2020

LeSeulArtichaut mentioned this pull request May 22, 2020

Automatically add a testcase via a comment rust-lang/glacier#231

Closed

Elinvynia deleted the pr branch May 26, 2020 07:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `glacier` command to track ICEs #526

Add `glacier` command to track ICEs #526

Elinvynia commented May 11, 2020

hellow554 May 12, 2020 •

edited

Loading

Elinvynia May 12, 2020

Elinvynia commented May 21, 2020

LeSeulArtichaut commented May 21, 2020

LeSeulArtichaut left a comment

LeSeulArtichaut May 21, 2020

LeSeulArtichaut May 21, 2020

LeSeulArtichaut May 21, 2020

LeSeulArtichaut May 21, 2020

LeSeulArtichaut May 21, 2020

LeSeulArtichaut May 21, 2020

LeSeulArtichaut May 21, 2020

Mark-Simulacrum May 22, 2020

Mark-Simulacrum May 22, 2020

Mark-Simulacrum May 22, 2020

Mark-Simulacrum May 22, 2020

Mark-Simulacrum May 22, 2020

LeSeulArtichaut May 22, 2020

Mark-Simulacrum May 22, 2020

Mark-Simulacrum May 22, 2020

Elinvynia May 22, 2020

Mark-Simulacrum May 22, 2020

Mark-Simulacrum left a comment


		let octocrab = octocrab::instance();

		let fork = octocrab.repos("rust-lang", "glacier");

	.branch("triagebot-ice")
	.branch(format!("triagebot-ice-{}", number))

Add glacier command to track ICEs #526

Add glacier command to track ICEs #526

Conversation

Elinvynia commented May 11, 2020

hellow554 May 12, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Elinvynia commented May 21, 2020

LeSeulArtichaut commented May 21, 2020

LeSeulArtichaut 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

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

Choose a reason for hiding this comment

Mark-Simulacrum left a comment

Choose a reason for hiding this comment

Add `glacier` command to track ICEs #526

Add `glacier` command to track ICEs #526

hellow554 May 12, 2020 •

edited

Loading