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 license mismatch allowlist to Homebrew/core #9178

Merged
merged 3 commits into from Nov 20, 2020

Conversation

Rylan12
Copy link
Member

@Rylan12 Rylan12 commented Nov 18, 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? (I've run the audit tests only)
  • Have you successfully run brew man locally and committed any changes? (no changes)

This PR migrates the PERMITTED_FORMULA_LICENSE_MISMATCHES audit allowlist to Homebrew/core.

Additionally, I added some more tests for the lists migrated in #9039

Prompted by Homebrew/homebrew-core#59828

Corresponding Homebrew/core PR (should be merged at the same time or before): Homebrew/homebrew-core#65061

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.

👍🏻 to merge after typo resolved.

@Rylan12
Copy link
Member Author

Rylan12 commented Nov 18, 2020

Thanks, both!

I'm going to spend some time reworking the logic here now that #9187 has been merged. I'm planning on extracting the logic that handles the lists to a new location so it can be used by other modules as well (e.g. PyPI for #9185).

Thoughts on:

  1. Renaming audit_exceptions to e.g. tap_lists for formula_lists
  2. The location for this code. I'm leaning toward Library/Homebrew/tap_lists.rb (or formula_lists, depending on number 1) unless another location makes more sense.

@MikeMcQuaid
Copy link
Member

  • Renaming audit_exceptions to e.g. tap_lists for formula_lists

I prefer keeping audit_exceptions for the stuff related to audits and having different names/folders for things used for different tools. A more generic solution may mean less code but it makes the contributor/user workflow for figuring out what these files are for a little harder.

@Rylan12
Copy link
Member Author

Rylan12 commented Nov 18, 2020

Okay, that's fair. How about going with directories that end in *_lists/. So rename the audit lists to audit_exception_lists/ and then we can add others like pypi_lists/?

I'm trying to reduce the number of hardcoded files so adding lists in the future is pretty easy.

@Rylan12 Rylan12 force-pushed the migrate-license-mismatch-allowlist branch from 4641cd5 to 1bb2d7c Compare November 19, 2020 00:12
@Rylan12
Copy link
Member Author

Rylan12 commented Nov 19, 2020

Okay, I've reworked the handling of lists a little bit.

Now, all directories that end in *_lists will be parsed and can be accessed via tap.formula_lists.

There are several two PRs as part of this migration. Here is the order in which they should be merged to avoid breaking CI:

  1. Merge Migrate license mismatch allowlist to Homebrew/core homebrew-core#65061 to add all necessary lists to the audit_exceptions_lists/ directory
  2. Wait for Migrate license mismatch allowlist to Homebrew/core homebrew-core#65061 to be merged into linuxbrew-core
  3. Rerun CI on this PR and merge once CI is green
  4. Merge remove unused audit_exceptions directory homebrew-core#65159 to remove the (what will be) unused audit_exceptions/ directory.

@Rylan12 Rylan12 changed the title Migrate license mismatch allowlist to Homebrew/core Update formula lists and migrate license mismatch allowlist Nov 19, 2020
@MikeMcQuaid
Copy link
Member

Now, all directories that end in *_lists will be parsed and can be accessed via tap.formula_lists.

That's a bit too magic for my liking. audit_exceptions/ feels like a more appropriate name, too.

Okay, that's fair. How about going with directories that end in *_lists/. So rename the audit lists to audit_exception_lists/ and then we can add others like pypi_lists/?
I'm trying to reduce the number of hardcoded files so adding lists in the future is pretty easy.

Do we anticipate having more than one list for PyPI? If not: YAGNI.

Sorry to keep jerking you around here. I'm happy for you to not make any code changes until we're agree on the implementation (if that works for you)? I don't mind you making changes back and forth but I don't want to waste your time (if you feel like I am).

@Rylan12
Copy link
Member Author

Rylan12 commented Nov 19, 2020

That's a bit too magic for my liking. audit_exceptions/ feels like a more appropriate name, too.

Okay, I think that's fair. Right now there are some constants in tap.rb so I think this would be a good place to hardcode which files we're going to allow. I was trying to make things work nicely, especially for third-party taps, but I'm not really sure it matters because the brew code will need to be updated anyway if new lists are going to be added.

Do we anticipate having more than one list for PyPI? If not: YAGNI.

Probably not so fair point.

Sorry to keep jerking you around here. I'm happy for you to not make any code changes until we're agree on the implementation (if that works for you)? I don't mind you making changes back and forth but I don't want to waste your time (if you feel like I am).

No worries! Not a waste of time. Timezones are a little tricky here because most of my free time is after the European maintainers go to bed, it seems, so I'm just trying to have workable solutions in place for the morning 😄


So, moving forward, are we thinking:

  • We'll hardcode in tap.rb which files are the list files.
  • We'll continue to use audit_exceptions/ for the audit exception lists

So then, my question:

  • Where will files that don't belong in the audit exceptions category live? The tap's root directory? A formula_lists/ directory? Right now we only have one that I'm in the process of moving for pypi resources, but yesterday I saw another list (I believe relating to livecheck that won't fit under the audit_exceptions title. Maybe I need to do some more investigation into what lists we have that will be migrated so I can get a better sense of how many non-audit-exception-lists we're talking about.

@MikeMcQuaid
Copy link
Member

but I'm not really sure it matters because the brew code will need to be updated anyway if new lists are going to be added.

Agreed 👍🏻

  • I believe relating to livecheck that won't fit under the audit_exceptions title. Maybe I need to do some more investigation into what lists we have that will be migrated so I can get a better sense of how many non-audit-exception-lists we're talking about.

Yeh, I think this is probably required before we can figure out whether it's best to have one-or-more new directories or one-or-more new files in the repo root.

  • We'll hardcode in tap.rb which files are the list files.
  • We'll continue to use audit_exceptions/ for the audit exception lists

Yup 👍🏻

@MikeMcQuaid
Copy link
Member

Do we anticipate having more than one list for PyPI? If not: YAGNI.

Probably not so fair point.

Sorry, missed this bit. I'm more pointing to the principle here; I think it's easy enough to change these when we need them more rather than pre-planning things just in case we need them in future.

@Rylan12
Copy link
Member Author

Rylan12 commented Nov 19, 2020

I should have some time in the next couple of hours to look into this a little more. I'll try to find all of the non-audit lists that will need to be migrated eventually and then, maybe, we can make a better decision (I'll need to find these lists in the future, anyway, so may as well do it now).

@MikeMcQuaid
Copy link
Member

Thanks again @Rylan12, you're doing great work ❤️

@Rylan12
Copy link
Member Author

Rylan12 commented Nov 19, 2020

Okay, I've looked through and found lists that should be migrated in the following categories:

  • audit lists
    • We will keep using audit_exceptions/
  • rubocop lists
    • We can either add these to the existing audit_exceptions/ directory or create a new rubocop_exceptions/ or style_exceptions/ directory. I'm leaning toward style_exceptions/ because it matches with the command (brew style not brew rubocop) so it's clearer to anyone who doesn't know about rubocop or the inner workings of brew style.
  • other lists
    • The Livecheck module's GITHUB_SPECIAL_CASES
    • The PyPI module's AUTOMATIC_RESOURCE_UPDATE_BLOCKLIST
    • We can either add these as individual files in the tap's root directory or can nest them under an e.g. formula_lists/ directory. I'm leaning toward the second option because I think it is cleaner and adding more lists in the future won't increase clutter.

What I'm envisioning is that we can use tap.audit_exceptions for audit exceptions, tap.style_exceptions to access style exceptions, and tap.formula_lists to access the lists. Each of these will contain a hash containing all of the lists in their directory (like currently exists with audit_exceptions). For example, tap.formula_lists may return:

{
  pypi_automatic_resource_update_list: {
    "foo" => false,
    "bar" => "bar[abc]",
    "baz" => {
      "package_name" => "baz[xyz]"
      "extra_packages" => ["a", "b", "c"],
      "exclude_packages" => ["x", "y", "z"]
    }
  },
  livecheck_github_special_cases: [
    "foo",
    "bar",
    "baz"
  ]
}

(the contents are just an example)


However, none of the questions listed above will affect this PR, so I'm going to push forward with this (modifying to keep the audit directory in audit_exceptions/)

Rylan12 and others added 3 commits November 19, 2020 13:38
Co-authored-by: Mike McQuaid <mike@mikemcquaid.com>
Co-authored-by: Markus Reiter <me@reitermark.us>
@Rylan12 Rylan12 force-pushed the migrate-license-mismatch-allowlist branch from 1bb2d7c to 67e4e78 Compare November 19, 2020 18:40
@Rylan12 Rylan12 changed the title Update formula lists and migrate license mismatch allowlist Migrate license mismatch allowlist to Homebrew/core Nov 19, 2020
@MikeMcQuaid
Copy link
Member

  • We will keep using audit_exceptions/

👍🏻

  • We can either add these to the existing audit_exceptions/ directory or create a new rubocop_exceptions/ or style_exceptions/ directory. I'm leaning toward style_exceptions/ because it matches with the command (brew style not brew rubocop) so it's clearer to anyone who doesn't know about rubocop or the inner workings of brew style.

👍🏻 to either audit_exceptions or style_exceptions.

  • We can either add these as individual files in the tap's root directory or can nest them under an e.g. formula_lists/ directory. I'm leaning toward the second option because I think it is cleaner and adding more lists in the future won't increase clutter.

Given the above: I'd be tempted to put them in either files or directories relating to the command(s) that will run them.

For example, tap.formula_lists may return:

👍🏻

However, none of the questions listed above will affect this PR, so I'm going to push forward with this (modifying to keep the audit directory in audit_exceptions/)

👍🏻

@Rylan12
Copy link
Member Author

Rylan12 commented Nov 20, 2020

  • We can either add these as individual files in the tap's root directory or can nest them under an e.g. formula_lists/ directory. I'm leaning toward the second option because I think it is cleaner and adding more lists in the future won't increase clutter.

Given the above: I'd be tempted to put them in either files or directories relating to the command(s) that will run them.

Okay. May as well discuss this in #9185. For now, is this PR good to go? If so, I'll start the merge. I don't think we'll need to wait for linuxbrew-core merges as this change isn't breaking CI.

@Rylan12 Rylan12 merged commit 5a14be6 into Homebrew:master Nov 20, 2020
@Rylan12 Rylan12 deleted the migrate-license-mismatch-allowlist branch November 20, 2020 14:40
@BrewTestBot BrewTestBot added the outdated PR was locked due to age label Dec 21, 2020
@Homebrew Homebrew locked as resolved and limited conversation to collaborators Dec 21, 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