-
-
Notifications
You must be signed in to change notification settings - Fork 9.9k
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
Conversation
Signed-off-by: Matt Coster <[email protected]>
|
||
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." |
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.
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?
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 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?
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.
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
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.
@MTCoster Yes, please do 👍🏻
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.
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.
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.
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).
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.
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.
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.
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
.
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.
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"
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.
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?
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. |
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 thegithub_prerelease_allowlist
(that I can see).brew style
with your changes locally?brew typecheck
with your changes locally?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 thegithub_prerelease_allowlist
to effectively suppress the "not a pre-release" audit failure.