change cURL download behaviour for HOMEBREW_ARTIFACT_DOMAIN. #13258
Conversation
|
@UiP9AV6Y Thanks for the PR! It's broken some tests that need fixed. How do |
yes, i have added test cases for this feature, which
|
This was reverted again in #11793. |
Seems to be, thanks! |
not sure how i have missed that tbh. i also did not find it in the manpage on the homepage itself when i was looking. anyway, since sorry for the noise |
| @@ -459,7 +459,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{^(https?://#{GitHubPackages::URL_DOMAIN}/)?}o, "#{domain.chomp("/")}/") | |||
| url = url.sub(%r{^https?://#{GitHubPackages::URL_DOMAIN}/}o, "#{domain.chomp("/")}/") | |||
There was a problem hiding this comment.
It seems like this may still be a desirable change? Thoughts?
There was a problem hiding this comment.
not really if i understand the codebase correctly. while this logic is implemented in a Curl base class, which is used by a variety of ther download stategies, it really only affects GitHubPackages::URL_DOMAIN, which is where the GithubPackages download stategy comes into play; and that can be influence with the HOMEBREW_BOTTLE_DOMAIN variable.
going forward with this change would essentially make HOMEBREW_BOTTLE_DOMAIN and HOMEBREW_ARTIFACT_DOMAIN somewhat equal (albeight being implemented differently)
with #13313 on the horizon, even authentication can be
supported for both scenarios.
There was a problem hiding this comment.
So missed that this was not merged for the 3.5.x release. My issue didn't have anything to do with Authentication and it was the fact that the Packages URL was incorrectly prefixing Cask URLs. So that change that @MikeMcQuaid you call out is key for that to work and I think was a bug that creeped in on the original work. I can't see any value on having a positive regex match on "https" and trigger a replace. I have tested w 3.5.1 and the cask download issue persists.
Can we get this PR re-reviewed for merge? The auth change won't obsolete this one. I know the JFrog team is also watching this one as well.
There was a problem hiding this comment.
Can we get this PR re-reviewed for merge? The auth change won't obsolete this one. I know the JFrog team is also watching this one as well.
@byjrack The PR author closed the PR. If they want to reopen it they are welcome. It may make more sense for you to make your own PR, though.
There was a problem hiding this comment.
Yeah I had my original PR, but it was missing the tests so I will get my local env cleaned up and see about reopening my original one with this fix, the doc update, and the tests.
setting |
|
@UiP9AV6Y thanks for reopening the PR. Looks like the check it failed was due to a rate limiter on the API and not code related so you may need to close/open again to re-trigger the checks. |
instead of prefixing and/or replacing data in URLs, the
HOMEBREW_ARTIFACT_DOMAIN environment variable only replaces
the bottle base URL. this causes URLs from Casks and other assets
to be no longer affected by this feature.
closes #13226
closes #13222
obsoletes #13227
brew stylewith your changes locally?brew typecheckwith your changes locally?brew testswith your changes locally?