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

Affect triage/needs-rebase on Pull Requests with merge conflicts #232

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

gastaldi
Copy link
Contributor

@gastaldi gastaldi commented Jun 9, 2022

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

@gastaldi gastaldi marked this pull request as draft June 9, 2022 21:03
@gastaldi
Copy link
Contributor Author

gastaldi commented Jun 9, 2022

@gsmet WDYT of this?

@gastaldi gastaldi force-pushed the needs_rebase branch 2 times, most recently from 8eca863 to dc99f13 Compare June 9, 2022 21:08
@gastaldi gastaldi marked this pull request as ready for review June 9, 2022 21:08
@gastaldi gastaldi force-pushed the needs_rebase branch 2 times, most recently from 8e63f59 to 97af29e Compare June 9, 2022 21:16
Copy link
Member

@gsmet gsmet left a 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.

README.adoc Outdated Show resolved Hide resolved
GHRepository repository = pushRequestPayload.getRepository();
for (GHPullRequest pullRequest : repository.getPullRequests(GHIssueState.OPEN)) {
Boolean mergeable = pullRequest.getMergeable();
if (mergeable != null) {
Copy link
Member

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?

Copy link
Contributor Author

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

Copy link
Member

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.

README.adoc Outdated Show resolved Hide resolved
@gastaldi gastaldi force-pushed the needs_rebase branch 3 times, most recently from 129830d to f6bf8e1 Compare June 9, 2022 22:56
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 <guillaume.smet@gmail.com>
@gastaldi gastaldi marked this pull request as draft June 10, 2022 01:20
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.

2 participants