search

rust-lang / triagebot

Automation/tooling for Rust spaces
https://triage.rust-lang.org
Apache License 2.0
170 stars 75 forks source link

Use GraphQL to determine if a user is new. #1713

Closed ehuss closed 10 months ago

ehuss commented 11 months ago

For some reason the /repos/ORG/REPO/commits?author=AUTHOR endpoint hasn't been working to see if a user has posted any commits (I don't know why). This switches it to use GraphQL which seems to be more reliable in my testing. AFAICT, the query costs 1 point, so it shouldn't be too expensive.

I could instead revert 077cdf8b11e0aa10d2cdfad655f14bbb0a4532b9 and use the /search endpoint instead. However, that endpoint has some issues, such as not working on forks (mainly a problem for testing), and tight rate limits (30/minute, which is very unlikely to be hit, but I think is very small and could compete with other triagebot handlers).

I decided to not use cynic for this and instead just issue JSON to the endpoint directly. I can build the cynic types, but I'm finding using cynic to be awkward. For one-off queries like this, it generates a dozen types, and it is also a pain to iterate and change the query. If you want to make any changes, you have to find the original query, find and set up the cynic generator, and regenerate the types, and then copy all that stuff back in and adjust all the code using it. Cynic is better if you are actually using the returned values, but in this case there is just one value that is needed in one place.

I'm fine with doing either alternative (use /search or use cynic), just let me know. I just find this approach a little more reliable and easier to write and iterate on.

Fixes #1689

ehuss commented 10 months ago

@Mark-Simulacrum Just checking if you had any questions on this?

Mark-Simulacrum commented 10 months ago

My previous approval is still good here - do we still have the merge limitations, or can you go ahead and merge?

ehuss commented 10 months ago

Hm, I'm not entirely sure why I can't merge. It says I am not authorized. I know that GitHub does not allow an author to approve their own PR, but I've had situations on other repos where after someone else approves it, the author is allowed to merge it. But maybe the branch protection rule in this repo is slightly different. Unfortunately GitHub doesn't provide any information to help debug an authorization problem, and I don't have permissions to inspect the permissions in this repo.

Mark-Simulacrum commented 10 months ago

Yeah, I remembered since then - we unblocked merges for a bit by manually adding the triagebot team to branch protections, but that broke sync-team since it doesn't know what that entry means. https://github.com/rust-lang/team/blob/master/repos/rust-lang/triagebot.toml could have branch protections dropped potentially...