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

.github/workflows: add auto label merge conflicts #99064

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

Conversation

ryantm
Copy link
Member

@ryantm ryantm commented Sep 29, 2020

OfBorg labels merge conflicts when people push to their branch, but it
doesn't relabel PRs when there are updates to master. This would do
that.

The reason to have good merge conflict labeling is then you can search
and filter PRs. GitHub does not provide this feature itself,
unfortunately.

I do not know how technically and/or economically efficient this
action is.

I looked over the code for the action, it seems legit.

@zowoq
Copy link
Contributor

zowoq commented Sep 30, 2020

Looks like this checks the 100 most recent PRs so we might want to run this a couple of times a day.

@ryantm
Copy link
Member Author

ryantm commented Sep 30, 2020

Looks like this checks the 100 most recent PRs so we might want to run this a couple of times a day.

@zowoq I'm pretty sure it tries to fetch all the PRs, not just the most recent ones:
https://github.com/mschilde/auto-label-merge-conflicts/blob/2f64072651aa21e322975df6125041d5421a5864/lib/queries.ts#L78

@zowoq
Copy link
Contributor

zowoq commented Sep 30, 2020

Oops, TBH I stopped looking once I saw 100 as I assumed it would have a limit.

If this is going to check all 2000+ PRs it might use up a bit of the API quota.

@ryantm
Copy link
Member Author

ryantm commented Sep 30, 2020

@zowoq I think it will be really useful for people to filter PRs for review, so I consider solving this a pretty high priority. Do we have a way to pay GitHub for more API quota? I'm personall willing to pay the costs to see how much it costs, and if it isn't crazy continue supporting it.

Maybe it is also possible to run it on my personal server, so I'll look into that as a backup option.

@zowoq
Copy link
Contributor

zowoq commented Oct 1, 2020

https://docs.github.com/en/free-pro-team@latest/actions/reference/usage-limits-billing-and-administration#usage-limits

API requests - You can execute up to 1000 API requests in an hour across all actions within a repository. If exceeded, additional API calls will fail, which might cause jobs to fail.

https://docs.github.com/en/free-pro-team@latest/actions/hosting-your-own-runners/about-self-hosted-runners#usage-limits

Self-hosted seems to have the same limit but it it counted separately from github-hosted.

Looks the 1000 does include self-hosted.


Do we have a way to pay GitHub for more API quota?

No idea, sorry.

@doronbehar
Copy link
Contributor

@zowoq I think it will be really useful for people to filter PRs for review, so I consider solving this a pretty high priority.

@ryantm is the intention of this change to make reviewers know that there is a merge conflict, or that ther isn't? :) TBH I don't see how ofborg's labeling merge conflicts is not good enough. I'm not strictly against the idea, but considering our API quota, this will not use up ofborg's quota? I hope it won't.

However, I would have been delighted if some bot would have commented on PRs that there is a merge conflict, and hence notify subscribers of the issue, not only silently label there's a merge conflict.

@ryantm
Copy link
Member Author

ryantm commented Oct 1, 2020

@doronbehar Yes, that's the intention. Mainly so reviewers can filter out PRs that are in conflict when reviewing. OfBorg is not good enough because it doesn't update the labels when changes to master introduce the conflict. I think it would affect OfBorg's quota since they would both be workflows on the same project. That's a good point that it would be much more useful if it notifies people.

@doronbehar
Copy link
Contributor

Investigating a bit led me to: mschilde/auto-label-merge-conflicts#24 which points to this fork: https://github.com/Marr11317/ConflictAdviser . Note the Limitations.

OfBorg labels merge conflicts when people push to their branch, but it
doesn't relabel PRs when updates to master introduce the conflict.

The reason to have good merge conflict labeling is then you can search
and filter PRs. GitHub does not provide this feature itself,
unfortunately.

This took 1hr to run on nixpkgs, but it might be quicker depending on
how many merges to master are happening while it runs.
@ryantm
Copy link
Member Author

ryantm commented Oct 4, 2020

I figured out how to run this locally with act, fixed a bugs with it, and ran it on nixpkgs. It turns out about 1/3 of our open PRs are conflicted! It took about 57 minutes to run, a large part of that time was waiting for GitHub to have a merge status for each PR. Probably it will run faster during times when not many merges to master are happening.

I misunderstood earlier when @zowoq mentioned a quota. I thought you were talking about GitHub Actions quota, but it looks like GitHub Actions is free for public repositories. https://docs.github.com/en/free-pro-team@latest/github/setting-up-and-managing-billing-and-payments-on-github/about-billing-for-github-actions

If there is a problem with the API quota, it seems like we can slow down how often it makes requests. On my fork, I've already slowed down the merge status requests to happen once every minute instead of once every 5 seconds, and increased the retries from 5 to 60.

@ryantm ryantm requested a review from zowoq October 4, 2020 00:23
@zowoq
Copy link
Contributor

zowoq commented Oct 4, 2020

How many API calls per PR and how many PRs an hour?

Also this seems to cause the stale label to be removed when it adds the conflict label? Kind of makes the stale bot less useful for PRs.

Edit: I guess it would also be a race between the conflict and stale labels as the conflict label will reset the stale countdown.

@Mic92
Copy link
Member

Mic92 commented Oct 4, 2020

The stale bot ignores bots: https://github.com/probot/stale/blob/e3c53ebf879cccaf856fba1602fdd67f878c3ee3/index.js#L24
so we need to make sure the auto label bot is also marked as such.

@SuperSandro2000
Copy link
Member

For me this is very important because PRs with merge conflicts can't run with nixpkgs-review and realizing this after walking away is always a bummer.

@SuperSandro2000 SuperSandro2000 marked this pull request as draft November 28, 2020 00:13
@stale
Copy link

stale bot commented Jun 3, 2021

I marked this as stale due to inactivity. → More info

@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jun 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md 6.topic: policy discussion 10.rebuild-darwin: 0 10.rebuild-linux: 0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants