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

dev-cmd/bump*: limit the number of open PRs to 15. #16962

Merged
merged 2 commits into from Mar 29, 2024
Merged

Conversation

MikeMcQuaid
Copy link
Member

Don't let users open more than 15 PRs at a time. We have other tooling to nudge them to not do this but let's put it in the worst offenders: the bump* commands.

I'm taking two overlapping approaches here:

  • Check all open PRs the user has on GitHub. This is a single, non-search query and if they have fewer than 15 overall or in the @Homebrew org: they are good.
  • Check all open issues and PRs on the given tap created by the user and filter this down to just the PRs, failing if there are over 15.

This may have false-negatives if a user has huge numbers of e.g. issues and PRs but: this doesn't happen on official repos anyway.

Requesting a bunch of reviews as this may be controversial.

Don't let users open more than 15 PRs at a time. We have other tooling
to nudge them to not do this but let's put it in the worst offenders:
the `bump*` commands.
@MikeMcQuaid MikeMcQuaid requested review from a team March 28, 2024 11:57
Library/Homebrew/utils/github.rb Outdated Show resolved Hide resolved
Co-authored-by: Carlo Cabrera <[email protected]>
@fxcoudert
Copy link
Member

I'm not sure what problem we are trying to address exactly. I feel like right now we're in a good state for homebrew-core (and homebrew-cask doesn't look bad either, although I spend less time over there).

  • Are we trying to fight against people opening 20 PRs in one batch? If they're okay PRs, why do we want to limit that?
  • Are we trying to counter people keeping failing PRs open for a long time? Maybe it's best addressed with stale. Again, I don't think it's a problem right now, our number is stable and the most problematic PRs that are staying a long time are either complex issues, or new formulas with unresponsive authors. But not mass PR creation.

@MikeMcQuaid
Copy link
Member Author

I'm not sure what problem we are trying to address exactly.

This is continued from the AGM discussion that we should limit these PRs. I've seen multiple occasions where the warning messages are ignored and I'd like to adjust things programmatically so that doesn't happen.

  • Are we trying to fight against people opening 20 PRs in one batch? If they're okay PRs, why do we want to limit that?

Yes. That's too many for any of reviewers, authors or CI to keep on top of.

  • Are we trying to counter people keeping failing PRs open for a long time?

Yes but, more specifically, we're trying to prioritise completing existing work over starting new work.

Obviously there's a trivial workaround: create your bump without these tools. I think these tools are great but have encouraged a bit of a "fire and forget" mentality that's better prevented at source with quick API call than messaging after the fact.

@MikeMcQuaid
Copy link
Member Author

or CI to keep on top of

This is the main thing I don't see addressed with previous solutions, FYI.

@SMillerDev
Copy link
Member

Are we trying to fight against people opening 20 PRs in one batch? If they're okay PRs, why do we want to limit that?

One of the reasons: if those people are maintainers they are duplicating the work of a bot. If the bot did it, they could have reviewed and merged it. If they do it, it needs to wait for a second maintainer.

@fxcoudert
Copy link
Member

or CI to keep on top of

For that we have a queue, so I don't think it's too much of a problem (in practice).

if those people are maintainers they are duplicating the work of a bot. If the bot did it, they could have reviewed and merged it. If they do it, it needs to wait for a second maintainer

That is a really good argument I hadn't thought of! Thanks @SMillerDev

@Bo98
Copy link
Member

Bo98 commented Mar 28, 2024

Are we trying to fight against people opening 20 PRs in one batch? If they're okay PRs, why do we want to limit that?

One of the reasons: if those people are maintainers they are duplicating the work of a bot. If the bot did it, they could have reviewed and merged it. If they do it, it needs to wait for a second maintainer.

Don't we already have a restriction on that? Isn't all maintainers PRs not things that BrewTestBot bumps now?

@SMillerDev
Copy link
Member

Don't we already have a restriction on that? Isn't all maintainers PRs not things that BrewTestBot bumps now?

We do have a restriction on people bumping the same things, but there is also a chunk of software that the bot isn't updating but should. The goal is for humans to only do complicated things that the bot can't do.

@Bo98
Copy link
Member

Bo98 commented Mar 28, 2024

Things are being added to the autobump file multiple times a day: https://github.com/Homebrew/homebrew-core/commits/master/.github/autobump.txt. Often when identifying a version bump that the bot hasn't caught. A lot of manual version bump PRs are doubling up as PRs that are helping the bot going forward.

If we believe that there is an ongoing failure to add things to autobump, can we identify what they are and why?


Relatedly, this isn't the first time people have accused others of "point scoring" over the bot but it hasn't actually ever been fully explained what the root cause is or the impact that necessarily is having (including non-maintainer PRs).

@MikeMcQuaid
Copy link
Member Author

If we believe that there is an ongoing failure to add things to autobump, can we identify what they are and why?

We should do this, I agree, but that's orthogonal to the intent of this PR which is about programmatically enforcing what we have documented and as PR message requests in a lower part of our tooling stack (which is trivial to workaround if possible).

@SMillerDev
Copy link
Member

If we believe that there is an ongoing failure to add things to autobump, can we identify what they are and why?

I don't believe there is. But that isn't really related to this PR.

We do have a restriction on people bumping the same things, but there is also a chunk of software that the bot isn't updating but should.

I don't believe this to be intentional. But it is the reality and I think every nudge in the direction that we agreed on is worth doing.

Relatedly, this isn't the first time people have accused others of "point scoring" over the bot but it hasn't actually ever been fully explained what the root cause is or the impact that necessarily is having (including non-maintainer PRs).

I consider a long list of PRs created automatically by the same author to be a huge drain on my motivation since if it fails it'll likely never be fixed "unless I do it myself". There is simply no benefit I can think of in having a human account associated with bot actions.

Copy link
Member

@p-linnane p-linnane left a comment

Choose a reason for hiding this comment

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

I support the intent. Trying to think of a better message though.

@bevanjkay
Copy link
Member

There's one edge case in the cask-fonts repo. A font family that has more than 15 cask files 😅

We don't have autobump turned on there, so maybe that's the solution.

@MikeMcQuaid
Copy link
Member Author

Seems there's enough ✅ to try this out. Can revert if it causes major issues or iterate on the approach/messaging. Thanks all!

@MikeMcQuaid MikeMcQuaid merged commit ec74bad into master Mar 29, 2024
25 checks passed
@MikeMcQuaid MikeMcQuaid deleted the bump_15_prs branch March 29, 2024 08:16
@github-actions github-actions bot added the outdated PR was locked due to age label Apr 29, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 29, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants