Stream: t-release/triagebot

Topic: Transition to the Octocrab GitHub API


LeSeulArtichaut (May 15 2020 at 18:06, on Zulip):

@Elinvynia @XAMPPRocky Created a new topic to discuss about Octocrab :innocent:

LeSeulArtichaut (May 15 2020 at 18:06, on Zulip):

@simulacrum I’m unsure if I should use a static instance (Octocrab::instance), or store the Octocrab client just like how we store the GithubClient today, passing it as context to any function that might need it...

LeSeulArtichaut (May 15 2020 at 18:07, on Zulip):

Do you have a preference, or arguments about how it affects performance? I have absolutely no idea of the drawbacks of both approaches

simulacrum (May 15 2020 at 18:09, on Zulip):

Please don't use the global

simulacrum (May 15 2020 at 18:09, on Zulip):

That's just generally more painful in the long run

LeSeulArtichaut (May 15 2020 at 18:40, on Zulip):

Alright

Elinvynia (May 15 2020 at 22:42, on Zulip):

@LeSeulArtichaut https://github.com/LeSeulArtichaut/triagebot is this the repo you are working in?

LeSeulArtichaut (May 16 2020 at 00:27, on Zulip):

Yes, but I haven't pushed anything yet :confused:

Elinvynia (May 16 2020 at 22:00, on Zulip):

@XAMPPRocky A bit of a stupid question, but how would I actually create the "diff" for a PR with octograb?

XAMPPRocky (May 16 2020 at 22:02, on Zulip):

@Elinvynia I haven’t implemented that API yet so you need to use the HTTP API for now.

XAMPPRocky (May 16 2020 at 22:04, on Zulip):

You need to set a custom media type. https://developer.github.com/v3/pulls/#custom-media-types

LeSeulArtichaut (May 16 2020 at 22:07, on Zulip):

I can also try help implementing some things in Octocrab

XAMPPRocky (May 17 2020 at 07:03, on Zulip):

@Elinvynia Okay, I've just pushed to master a API that should allow to get a diff or patch with pulls().get_diff and pulls().get_patch. Try it out, and if there's no bugs I'll release it soon.

XAMPPRocky (May 17 2020 at 07:03, on Zulip):

@LeSeulArtichaut Thanks for being so helpful, I've reviewed your PRs now. :slight_smile:

XAMPPRocky (May 17 2020 at 09:45, on Zulip):

I've just released 0.2.3 which has the get diff and patch methods as well adds the markdown api and the gitignore api.

Elinvynia (May 18 2020 at 12:30, on Zulip):

Is there an easy tool for creating a patch/diff?

XAMPPRocky (May 18 2020 at 12:58, on Zulip):

@Elinvynia Oh sorry I see what you meant now. You would need to use the Git Data API. https://developer.github.com/v3/git/

XAMPPRocky (May 18 2020 at 13:01, on Zulip):

You don't create a patch or diff to send, you commit to a branch.

XAMPPRocky (May 18 2020 at 13:38, on Zulip):

You might have an easier time using git2 directly.

simulacrum (May 18 2020 at 13:42, on Zulip):

git2 probably requires a file system which we ~don't have

Pietro Albini (May 18 2020 at 13:48, on Zulip):

well it can be reset at any point, but we have it

simulacrum (May 18 2020 at 14:00, on Zulip):

hm okay -- that seems like a road to needing integrity checks all over the place and such, I'd rather avoid it if possible

Elinvynia (May 18 2020 at 16:24, on Zulip):

I'll think about possible solutions then! Perhaps it opens up an issue with a link to the code? Still easier than us having to find them in the list of ICEs

simulacrum (May 18 2020 at 16:29, on Zulip):

it should be possible to do everything through the github api I think

Elinvynia (May 18 2020 at 16:35, on Zulip):

To open a PR with changes purely through the API? I only found how to open a PR between branches

simulacrum (May 18 2020 at 17:13, on Zulip):

well, you'd create a branch via the api and then open a pr with that branch

LeSeulArtichaut (May 18 2020 at 20:27, on Zulip):

Hehe, I feel lonely

LeSeulArtichaut (May 18 2020 at 20:28, on Zulip):

@Elinvynia From what I've seen (from my own investigation, I mean), when you create a new file using GitHub's UI, you send a POST request to https://github.com/:owner/:repo/create/:branch/path/to/folder

LeSeulArtichaut (May 18 2020 at 20:28, on Zulip):

I'm not sure if we can use that in an automated app though

LeSeulArtichaut (May 18 2020 at 20:29, on Zulip):

Here is the form data:


LeSeulArtichaut (May 18 2020 at 20:32, on Zulip):

If JavaScript is able to make that commit in the browser's sandbox, then I think it should be possible with triagebot?

LeSeulArtichaut (May 18 2020 at 20:33, on Zulip):

@Elinvynia Nevermind me, there is actually an API endpoint that allows you to create files

LeSeulArtichaut (May 18 2020 at 20:35, on Zulip):

There is also an API endpoint to crate a fork but I think we can create that manually and hard-code it into triagebot?

LeSeulArtichaut (May 18 2020 at 20:38, on Zulip):

Here is how you can create a branch, but you'd need to get the master HEAD commit first

LeSeulArtichaut (May 18 2020 at 20:38, on Zulip):

And I think that's all we need, right? :slight_smile:

Elinvynia (May 18 2020 at 20:45, on Zulip):

Thanks a lot!

LeSeulArtichaut (May 18 2020 at 20:48, on Zulip):

Elinvynia said:

Thanks a lot!

You're welcome :slight_smile:
To be fair though, most of my results don't come from GitHub's doc directly but from StackOverflow posts :big_smile:

XAMPPRocky (May 19 2020 at 13:51, on Zulip):

I'll probably start work on the repositories API soon so hopefully this functionality will be available in a day or two.

LeSeulArtichaut (May 19 2020 at 13:57, on Zulip):

@XAMPPRocky Oh, I was going to implement the functionalities we'll need for triagebot. Would you mind accepting a PR with those few functionalities?

XAMPPRocky (May 19 2020 at 14:00, on Zulip):

Yeah sure.

LeSeulArtichaut (May 19 2020 at 14:00, on Zulip):

:+1:

LeSeulArtichaut (May 19 2020 at 17:18, on Zulip):

@XAMPPRocky @Elinvynia I made a small proof-of-concept program, but it created this!

XAMPPRocky (May 19 2020 at 17:20, on Zulip):

Can you share the proof of concept? I'd be interested in seeing it.

LeSeulArtichaut (May 19 2020 at 17:20, on Zulip):

I was going to ;)

LeSeulArtichaut (May 19 2020 at 17:20, on Zulip):

However I'm diving into a backtrace

LeSeulArtichaut (May 19 2020 at 17:21, on Zulip):

Octocat ran into an issue when deserializing the response to creating the PR

LeSeulArtichaut (May 19 2020 at 17:21, on Zulip):

Error("invalid type: null, expected a string", line: 1, column: 1657)

LeSeulArtichaut (May 19 2020 at 17:21, on Zulip):

It's a shame it doesn't give more info, like which field it issued at

LeSeulArtichaut (May 19 2020 at 17:22, on Zulip):

Anyway here's the code:

use std::env;
use octocrab::Octocrab;
use octocrab::params::repos::Reference;
use octocrab::models::Object;

#[tokio::main]
async fn main() -> octocrab::Result<()> {
    let octocrab = Octocrab::builder()
        .personal_token(env::var("TOKEN").expect("No GitHub token"))
        .build()
        .unwrap();

    let fork = octocrab.repos("LeSeulArtichaut", "glacier");
    let base = octocrab.repos("LeSeulArtichaut", "glacier");

    let master = base.get_ref(&Reference::Branch("master".to_string())).await?.object;
    let master = if let Object::Commit { sha, ..} = master {
        sha
    } else {
        panic!()
    };
    fork.create_ref(&Reference::Branch("ice-dummy".to_string()), master).await?;
    fork.create_file("ices/00000.rs", "Test creating a file from Octocat", "fn main() {}")
        .branch("ice-dummy")
        .send()
        .await?;
    base.pulls()
        .create("Dummy ICE!", "ice-dummy", "master")
        .body("This is a fake new catastrophic avalanche of ICE!")
        .send()
        .await?;
    Ok(())
}
XAMPPRocky (May 19 2020 at 17:23, on Zulip):

I would also love that in general for serde_json, its errors are basically useless as is in HTTP apps.

LeSeulArtichaut (May 19 2020 at 17:24, on Zulip):

I don't want to compare by hand :sob:

LeSeulArtichaut (May 19 2020 at 17:24, on Zulip):

Too many fields :D

simulacrum (May 19 2020 at 17:24, on Zulip):

https://docs.rs/serde_path_to_error/0.1.2/serde_path_to_error/

XAMPPRocky (May 19 2020 at 17:25, on Zulip):

@simulacrum :heart:

LeSeulArtichaut (May 19 2020 at 17:25, on Zulip):

:surprise:

LeSeulArtichaut (May 19 2020 at 17:25, on Zulip):

:tada:

LeSeulArtichaut (May 19 2020 at 17:26, on Zulip):

I'll try that out then

XAMPPRocky (May 19 2020 at 17:27, on Zulip):

@LeSeulArtichaut That example is great, even in that small example you used the library in a way I didn't think people would.

LeSeulArtichaut (May 19 2020 at 17:28, on Zulip):

Well that's the beauty of open-source, right?

XAMPPRocky (May 19 2020 at 17:28, on Zulip):

Also makes me increasingly confident in the builder pattern approach to the design.

LeSeulArtichaut (May 19 2020 at 17:29, on Zulip):

Oh there's another issue I wanted to talk about

LeSeulArtichaut (May 19 2020 at 17:29, on Zulip):

Serde deserializes None into a null value

LeSeulArtichaut (May 19 2020 at 17:29, on Zulip):

Which doesn't make GitHub happy for certain cases

XAMPPRocky (May 19 2020 at 17:29, on Zulip):

Oh yeah, but null is not "don't show up in the json"

LeSeulArtichaut (May 19 2020 at 17:30, on Zulip):

I think we'll need to slap some #[serde(skip_serializing_if = "Option::is_none")] everywhere in the lib

LeSeulArtichaut (May 19 2020 at 17:30, on Zulip):

Because it triggered me some errors

LeSeulArtichaut (May 19 2020 at 17:30, on Zulip):

And I'll probably not be the only one :D

XAMPPRocky (May 19 2020 at 17:31, on Zulip):

Feel free to add it as you need, I wish there was a way to set that somehow at the struct or library level, since I never want to serialize None as null.

LeSeulArtichaut (May 19 2020 at 17:32, on Zulip):

Maybe we can have our own Option type

LeSeulArtichaut (May 19 2020 at 17:32, on Zulip):

Which doesn't deserialize as null?

LeSeulArtichaut (May 19 2020 at 17:32, on Zulip):

May be interesting :nerd:

XAMPPRocky (May 19 2020 at 17:37, on Zulip):

@LeSeulArtichaut That would actually be an acceptable alternative. And it's actually easy to implement.

#[derive(Serialize)]
#[serde(untagged)]
enum MyOption<T> {
  Some(T),
  #[serde(skip)]
  None,
}
simulacrum (May 19 2020 at 17:49, on Zulip):

/me wishes that github api had an actual spec so that we'd know when to do it

simulacrum (May 19 2020 at 17:50, on Zulip):

I would personally avoid a custom option type, and much prefer the skip_serializing_if annotations

XAMPPRocky (May 19 2020 at 17:59, on Zulip):

Yeah, my solution does mean that it basically be only used for serialisation, and you lose all of options methods. Thinking on it more the skip_serializing_if noise is fine. Maybe this is another thing a hypothetical proc macro would solve XAMPPRocky/octocrab#11.

XAMPPRocky (May 19 2020 at 18:21, on Zulip):

@LeSeulArtichaut Back to your example, why did you create fork and baserather than using just one of those?

Elinvynia (May 19 2020 at 19:03, on Zulip):

@LeSeulArtichaut Great work! I've been experimenting a bit myself, though I am not as experienced as you.

LeSeulArtichaut (May 19 2020 at 19:07, on Zulip):

LeSeulArtichaut Back to your example, why did you create fork and baserather than using just one of those?

Basically, I originally wanted to file a PR against the real rust-lang/glacier repo, but I didn’t want to make any noise so I changed it back to my fork. I let the two variables to make it easy to go back to having the PR agaist another repo.

XAMPPRocky (May 19 2020 at 20:42, on Zulip):

Ah makes sense.

LeSeulArtichaut (May 19 2020 at 20:43, on Zulip):

@XAMPPRocky I'm filing a PR to integrate serde_path_to_error btw

LeSeulArtichaut (May 19 2020 at 20:52, on Zulip):

XAMPPRocky/octocrab#14

LeSeulArtichaut (May 19 2020 at 21:00, on Zulip):

(Ping @XAMPPRocky :sweat_smile:)

LeSeulArtichaut (May 19 2020 at 21:11, on Zulip):

Thanks! I can rebase my branches and get proper errors now :D

Elinvynia (May 21 2020 at 10:54, on Zulip):

@LeSeulArtichaut I created a PR to address the review requests by XAMPP https://github.com/LeSeulArtichaut/octocrab/pull/1

LeSeulArtichaut (May 21 2020 at 13:32, on Zulip):

@Elinvynia Thank you very much! To thank you I'll give you a small tip: instead of:

fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result {
    f.write_str(&format!("refs/{}", self.ref_url()))
}

You can write:

fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result {
    write!(f, "refs/{}", self.ref_url())
}
Elinvynia (May 21 2020 at 13:55, on Zulip):

Interesting, thanks!

LeSeulArtichaut (May 21 2020 at 13:57, on Zulip):

Btw here are the docs

Elinvynia (May 21 2020 at 17:23, on Zulip):

@XAMPPRocky How often are octocrab releases made? I guess it wouldn't be acceptable to use a git dependency

XAMPPRocky (May 21 2020 at 17:23, on Zulip):

@Elinvynia Use a git dependency and tell me if your code works, and if it does I'll make a release soon.

Elinvynia (May 21 2020 at 18:03, on Zulip):

@XAMPPRocky There is an issue, though:

error: failed to select a version for `base64`.
    ... required by package `postgres-protocol v0.5.1`
    ... which is depended on by `tokio-postgres v0.5.4`
    ... which is depended on by `postgres-native-tls v0.3.0`
    ... which is depended on by `triagebot v0.1.0 (/home/-snip-/projects/triagebot)`
versions that meet the requirements `= 0.12.0` are: 0.12.0

all possible versions conflict with previously selected packages.

  previously selected package `base64 v0.12.1`
    ... which is depended on by `octocrab v0.2.3 (https://github.com/XAMPPRocky/octocrab#077e39d3)`
    ... which is depended on by `triagebot v0.1.0 (/home/-snip-/projects/triagebot)`

failed to select a version for `base64` which could resolve this conflict
XAMPPRocky (May 21 2020 at 18:04, on Zulip):

@Elinvynia Fixed

Elinvynia (May 21 2020 at 18:05, on Zulip):

Thanks, it compiles now!

XAMPPRocky (May 21 2020 at 18:11, on Zulip):

@Elinvynia But does it run? :stuck_out_tongue_wink:

LeSeulArtichaut (May 21 2020 at 18:13, on Zulip):

It might not :D

LeSeulArtichaut (May 21 2020 at 18:14, on Zulip):

If we encounter problems with serializing Nones or failing to deserialize requests from GitHub

Elinvynia (May 21 2020 at 18:14, on Zulip):

Well, running it is a bit more difficult since that means setting up triagebot locally

Elinvynia (May 21 2020 at 18:14, on Zulip):

So far it compiles, at least, using the example by Artichaut with one small tweak (octograb.pulls instead of repo.pulls)

LeSeulArtichaut (May 21 2020 at 18:15, on Zulip):

@XAMPPRocky Shall I go and mark all Options in Serialize types as #[serde(skip_serializing_if = "Option::is_none")]?

LeSeulArtichaut (May 21 2020 at 18:16, on Zulip):

@Elinvynia What did you use Octocrab for, exactly?

LeSeulArtichaut (May 21 2020 at 18:16, on Zulip):

If you just used methods that I used in my example, it should be fine

LeSeulArtichaut (May 21 2020 at 18:16, on Zulip):

I hope :fingers_crossed:

XAMPPRocky (May 21 2020 at 18:16, on Zulip):

@LeSeulArtichaut Yes, but wait a moment. I'm about to push some code.

LeSeulArtichaut (May 21 2020 at 18:16, on Zulip):

Got it

XAMPPRocky (May 21 2020 at 18:17, on Zulip):

Pushed

LeSeulArtichaut (May 21 2020 at 18:22, on Zulip):

Oh actually I think we should have this patch:

diff --git a/src/api/pulls/create.rs b/src/api/pulls/create.rs
index 7c4c525..3783ec2 100644
--- a/src/api/pulls/create.rs
+++ b/src/api/pulls/create.rs
@@ -7,8 +7,11 @@ pub struct CreatePullRequestBuilder<'octo, 'b> {
     title: String,
     head: String,
     base: String,
+    #[serde(skip_serializing_if = "Option::is_none")]
     body: Option<String>,
+    #[serde(skip_serializing_if = "Option::is_none")]
     draft: Option<bool>,
+    #[serde(skip_serializing_if = "Option::is_none")]
     maintainer_can_modify: Option<bool>,
 }

diff --git a/src/models.rs b/src/models.rs
index 47bd694..660ab91 100644
--- a/src/models.rs
+++ b/src/models.rs
@@ -33,7 +33,7 @@ pub struct PullRequest {
     pub closed_at: Option<chrono::DateTime<chrono::Utc>>,
     pub mergeable: Option<bool>,
     pub merged_at: Option<String>,
-    pub merge_commit_sha: String,
+    pub merge_commit_sha: Option<String>,
     pub assignee: Option<User>,
     pub assignees: Vec<User>,
     pub requested_reviewers: Vec<User>,
LeSeulArtichaut (May 21 2020 at 18:22, on Zulip):

I forgot to include it in my repos PR

XAMPPRocky (May 21 2020 at 18:22, on Zulip):

@LeSeulArtichaut Ping me here when you’ve made the PR, and I’ll release with those changes later today.

LeSeulArtichaut (May 21 2020 at 19:47, on Zulip):

@XAMPPRocky opened XAMPPRocky/octocrab#16

LeSeulArtichaut (May 21 2020 at 19:58, on Zulip):

That's a solid +226 lines xD

Elinvynia (May 21 2020 at 20:52, on Zulip):

@LeSeulArtichaut @XAMPPRocky I made something that compiles and looks like it could work in my latest commit - https://github.com/rust-lang/triagebot/pull/526

XAMPPRocky (May 21 2020 at 20:59, on Zulip):

@Elinvynia The builder struct is unused, so it won’t work as is. I’d recommend adding Octocrab to Context, and using that in your Handler implementation. https://github.com/rust-lang/triagebot/blob/2822adc8be10b5fd46b95fefe240e2b31f6e3e62/src/handlers.rs

XAMPPRocky (May 22 2020 at 00:50, on Zulip):

@Elinvynia I've published 0.3 to crates.io

Elinvynia (May 22 2020 at 13:02, on Zulip):

@XAMPPRocky Is there a way to check if a branch exists?

XAMPPRocky (May 22 2020 at 13:07, on Zulip):

@Elinvynia Yes, with Octocrab::repos::get_ref. E.g

if octocrab.repos("owner", "repo").get_ref(&Reference::Branch("master")).is_ok() {
  // branch exists
}
Elinvynia (May 22 2020 at 13:14, on Zulip):

Will it give a specific error if it doesn't exist (rather than for example if ratelimit was reached)

simulacrum (May 22 2020 at 13:19, on Zulip):

@XAMPPRocky Is there a reason you're enabling "full" on tokio's features? That brings in things like signal handling which, well, seems like a bit of waste dependency wise :)

XAMPPRocky (May 22 2020 at 13:19, on Zulip):

No reason.

XAMPPRocky (May 22 2020 at 13:20, on Zulip):

Really I shouldn't be pulling in tokio directly at all, I'm only using it in examples and tests.

Elinvynia (May 22 2020 at 13:22, on Zulip):

Would this be an use case for dev dependencies?

simulacrum (May 22 2020 at 13:23, on Zulip):

Ah okay, would be good to fix that :)

re:the glacier PR, I just left a comment there as well but happy to merge that with a crates.io dep for octocrab

simulacrum (May 22 2020 at 13:23, on Zulip):

no rush of course

XAMPPRocky (May 22 2020 at 13:31, on Zulip):

I'm publishing 0.3.1 that moves tokio to dev-dependencies now.

LeSeulArtichaut (May 22 2020 at 13:33, on Zulip):

@Elinvynia IIRC in my testing create_ref doesn’t error if the ref exists

LeSeulArtichaut (May 22 2020 at 13:33, on Zulip):

I will do some testing when I have time later today

XAMPPRocky (May 22 2020 at 13:36, on Zulip):

Yeah I tried looking into that, and neither GitHub's nor Git's documentation give any real indication on whether it would work or fail.

Elinvynia (May 22 2020 at 13:40, on Zulip):

At least its still better than reddit's API

LeSeulArtichaut (May 22 2020 at 13:40, on Zulip):

documentation is hard

Elinvynia (May 22 2020 at 15:36, on Zulip):

@simulacrum Sorry for the stupid question but what does "r=me" mean?

simulacrum (May 22 2020 at 15:38, on Zulip):

will merge when whatever I said was done

LeSeulArtichaut (May 22 2020 at 15:51, on Zulip):

I'm going to test creating a branch that exists

LeSeulArtichaut (May 22 2020 at 16:05, on Zulip):

Here's the GitHub response if the branch already exists:

{
  "message": "Reference already exists",
  "documentation_url": "https://developer.github.com/v3/git/refs/#create-a-reference"
}

Which means octocrab will return an Err(Error::Github).

Elinvynia (May 22 2020 at 16:24, on Zulip):

Alright, I think I should be able to incorporate that, thanks a lot Artichaut!

LeSeulArtichaut (May 22 2020 at 16:24, on Zulip):

You're welcome ;)

LeSeulArtichaut (May 22 2020 at 16:24, on Zulip):

By the way do you speak French? :P

Elinvynia (May 22 2020 at 17:01, on Zulip):

Unfortunately not :c

LeSeulArtichaut (May 22 2020 at 17:03, on Zulip):

I thought you did :D

Elinvynia (May 22 2020 at 17:23, on Zulip):

:P

Elinvynia (May 22 2020 at 17:24, on Zulip):

@XAMPPRocky What can I help with on octocrab?

XAMPPRocky (May 22 2020 at 17:29, on Zulip):

@Elinvynia The most helpful thing right now is adding more methods for different endpoints. Generally how I have been doing it is going to https://octokit.github.io/rest.js/v17/ Picking a section and going through and implementing the different methods.

LeSeulArtichaut (May 22 2020 at 17:42, on Zulip):

@Elinvynia @simulacrum Should we add a page in the wiki for the glacier command?

simulacrum (May 22 2020 at 17:42, on Zulip):

yes please do!

Elinvynia (May 22 2020 at 17:49, on Zulip):

Sure!

Elinvynia (May 22 2020 at 22:10, on Zulip):

@XAMPPRocky Can I make the error fields public? Right now I can't match on them

XAMPPRocky (May 22 2020 at 22:13, on Zulip):

@Elinvynia Yeah go ahead, if you could also add #[non_exhaustive] to the struct that’s be great.

Elinvynia (May 31 2020 at 15:52, on Zulip):

@XAMPPRocky Does the instance() work now?

XAMPPRocky (May 31 2020 at 15:53, on Zulip):

@Elinvynia What do you mean?

Elinvynia (May 31 2020 at 15:55, on Zulip):

The "build the instance then use it anywhere"

LeSeulArtichaut (May 31 2020 at 15:56, on Zulip):

I think simulacrum didn't want to use static instances in Triagebot

simulacrum (May 31 2020 at 15:56, on Zulip):

Yes whenever I've done that sort of thing in the past it's just led to trouble later down the line

simulacrum (May 31 2020 at 15:57, on Zulip):

not really entirely sure I feel that it belongs in octocrab either vs. having people roll their own given how relatively easy it is

Elinvynia (May 31 2020 at 15:58, on Zulip):

Oh I thought the issue was it not working rather than it being a decision to avoid it

Last update: Sep 22 2020 at 00:45UTC