formula_auditor: allow references to merged PRs

[carlocab]

• Have you followed the guidelines in our Contributing document?
• Have you checked to ensure there aren't other open Pull Requests for the same change?
• Have you added an explanation of what your changes do and why you'd like us to include them?
• Have you written new tests for your changes? Here's an example.
• Have you successfully run brew style with your changes locally?
• Have you successfully run brew typecheck with your changes locally?
• Have you successfully run brew tests with your changes locally?

---

The github_issue_comment audit disallows references to closed or
merged PRs in other repositories. We should allow those, since it is a
common pattern to reference merged PRs in formulae when adding comments
that explain changes that need to be made in future versions (e.g.
patch blocks).

[BrewTestBot]

Review period will end on 2023-02-13 at 14:05:21 UTC.

[SMillerDev]

I don't think that's a great solution. It still allows things like the PHP formula where the pull request was declined but we still ship the patch forever. I think some keyword like fixed in: url will make sure that we can document the rationale without losing track of the state of our references.

[carlocab]

We can't really prevent fixed in: url pointing to a pull request that was declined either, though.

[carlocab]

We should also find a way to allow comments like this: https://github.com/Homebrew/homebrew-core/blob/6adaa9491a02938ef8ce18c7139d662a1e949108/Formula/terraform.rb#L37-L39

[SMillerDev]

For that one we could reference our internal PR. I understand the point though and I'm trying to come up with a solution for it.

[carlocab]

Seems kinda tedious to reference an internal PR that one would have to read through to get to the correct upstream PR which contains the context you're looking for.

Not to mention that it would require modifying your PR after it's already been opened (since you don't know the PR number until you open it).

This doesn't seem like a good solution at all, sadly.

[MikeMcQuaid]

It still allows things like the PHP formula where the pull request was declined but we still ship the patch forever.

Can we allow referencing merged but not closed PRs, then?

[BrewTestBot]

Review period ended.

[SMillerDev]

I like that suggestion. It'll cover the cases I'm most worried about.

[carlocab]

It's an improvement, but it doesn't allow references to issues upstream that either 1) were closed by being fixed in a formula, or 2) contain the context for why a formula is written a certain way.

If you're concerned about PRs that were rejected upstream: why not disallow references to PRs that were closed but not merged? What are examples of closed issues (that are not PRs) that we don't want to be able to reference in a formula?

[SMillerDev]

I can see HDFGroup/hdf5#2422 becoming such a situation since we have an idea of a workaround and we have an upstream issue. Nothing else currently comes to mind.

For the terraform example I think we should suggest this to the upstream build instructions though, since we wouldn't be the only build system affected by this.

[SMillerDev]

Alternative solution, do we have a way to make this into a warning only?

[carlocab]

Yea, think so. You can make it require --strict and then add a --strict audit in test-bot that allows failures (they become warnings instead).

[MikeMcQuaid]

Thanks @carlocab!