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
cask/audit: allow the homepage https audit to have exceptions #12185
cask/audit: allow the homepage https audit to have exceptions #12185
Conversation
Review period will end on 2021-10-05 at 23:30:17 UTC. |
d3c121a
to
46797a1
Compare
The latest commit also refactors the |
Code looks good to me! Might just want to name it something like |
Review period ended. |
Thanks, good feedback. I haven't had free time to implement this yet and might not until the weekend, so apologies for the delay. |
Okay, I looked at this again but wasn't able to get the tests to work. The problem is that I've moved the logic that handles the exceptions to a I tried a handful of things to trick the tests into thinking it has a tap so as not to mass-change a ton of tests but I think the solution is to change the logic to ensure that even these formulae have a tap associated (maybe I'm on break this week (after Tuesday) so hopefully, I'll have some more time. I've found that I consistently have a lot less free time during the week than I expect so progress here has been a bit slow. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is fine without adding tests FWIW
The problem is that it breaks existing tests (but because the tests no longer set the formula being tested up correctly rather than because this PR breaks functionality) |
Ah gotcha 😭 |
78923d8
to
9ca0f68
Compare
described_class.new(Formulary.factory(path), options) | ||
formula = Formulary.factory(path) | ||
|
||
if options.key? :tap_audit_exceptions | ||
tap = Tap.fetch("test/tap") | ||
allow(tap).to receive(:audit_exceptions).and_return(options[:tap_audit_exceptions]) | ||
allow(formula).to receive(:tap).and_return(tap) | ||
options.delete :tap_audit_exceptions | ||
end | ||
|
||
described_class.new(formula, options) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got the tests working!
For all of the formula_auditor
tests, I've stubbed Formula#tap
to return a test tap (test/tap
) that contains the necessary exceptions.
brew style
with your changes locally?brew typecheck
with your changes locally?brew tests
with your changes locally?This PR adds a
cert_error_allowlist
audit exception for the audit that checks HTTPS availability for cask homepages.This list is designed to be used to allow audits to pass for the formulae affected by the Cloudflare issue we've been having.
CC: @bevanjkay and @GregBrimble
Corresponding homebrew/cask PR: Homebrew/homebrew-cask#112093 (can be merged in either order)