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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for private registry #11766

Merged
merged 4 commits into from Jul 27, 2021
Merged

Conversation

yahavi
Copy link

@yahavi yahavi commented Jul 25, 2021

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

Supersedes #11339
Partially addresses #11287

Allow using a private registry mirroring the common URL for blobs and bottles (https://ghcr.io/v2/homebrew/core). This feature worked before when the Homebrew packages registry was Bintray and this PR makes sure it works as before.

Usage example with Artifactory:

  1. Create a remote Docker repository in Artifactory proxies https://ghcr.io. In my example, I named it brew-remote.
  2. Populate HOMEBREW_DOCKER_REGISTRY_TOKEN environment variable with Artifactory access token and HOMEBREW_ARTIFACT_DOMAIN with a URL to Artifactory repository:
export HOMEBREW_DOCKER_REGISTRY_TOKEN=<artifactory-access-token>
export HOMEBREW_ARTIFACT_DOMAIN=http://127.0.0.1:8081/artifactory/brew-remote
  1. Run brew install:
brew install jfrog-cli

==> Downloading https://ghcr.io/v2/homebrew/core/jfrog-cli/manifests/2.0.1
==> Downloading from http://127.0.0.1:8081/artifactory/brew-remote/v2/homebrew/core/jfrog-cli/manifests/2.0.1
######################################################################## 100.0%
==> Downloading https://ghcr.io/v2/homebrew/core/jfrog-cli/blobs/sha256:484e41483de267892b3b87aceb84d5040cd66df4db4ced30de419f8e32011a84
==> Downloading from http://127.0.0.1:8081/artifactory/brew-remote/v2/homebrew/core/jfrog-cli/blobs/sha256:484e41483de267892b3b87aceb84d5040cd66df4db4ced30de419f8e32011a84
######################################################################## 100.0%
==> Pouring jfrog-cli--2.0.1.big_sur.bottle.tar.gz
==> Caveats
zsh completions have been installed to:
/Users/yahavi/code/brew/share/zsh/site-functions
==> Summary
馃嵑 /Users/yahavi/code/brew/Cellar/jfrog-cli/2.0.1: 7 files, 19.2MB

@jkenn99
Copy link

jkenn99 commented Jul 25, 2021

How would this work if the private registry doesn't require authentication?
I tried implementing fetching a valid token in #11339.

@yahavi
Copy link
Author

yahavi commented Jul 26, 2021

@jkenn99
Thanks for your feedback! I added another commit to allow anonymous access for private registries.

@MikeMcQuaid
Could you please let me know if you like the design? I can add tests to this PR after your code review.

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.

A few minor comments but I really like this approach. I like how non-invasive the approach is. Great work 馃憦馃徎

@@ -449,7 +449,7 @@ def resolve_url_basename_time_file_size(url, timeout: nil)
return @resolved_info_cache[url] if @resolved_info_cache.include?(url)

if (domain = Homebrew::EnvConfig.artifact_domain)
url = url.sub(%r{^((ht|f)tps?://)?}, "#{domain.chomp("/")}/")
url = url.sub(%r{^((ht|f)tps?://ghcr.io/)?}, "#{domain.chomp("/")}/")
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
url = url.sub(%r{^((ht|f)tps?://ghcr.io/)?}, "#{domain.chomp("/")}/")
url = url.sub(%r{^(https?://#{GitHubPackages::URL_DOMAIN}/)?}, "#{domain.chomp("/")}/")

Copy link
Author

Choose a reason for hiding this comment

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

Agree, with 2 additional changes:

  1. Added the o flag to handle RuboCop error: https://msp-greg.github.io/rubocop-performance/RuboCop/Cop/Performance/ConstantRegexp.html
  2. URL_DOMAIN constant became public

Library/Homebrew/download_strategy.rb Outdated Show resolved Hide resolved
Library/Homebrew/download_strategy.rb Outdated Show resolved Hide resolved
Library/Homebrew/env_config.rb Outdated Show resolved Hide resolved
Library/Homebrew/env_config.rb Outdated Show resolved Hide resolved
@MikeMcQuaid
Copy link
Member

If the private registry doesn't require authentication this still won't work since it doesn't request a valid token. Please take a look at the changes in #11339.

@jkenn99 Please try to be a little more positive on this PR given that someone else seems likely to implement the functionality that you weren't willing to push to completion. Thanks 鉂わ笍

@jkenn99
Copy link

jkenn99 commented Jul 26, 2021

@jkenn99 Please try to be a little more positive on this PR given that someone else seems likely to implement the functionality that you weren't willing to push to completion. Thanks 鉂わ笍

Good call, my bad.

@jkenn99
Copy link

jkenn99 commented Jul 26, 2021

@jkenn99

Thanks for your feedback! I added another commit to allow anonymous access for private registries.

@yahavi Thanks for taking a look at it. Is there any minimum Artifactory version needed to support this?

@MikeMcQuaid
Copy link
Member

Good call, my bad.

@jkenn99 Thanks for understanding 馃挅

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.

This looks good to me! @yahavi can you confirm it all still works for you after the most recent changes and I think I'm good to merge.

Library/Homebrew/download_strategy.rb Outdated Show resolved Hide resolved
@yahavi
Copy link
Author

yahavi commented Jul 27, 2021

@jkenn99 - It works for me on Artifactory 6.9 and above.

@MikeMcQuaid - I confirm. Please merge.

@MikeMcQuaid MikeMcQuaid merged commit 250233a into Homebrew:master Jul 27, 2021
@MikeMcQuaid
Copy link
Member

Thanks so much for your first contribution! Without people like you submitting PRs we couldn't run this project. You rock, @yahavi!

@yahavi yahavi deleted the private-registry branch July 27, 2021 12:54
@zie-ent
Copy link

zie-ent commented Jul 27, 2021

I just followed this PR - but thank all of you.
Excited to test this, once you guys release a new version - maybe later today ? 馃 聽

@yahavi
Copy link
Author

yahavi commented Jul 27, 2021

@MikeMcQuaid thanks for merging this PR and for your kind words :)

@SMillerDev
Copy link
Member

It seems like this merge has broken core CI, as noticed by @alebcay :

https://github.com/Homebrew/homebrew-core/pull/81954/checks?check_run_id=3171769122
https://github.com/Homebrew/homebrew-core/runs/3171780579?check_suite_focus=true

Error: undefined method `delete_if' for nil:NilClass
/opt/homebrew/Library/Homebrew/download_strategy.rb:394:in `fetch'
/opt/homebrew/Library/Homebrew/resource.rb:142:in `fetch'
/System/Library/Frameworks/Ruby.framework/Versions/2.6/usr/lib/ruby/2.6.0/forwardable.rb:230:in `fetch'
/opt/homebrew/Library/Homebrew/formula.rb:2016:in `fetch'
/opt/homebrew/Library/Homebrew/formula_installer.rb:1133:in `fetch'
/opt/homebrew/Library/Homebrew/install.rb:299:in `install_formula'
/opt/homebrew/Library/Homebrew/cmd/install.rb:207:in `block in install'
/opt/homebrew/Library/Homebrew/cmd/install.rb:205:in `each'
/opt/homebrew/Library/Homebrew/cmd/install.rb:205:in `install'
/opt/homebrew/Library/Homebrew/brew.rb:131:in `<main>'

@ltm
Copy link
Sponsor

ltm commented Jul 27, 2021

I'm getting the same error when trying to install a Cask with Homebrew 3.2.5-5-g250233a:

$ brew install --verbose --debug clion
==> Cask::Installer#install
==> Printing caveats
==> Cask::Installer#fetch
/usr/bin/curl --disable --globoff --show-error --user-agent Homebrew/3.2.5-5-g250233a\ \(Macintosh\;\ Intel\ Mac\ OS\ X\ 11.5\)\ curl/7.64.1 --header Accept-Language:\ en --retry 3 --location --silent --head --request GET https://download.jetbrains.com/cpp/CLion-2021.1.3.dmg
==> Downloading https://download.jetbrains.com/cpp/CLion-2021.1.3.dmg
Error: Download failed on Cask 'clion' with message: undefined method `delete_if' for nil:NilClass
/usr/local/Homebrew/Library/Homebrew/download_strategy.rb:394:in `fetch'
/usr/local/Homebrew/Library/Homebrew/cask/download.rb:25:in `fetch'
/usr/local/Homebrew/Library/Homebrew/cask/installer.rb:171:in `download'
/usr/local/Homebrew/Library/Homebrew/cask/installer.rb:73:in `fetch'
/usr/local/Homebrew/Library/Homebrew/cask/installer.rb:100:in `install'
/usr/local/Homebrew/Library/Homebrew/cask/cmd/install.rb:69:in `block in install_casks'
/usr/local/Homebrew/Library/Homebrew/cask/cmd/install.rb:68:in `each'
/usr/local/Homebrew/Library/Homebrew/cask/cmd/install.rb:68:in `install_casks'
/usr/local/Homebrew/Library/Homebrew/cmd/install.rb:167:in `install'
/usr/local/Homebrew/Library/Homebrew/brew.rb:131:in `<main>'

I don't see an open issue for this yet.

@yahavi
Copy link
Author

yahavi commented Jul 27, 2021

@SMillerDev @ltm
Sorry for this issue. The tests passed for me in the Homebrew/brew repository.
I believe #11782 should fix it.

@ltm
Copy link
Sponsor

ltm commented Jul 27, 2021

Thanks, @yahavi. I can confirm that #11782 fixes the issue for me:

$ brew install --verbose --debug clion
==> Cask::Installer#install
==> Printing caveats
==> Cask::Installer#fetch
/usr/bin/curl --disable --globoff --show-error --user-agent Homebrew/3.2.5-5-g250233a-dirty\ \(Macintosh\;\ Intel\ Mac\ OS\ X\ 11.5\)\ curl/7.64.1 --header Accept-Language:\ en --retry 3 --location --silent --head --request GET https://download.jetbrains.com/cpp/CLion-2021.1.3.dmg
==> Downloading https://download.jetbrains.com/cpp/CLion-2021.1.3.dmg
==> Downloading from https://download-cdn.jetbrains.com/cpp/CLion-2021.1.3.dmg
/usr/bin/curl --disable --globoff --show-error --user-agent Homebrew/3.2.5-5-g250233a-dirty\ \(Macintosh\;\ Intel\ Mac\ OS\ X\ 11.5\)\ curl/7.64.1 --header Accept-Language:\ en --retry 3 --location --head https://download-cdn.jetbrains.com/cpp/CLion-2021.1.3.dmg
[...]

@MikeMcQuaid
Copy link
Member

I just followed this PR - but thank all of you.
Excited to test this, once you guys release a new version - maybe later today ? 馃

@zie-ent We'll likely have a new release within a week (probably Monday).

@MikeMcQuaid
Copy link
Member

This was tagged in 3.2.6

@github-actions github-actions bot added the outdated PR was locked due to age label Sep 4, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 4, 2021
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

6 participants