Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from
Sep 4, 2023

Conversation

ehuss
Copy link
Contributor

@ehuss ehuss commented Jul 31, 2023

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 077cdf8 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
Copy link
Contributor Author

ehuss commented Aug 28, 2023

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

Copy link
Member

@Mark-Simulacrum Mark-Simulacrum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Search API limits definitely hit us - e.g., lang agendas are rate limited to be generated twice a minute - so I'd avoid those. Not too familiar with cynic but the code here looks fine so fine with that choice too.

@Mark-Simulacrum
Copy link
Member

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

@ehuss
Copy link
Contributor Author

ehuss commented Sep 4, 2023

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
Copy link
Member

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

@Mark-Simulacrum Mark-Simulacrum merged commit a50b73a into rust-lang:master Sep 4, 2023
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

rustbot always thinks @oli-obk is a new contributor
2 participants