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
CurlGitHubPackagesDownloadStrategy: Fix third-party taps #11025
Conversation
Review period will end on 2021-04-06 at 00:00:00 UTC. |
Review period skipped due to |
87b2fdd
to
00c510b
Compare
Fix CurlGitHubPackagesDownloadStrategy for third party taps.
00c510b
to
a28b419
Compare
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.
Some cleanup requests.
@@ -306,9 +306,7 @@ def initialize(formula, spec) | |||
|
|||
filename = Filename.create(formula, tag, spec.rebuild).bintray | |||
|
|||
# TODO: this will need adjusted when if we use GitHub Packages by default |
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.
Please re-add this comment.
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.
How does it need to be adjusted when we use GitHub Contain Registry by default in Homebrew/core?
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.
All the Bintray logic will need to be removed and this will need to be the default path.
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.
path, resolved_basename = if spec.root_url.match?(GitHubPackages::URL_REGEX)
["#{@name}/blobs/sha256:#{checksum}", filename]
else
filename
end
Logic specific to Bintray will need to be removed, but this particular code is not specific to Bintray. It will need to remain as is to support third-party tap that use a root_url
to store files on a normal non-GHCR HTTPS server.
# TODO: this will need adjusted when if we use GitHub Packages by default | ||
path, resolved_basename = if (bottle_domain = Homebrew::EnvConfig.bottle_domain.presence) && | ||
bottle_domain.start_with?(GitHubPackages::URL_PREFIX) | ||
path, resolved_basename = if spec.root_url.match?(GitHubPackages::URL_REGEX) |
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.
Why match?
instead of start_with?
The latter is much faster.
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.
To permit root_url "docker://ghcr.io/brewsci/bio"
GitHubPackages.root_url(tap.user, tap.repo).to_s | ||
else | ||
"#{Homebrew::EnvConfig.bottle_domain}/#{Utils::Bottles::Bintray.repository(tap)}" | ||
end | ||
else | ||
@root_url = var | ||
@root_url = if var.to_s.start_with? "docker://" |
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.
The root_url
should never start with docker://
. When is it set to that?
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.
In Brewsci/bio I set root_url "docker://ghcr.io/brewsci/bio"
. It also supports other Docker / OCI / ORAS container registries such as docker://hub.docker.com
and docker://quay.io
.
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.
@sjackman Ok. Can this logic get moved to github_packages.rb
? Ideally this would just be part of the root_url
method.
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'm not entirely sure that I followed, but I took a stab at it in PR #11037
Fix CurlGitHubPackagesDownloadStrategy for third party taps.
brew style
with your changes locally?brew typecheck
with your changes locally?brew tests
with your changes locally?