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

formulae: use strict audit #913

Closed
wants to merge 2 commits into from

Conversation

ZhongRuoyu
Copy link
Member

Since we are already requesting Homebrew/core contributors to use brew audit --strict (it's in the checklist), I believe our test bot should do the same.

This should help us, for instance, to notice the missing test block in Homebrew/homebrew-core#129122.

Since we are already requesting `Homebrew/core` contributors to use
`brew audit --strict` (it's in the checklist), I believe our test bot
should do the same.

This should help us, for instance, to notice the missing `test` block in
Homebrew/homebrew-core#129122.

Signed-off-by: Ruoyu Zhong <[email protected]>
@Bo98
Copy link
Member

Bo98 commented Apr 23, 2023

This is basically the reason --strict exists.

Maybe what test-bot should do is treat --strict audits as a warning and the rest as errors.

If there's anything in particular you think should be failing CI, maybe the particular audits you're thinking about could be promoted from strict audits to regular audits. This also has the benefit that it'll be covered in existing tap-wide audit checks to ensure we're not going to start failing hundreds of formulae.

There's also non-core taps to consider for some strict audits. To use the test block example: many run external CI on binaries and don't use formula test blocks, so I reckon that particular audit should be scoped to Homebrew/core accordingly.

@MikeMcQuaid
Copy link
Member

Maybe what test-bot should do is treat --strict audits as a warning and the rest as errors.

No, we've just removed "warnings" vs "errors" in brew audit because warnings are spammy and don't get fixed and because it's confusing to contributors which ones they have to fix.

If there's anything in particular you think should be failing CI, maybe the particular audits you're thinking about could be promoted from strict audits to regular audits.

Agreed.

There's also non-core taps to consider for some strict audits. To use the test block example: many run external CI on binaries and don't use formula test blocks, so I reckon that particular audit should be scoped to Homebrew/core accordingly.

Also agreed.

Copy link
Member

@MikeMcQuaid MikeMcQuaid left a comment

Choose a reason for hiding this comment

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

As comments: passing on this, we shouldn't do this, sorry! We could consider having a --strict option here that could be applied with a label in specific PRs but the discussed solutions seem better than that.

@ZhongRuoyu
Copy link
Member Author

OK, thanks @Bo98 @MikeMcQuaid -- let me revisit this later.

Yes, we can probably scope the test block audit to Homebrew/core and make it a regular one. But are we able to keep it as a strict audit in order not to cause regression? (Otherwise brew audit --strict will stop reporting missing test blocks for third-party taps.)

An alternative solution would be to add a --strict option to brew test-bot and enable it in Homebrew/core. I'm actually more inclined to this one because this is consistent with what we ask our contributors to do, will enable several other checks in Homebrew/core, and won't require us to add more exceptions for Homebrew/core.

@ZhongRuoyu ZhongRuoyu closed this Apr 24, 2023
@ZhongRuoyu ZhongRuoyu deleted the audit-strict branch April 24, 2023 09:34
@Bo98
Copy link
Member

Bo98 commented Apr 24, 2023

Yes, we can probably scope the test block audit to Homebrew/core and make it a regular one. But are we able to keep it as a strict audit in order not to cause regression? (Otherwise brew audit --strict will stop reporting missing test blocks for third-party taps.)

This is somewhat an existing issue. IMO we should have a better system that taps can opt-in to some core checks, but there's been strong disagreements to this before.

An alternative solution would be to add a --strict option to brew test-bot and enable it in Homebrew/core. I'm actually more inclined to this one because this is consistent with what we ask our contributors to do, will enable several other checks in Homebrew/core, and won't require us to add more exceptions for Homebrew/core.

Some strict audits are intentionally non-errors for Homebrew/core too. An example is go_resource deprecation. Strict audits are enabled for new formulae, so it blocks anything new using it. But existing usages are largely unfixable without upstream adding what we want: a go.lock file. We don't want to remove formulae outright because of it, and they are naturally being deprecated over time for not being maintained upstream anyway - almost all have by this point.

So I guess it's effective grandfathering for that stuff. All of the strict checks are in a way, for varying degrees of temporary timeframes.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 25, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants