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

Migrate remaining audit exceptions to homebrew/core #9314

Merged
merged 8 commits into from Nov 30, 2020

Conversation

Rylan12
Copy link
Member

@Rylan12 Rylan12 commented Nov 26, 2020

  • 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 tests with your changes locally?
  • Have you successfully run brew man locally and committed any changes?

This PR migrates the remaining audit exceptions to homebrew/core.

This is pretty straight forward, on this end, but there will be a few additional notes in the corresponding homebrew/core PR: Homebrew/homebrew-core#65712

Note that it is expected that CI will fail until Homebrew/homebrew-core#65712 has been merged into homebrew/core and linuxbrew/core.

@BrewTestBot
Copy link
Member

Review period will end on 2020-11-27 at 20:40:14 UTC.

@BrewTestBot BrewTestBot added the waiting for feedback Merging is blocked until sufficient time has passed for review label Nov 26, 2020
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.

Great work here @Rylan12. I've been impressed not just with your work on this but how you've handled the incremental rollout 👏🏻

Library/Homebrew/test/dev-cmd/audit_spec.rb Show resolved Hide resolved
Library/Homebrew/formula_auditor.rb Show resolved Hide resolved
Library/Homebrew/formula_auditor.rb Outdated Show resolved Hide resolved
@Rylan12
Copy link
Member Author

Rylan12 commented Nov 27, 2020

Thanks, @MikeMcQuaid! It means a lot and I appreciate the kind words!

It sounds like there are no major concerns, so I think I'll go ahead and merge the homebrew/core PR (after making the changes discussed above) to get the ball rolling as it will need to get through linuxbrew/core as well.

@MikeMcQuaid
Copy link
Member

Another nice-to-have here eventually (while I remember) would be having some of these checks fail if they are no longer needed to encourage their removal (like RuboCop does with disables/todos). This would ensure that these lists get pruned when they aren't needed rather than growing with stale things.

This should not block this PR!

@Rylan12
Copy link
Member Author

Rylan12 commented Nov 27, 2020

Another nice-to-have here eventually (while I remember) would be having some of these checks fail if they are no longer needed to encourage their removal (like RuboCop does with disables/todos). This would ensure that these lists get pruned when they aren't needed rather than growing with stale things.

Definitely agree. I'll have to think of how we want to do this. As one idea, we could add a --prune-exceptions option to audit which, if passed, could run all checks and compare audit failures with the exceptions. I'll have to think more about the specifics so this will come later.

@MikeMcQuaid
Copy link
Member

Definitely agree. I'll have to think of how we want to do this. As one idea, we could add a --prune-exceptions option to audit which, if passed, could run all checks and compare audit failures with the exceptions. I'll have to think more about the specifics so this will come later.

👍🏻 on deciding later. I'd say in general when the audits are checked for validity that they could be quickly scanned to ensure that they all fail and, if not, complain. I think this could be default behaviour because it's pretty much always going to be desirable behaviour.

@BrewTestBot BrewTestBot removed the waiting for feedback Merging is blocked until sufficient time has passed for review label Nov 27, 2020
@BrewTestBot
Copy link
Member

Review period ended.

Rylan12 added a commit to Rylan12/linuxbrew-core that referenced this pull request Nov 29, 2020
@Rylan12
Copy link
Member Author

Rylan12 commented Nov 29, 2020

Just waiting on https://github.com/Homebrew/linuxbrew-core/pull/21668

Edit: and... one more: https://github.com/Homebrew/linuxbrew-core/pull/21670

Should be good for real after this (I hope 😅)

@jonchang
Copy link
Contributor

jonchang commented Nov 30, 2020

Homebrew/linuxbrew-core#21670 is merged. I've restarted the CI jobs.

@Rylan12
Copy link
Member Author

Rylan12 commented Nov 30, 2020

Thanks, @jonchang!

@Rylan12 Rylan12 merged commit 62df715 into Homebrew:master Nov 30, 2020
@Rylan12 Rylan12 deleted the migrate-audit-exceptions branch November 30, 2020 05:10
@BrewTestBot BrewTestBot added the outdated PR was locked due to age label Dec 31, 2020
@Homebrew Homebrew locked as resolved and limited conversation to collaborators Dec 31, 2020
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

4 participants