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

pull: fix bintray verification failing due to redirection #3298

Merged
merged 1 commit into from Oct 17, 2017

Conversation

Projects
None yet
4 participants
@vszakats
Member

vszakats commented Oct 10, 2017

  • 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 tests with your changes locally?

When doing a brew pull --bottle <n> for the second time, the command tries to verify if the uploaded package is there and correct. This failed in my case with an error:

==> Verifying bottles published on Bintray
Verifying bottle: caddy-0.10.10.high_sierra.bottle.tar.gz
Error: Failed to find published caddy bottle at https://homebrew.bintray.com/bottles/caddy-0.10.10.high_sierra.bottle.tar.gz (302 )!

What happens here is that brew pull is attempting a pre-check of the bottle URL (before starting the full download), by using the Net::HTTP Ruby class. In this particular case though — and this well may depend on geolocation — the URL returns a 302 redirect (to a valid CloudFront URL). 302 is then considered and error and the command bails out without continuing to the curl download to do the actual verification.

Net::HTTP doesn't have an option to follow redirects automatically and 302 is likely satisfying the goal of the pre-check (its stated goal is to wait in a loop till the upload finishes to propagate/publish upstream), so this patch simply accepts 302 as a success and let it proceed to the actual download.

@iMichka

This comment has been minimized.

Show comment
Hide comment
@iMichka

iMichka Oct 10, 2017

Contributor

See #3134, where I tried to fix the same problem.

Contributor

iMichka commented Oct 10, 2017

See #3134, where I tried to fix the same problem.

@vszakats

This comment has been minimized.

Show comment
Hide comment
@vszakats

vszakats Oct 10, 2017

Member

Thanks @iMichka, I haven't seen it.

Mine is the "lazy fix" though, yours is the complete package with following redirects.

FWIW in my case 302 happened right after the initial upload and on macOS. I think the point is that such may happen anytime on any OS since it's up to Bintray (or to any other CDN) to serve its content this way whenever it decides to.

IMO your original PR should be reopened and this one closed in favour of it.

Member

vszakats commented Oct 10, 2017

Thanks @iMichka, I haven't seen it.

Mine is the "lazy fix" though, yours is the complete package with following redirects.

FWIW in my case 302 happened right after the initial upload and on macOS. I think the point is that such may happen anytime on any OS since it's up to Bintray (or to any other CDN) to serve its content this way whenever it decides to.

IMO your original PR should be reopened and this one closed in favour of it.

@ilovezfs

This comment has been minimized.

Show comment
Hide comment
@ilovezfs

ilovezfs Oct 10, 2017

Contributor

@vszakats if both solve the problem, why would you prefer a +39 −20 change over a +1 −1 change?

Contributor

ilovezfs commented Oct 10, 2017

@vszakats if both solve the problem, why would you prefer a +39 −20 change over a +1 −1 change?

@vszakats

This comment has been minimized.

Show comment
Hide comment
@vszakats

vszakats Oct 10, 2017

Member

@ilovezfs Following the redirect is the canonical/future-proof way, though if the short fix is acceptable for everyone I for sure don't mind using it instead. It does fix the problem for now and may be extended later if it turns out a valid redirect would still not guarantee a propagated upload.

Member

vszakats commented Oct 10, 2017

@ilovezfs Following the redirect is the canonical/future-proof way, though if the short fix is acceptable for everyone I for sure don't mind using it instead. It does fix the problem for now and may be extended later if it turns out a valid redirect would still not guarantee a propagated upload.

@ilovezfs

This comment has been minimized.

Show comment
Hide comment
@ilovezfs

ilovezfs Oct 10, 2017

Contributor

@vszakats In the 302 case, with your fix, it still ends up downloading the file, right?

Contributor

ilovezfs commented Oct 10, 2017

@vszakats In the 302 case, with your fix, it still ends up downloading the file, right?

@vszakats

This comment has been minimized.

Show comment
Hide comment
@vszakats

vszakats Oct 10, 2017

Member

@ilovezfs: It does, yes.

Member

vszakats commented Oct 10, 2017

@ilovezfs: It does, yes.

@MikeMcQuaid

Nice simple fix, thanks @vszakats!

@MikeMcQuaid MikeMcQuaid merged commit 5f64d0f into Homebrew:master Oct 17, 2017

1 of 3 checks passed

codecov/patch 0% of diff hit (target 68.11%)
Details
codecov/project 67.18% (-0.93%) compared to 56458f0
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@vszakats vszakats deleted the vszakats:pull302 branch Oct 17, 2017

@Homebrew Homebrew locked and limited conversation to collaborators May 4, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.