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.

Sign up for GitHub

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

Jump to bottom

Allow "any" value in github_prerelease_allowlist #18488

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

Allow "any" value in github_prerelease_allowlist #18488

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions Library/Homebrew/utils/shared_audits.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,9 +62,9 @@ def self.github_release(user, repo, tag, formula: nil, cask: nil)
[cask.tap&.audit_exception(:github_prerelease_allowlist, cask.token), cask.token, cask.version]
end

return "#{tag} is a GitHub pre-release." if release["prerelease"] && [version, "all"].exclude?(exception)
return "#{tag} is a GitHub pre-release." if release["prerelease"] && [version, "all", "any"].exclude?(exception)

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 •
edited
Loading

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 •
edited
Loading

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 •
edited
Loading

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?

end

Expand Down
Loading