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

resource_auditor: add audit for HEAD default branch #11720

Merged
merged 3 commits into from Aug 12, 2021

Conversation

nandahkrishna
Copy link
Member

  • 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?

This PR adds an audit to ensure that the correct default branch for a Git repository is specified for head when using --strict --online. For main and other alternatives, we need to explicitly specify the default branch using branch:, as the default is master.

Adding an audit will ensure this is checked in PRs to add new formulae.

I haven't written a test for this audit as it would require network access (and use a real repository URL), but I'd be happy to add one if that's preferable.

(CC @vladimyr)

@BrewTestBot
Copy link
Member

Review period will end on 2021-07-15 at 18:00:14 UTC.

@BrewTestBot BrewTestBot added waiting for feedback Merging is blocked until sufficient time has passed for review and removed waiting for feedback Merging is blocked until sufficient time has passed for review labels Jul 14, 2021
Library/Homebrew/resource_auditor.rb Outdated Show resolved Hide resolved
Library/Homebrew/resource_auditor.rb Outdated Show resolved Hide resolved
Library/Homebrew/resource_auditor.rb Outdated Show resolved Hide resolved
@nandahkrishna
Copy link
Member Author

nandahkrishna commented Jul 15, 2021

I realised that for some formulae we want the specified branch to not be the default branch – like versioned formulae where different versions have different development branches. I don't think this would be the case for too many formulae, so I think I'll add an allowlist for the audit, along with the suggested changes.

@BrewTestBot BrewTestBot removed the waiting for feedback Merging is blocked until sufficient time has passed for review label Jul 15, 2021
@BrewTestBot
Copy link
Member

Review period ended.

@Rylan12
Copy link
Member

Rylan12 commented Jul 16, 2021

I realised that for some formulae we want the specified branch to not be the default branch – like versioned formulae where different versions have different development branches. I don't think this would be the case for too many formulae, so I think I'll add an allowlist for the audit, along with the suggested changes.

Yeah, this is definitely a good idea to have. I'd say go ahead and continue with this route and if it turns out there are a ton of formulae on the list, we can change the audit to just be "you need to specify a branch name"

@Rylan12
Copy link
Member

Rylan12 commented Jul 16, 2021

Could this be done for stable, too?

Also, I'm tempted to not make this a normal audit over --strict. Would that be unreasonable because of the number of formulae that would need to be changed? What about making the audit is --strict if the default branch is master and not --strict otherwise (and then we can plan on removing the --strict in the future once more formulae are updated.

@nandahkrishna
Copy link
Member Author

nandahkrishna commented Jul 21, 2021

Could this be done for stable, too?

Stable URLs always specify a revision and mostly have a tag, so I don't know if this would be necessary. Hope there isn't some case I'm forgetting about here.

Would that be unreasonable because of the number of formulae that would need to be changed?

I haven't yet checked the number of formulae that need to be updated, but I thought keeping this as a strict audit would be better because:

  • Failing CI for an effectively syntax-only change seems undesirable. Those who use HEAD-installs and other interested contributors could volunteer to make the change in a separate syntax-only PR anyway. (I do plan to open a pull request to update the branches after this PR is merged.)
  • It will still be checked for brew audit --new, so new formulae have the correct default branch specified.

Seems like specifying the branch anyway (whether it's master or not) is better in the long run. I'll tweak this so that a branch must be specified, and add an allowlist which we would use when the default branch is intentionally not used for the formula (versioned formula with head and other cases).

@github-actions
Copy link

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@github-actions github-actions bot added the stale No recent activity label Aug 12, 2021
@nandahkrishna nandahkrishna removed the stale No recent activity label Aug 12, 2021
@nandahkrishna
Copy link
Member Author

Apologies for the late update, the latest commit makes specifying a branch mandatory for head and also adds an allowlist for specifying formulae where we intentionally use non-default branches. I tested this with some formulae:

  • sdl_mixer for an intentional non-default branch.
  • zsh-vi-mode to ensure that specifying a branch is mandatory.
  • Modifying the head branch to a formula and ensuring the correct default branch is reported by the audit.

The allowlist, located at audit_exceptions/head_non_default_branch_allowlist.json in a Tap would look like this:

{
    "sdl_mixer": "SDL-1.2",
    ...
}

Only one issue remains unsolved – head resources that specify non-default branches intentionally. I'll need to look for examples but here's a hypothetical one:

head do
  ...

  resource "example-resource" do
    url "https://github.com/example-resource/example-resource.git", branch: "example"
  end
end

At the moment, the audit would raise an error if example isn't the default branch. We'll have to extend the allowlist to account for such cases as well, but I'm not sure what a good format/schema would be, looking forward to your suggestions. Here's one potential format which looks complicated, but such formulae should be very rare:

{
    "sdl_mixer": "SDL-1.2",
    "example-formula": {
        "branch": "dev",
        "resources": {
            "example-resource": "example"
        }
    }
}

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.

Nice work! One tiny suggested tweak.

Library/Homebrew/resource_auditor.rb Outdated Show resolved Hide resolved
Copy link
Member

@Rylan12 Rylan12 left a comment

Choose a reason for hiding this comment

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

but such formulae should be very rare

I think it's fine to just figure that out if we encounter it, then.

Thanks, @nandahkrishna!

Co-authored-by: Mike McQuaid <mike@mikemcquaid.com>
@nandahkrishna nandahkrishna merged commit afc5804 into Homebrew:master Aug 12, 2021
@nandahkrishna nandahkrishna deleted the formula-branch-audit branch August 12, 2021 14:28
@vladimyr
Copy link
Contributor

@nandahkrishna Just for the record I still feel bad sending you down the rabbit hole. I really didn't mean to and I honestly wish I was able to help you.
Here goes a big thank you to @nandahkrishna and the rest of the team! Great job, as usual! ❤️

@nandahkrishna
Copy link
Member Author

No problem at all @vladimyr, happy to have worked on this 😄.

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

5 participants