-
Notifications
You must be signed in to change notification settings - Fork 25
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
Affect triage/needs-rebase
on Pull Requests with merge conflicts
#232
base: main
Are you sure you want to change the base?
Conversation
@gsmet WDYT of this? |
8eca863
to
dc99f13
Compare
8e63f59
to
97af29e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added some comment. I thought about doing it a few times but it's not as simple as it appears :).
I think it's probably worth pursuing though, it's just that it needs some fine tuning.
GHRepository repository = pushRequestPayload.getRepository(); | ||
for (GHPullRequest pullRequest : repository.getPullRequests(GHIssueState.OPEN)) { | ||
Boolean mergeable = pullRequest.getMergeable(); | ||
if (mergeable != null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I thought about doing this a few times but this is the tricky part... See https://docs.github.com/en/rest/guides/getting-started-with-the-git-database-api#checking-mergeability-of-pull-requests .
Basically, you would have to poll when the status is null. You can see how GitHub does it as sometimes you go to a PR and the mergeable state is not defined and it appears after a few seconds.
Doing that at a massive level with 100+ pull requests seems problematic to me as you have to be aware that there is some rate limiting. I think you should probably limit it to pull requests opened in the last 15 days maybe.
To limit the number of queries, I would also check if the label is already present before adding or removing it.
Also I would add a comment when the PR needs a rebase (that could be removed when you remove the label if you add a marker similar to what I did here: https://github.com/quarkusio/quarkus-github-bot/blob/main/src/main/java/io/quarkus/bot/workflow/WorkflowConstants.java#L8 ). Unfortunately, the method to list comments in the GitHub API doesn't support the since
parameter which would have been handy to avoid parsing all the old comments.
And finally you should limit the PR considered to the ones targeting the branch of the push event.
Maybe doing this once per day with a scheduled task rather than reacting to push would be less problematic for the rate limiting?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doing that at a massive level with 100+ pull requests seems problematic to me as you have to be aware that there is some rate limiting. I think you should probably limit it to pull requests opened in the last 15 days maybe.
I think using GraphQL I can get all the information I need with one request, add the label with another request and remove the labels with another request (3 in total).
This is how I'd grab the opened pull-requests and their respective mergeable
state:
{
repository(name: "quarkus", owner: "quarkusio") {
pullRequests(states: OPEN, first: 100) {
nodes {
... on PullRequest {
number
title
mergeable
url
labels(first: 10) {
nodes {
... on Label {
name
}
}
}
}
}
}
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, so be careful, the cost model and rate limiting of the GraphQL queries is... complicated :).
But my guess is that it doesn't change a thing: the mergeable
state will be null while GitHub is analyzing it.
129830d
to
f6bf8e1
Compare
This adds the `triage/needs-rebase` label (or removes it) on opened pull-requests, making it easier to find pull-requests that require a rebase to be merged Apply suggestions from code review Co-authored-by: Guillaume Smet <[email protected]>
This adds the
triage/needs-rebase
label (or removes it) on opened pull-requests, making it easier to find pull-requests that require a rebase to be merged