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

Allow "any" value in github_prerelease_allowlist #18488

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

MTCoster
Copy link

@MTCoster MTCoster commented Oct 3, 2024

  • 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.
    • test/livecheck/strategy/github_releases_spec.rb seems like the best place to add a test for this, but there isn't currently anything to cover the github_prerelease_allowlist (that I can see).
  • 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 is required for Homebrew/homebrew-cask#187392. Unfortunately, the vendor there unreliably uses GitHub releases for binary distribution and doesn't consistently set Stable/Pre-release tags.

The change here allows the use of "any" instead of "all" in the github_prerelease_allowlist to effectively suppress the "not a pre-release" audit failure.


if !release["prerelease"] && exception
if !release["prerelease"] && exception && exception != "any"
return "#{tag} is not a GitHub pre-release but '#{name}' is in the GitHub prerelease allowlist."
Copy link
Member

Choose a reason for hiding this comment

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

It seems weird to insist that the version must be a prerelease if used with all. Would getting rid of this produce wrong versions for the casks using it?

Copy link
Author

Choose a reason for hiding this comment

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

I agree, it was unexpected to see the audit error when a stable release appeared. Would you prefer I just amend the behaviour of all rather than adding a new keyword?

Copy link
Member

@carlocab carlocab Oct 4, 2024

Choose a reason for hiding this comment

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

Probably, as long as it works fine for the formulae and casks that currently use it. There are some in Homebrew/core too: https://github.com/Homebrew/homebrew-core/blob/6ad4b3a769dd0f4f1b36e782d0efe558c64e15ec/audit_exceptions/github_prerelease_allowlist.json#L5-L8

Copy link
Member

Choose a reason for hiding this comment

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

@MTCoster Yes, please do 👍🏻

Copy link
Member

@reitermarkus reitermarkus Oct 4, 2024

Choose a reason for hiding this comment

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

It's mainly an error so we don't forget to remove it here once there is a stable release, i.e. once it is stable, it should stay stable.

Copy link
Member

@samford samford Oct 4, 2024

Choose a reason for hiding this comment

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

If I understand correctly, it sounds like there may be a need for an explicit until-stable value, that would allow a package to use pre-release versions until a stable release appears. That would allow us to make the all behavior predictable, while continuing to handle the aforementioned situation with a new option.

The linked cask PR is a different situation, as upstream unpredictably marks releases using a stable version format as "pre-release" on GitHub, even when their first-party download page links to a GitHub release asset for a "pre-release" version as stable. In that situation we have to either use all or use a specific version and accept that almost every version bump will require updating the allowlist to pass CI (increasing maintainer/contributor burden).

Copy link
Author

Choose a reason for hiding this comment

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

This was my original intent with splitting the behaviour of all and any where all required all releases to be pre-releases (the current behaviour) while any allowed any releases to be pre-releases.

Copy link
Member

Choose a reason for hiding this comment

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

It's mainly an error so we don't forget to remove it here once there is a stable release, i.e. once it is stable, it should stay stable.

If that's the intent then maybe it would be better to specify versions for those instead of making them all.

Copy link
Author

Choose a reason for hiding this comment

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

Admittedly this is more complicated and beyond the original scope of this change, but perhaps a range check like <1.2.0 would be better so you can specify "allow any pre-release up to the stable release 1.2.0"

Copy link
Member

Choose a reason for hiding this comment

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

If that's the intent then maybe it would be better to specify versions for those instead of making them all.

Depends on what the more likely case is: I'd guess the case that only pre-releases are published until a stable release is, and not to randomly change between stable/prerelease versions.

The whole point of adding all in the first place was to reduce the need to update the allow list with every version bump.

perhaps a range check

Likely a non-starter because app developers will use pretty much any versioning-scheme except semver. What if 1.1.27 is a pre-release, 1.1.28 is stable, and 1.1.29 is a pre-release again?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants