-
Notifications
You must be signed in to change notification settings - Fork 407
Use separate session to download assets from AWS #288
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
Conversation
This fixes an error "Only one auth mechanism allowed; only the X-Amz-Algorithm query parameter, Signature query string parameter or the Authorization header should be specified"
Seems, tests for Assets downloading should be modified, but this is up to you :) |
@sigmavirus24 , do you have any plans to include this fix in next release? This is quite important for me as now i need to patch github3.py library and i really don't like this. |
@eugef: I just noticed that this caused a test failure under Travis CI:
|
The above error happens both on Travis CI and also when I checked out your branch locally and ran |
The test failures are due to the fact that Betamax prefers explicit over implicit (which is contrary to how it's inspiration, VCR, works). That said, you're trying to make a real request to the internet with an old asset id. When the tests were written, using the same session and eliminating the headers presented no issue. I need to reproduce the failure that @eugef has apparently seen before merging this (or coming up with a solution for the test problem). |
Here's how I started to look at the test failure with pdb, by using my
|
Ideas such as? Monkey-patching the world: no. |
I was referring to:
which admittedly, I don't understand, so I can't say anything smart. :-) |
Should have linked to the real VCR |
So here's the catch. If you do this: import github3
r = github3.repository('sigmavirus24', 'github3.py')
release = filter(lambda _r: _r.tag_name == 'v0.7.1', r.iter_releases())[0]
asset = next(release.iter_assets())
asset.download() It downloads perfectly for me. I even checked the MD5 checksums (downloading via |
@sigmavirus24. try to download asset from the private repository - probably you will catch the bug in this case. |
I don't have a private repository to test with. |
In my case i got the issue on a private repository. |
Do you have a spare private repo that I can test with? |
Hi @sigmavirus24, this issue occurs only if you use github3.login() method before github3.repository() - that is why i have it with a private repo. But you can reproduce this bug even with public repo by running this code: import github3
g = github3.login(USERNAME, PASSWORD_OR_ACCEES_TOKEN)
r = g.repository('sigmavirus24', 'github3.py')
release = filter(lambda _r: _r.tag_name == 'v0.7.1', r.iter_releases())[0]
asset = next(release.iter_assets())
asset.download() |
Ah yes, I reproduced the failure with the example that @eugef just provided.
|
In that case I know the proper way of fixing this. I'll leave this open as the bug report. |
There is a great deal of complexity in requests surrounding how headers are managed and merged. Since the authentication is applied after headers are merged, using basic authentication can still be applied. Using this context manager ensures it will not be set again. Fix #288
This has been released in 0.9.3 |
Thanks, it works like a charm! |
Glad to hear it @eugef and thanks for being patient |
There is a great deal of complexity in requests surrounding how headers are managed and merged. Since the authentication is applied after headers are merged, using basic authentication can still be applied. Using this context manager ensures it will not be set again. Fix sigmavirus24#288
This fixes an error "Only one auth mechanism allowed; only the X-Amz-Algorithm query parameter, Signature query string parameter or the Authorization header should be specified" which occurs when downloading asset for the release.
The easiest fix is to use another Session as clearing headers doesn't work properly.
Steps to reproduce an issue: