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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix - Use GITHUB_TOKEN when downloading zipballs #1055

Merged
merged 4 commits into from
Apr 16, 2021

Conversation

gnattishness
Copy link
Contributor

@gnattishness gnattishness commented Apr 14, 2021

What I did

Have _stream_download() also use GITHUB_TOKEN

Related issue: fixes #1054

How I did it

Move GITHUB_TOKEN retrieval into standalone helper function, pass headers as optional arguments to _stream_download() and _get_mix_default_branch()

Looks like this was also not being used in from_brownie_mix().

How to verify it

Can try installing a github brownie package from a private repo (it now succeeds).

Not too clear how to easily/reasonably include this in the test cases - the test would probably have to rely on an external AUTH token and an active internet connection, or some swanky mocking.

Checklist

  • I have confirmed that my PR passes all linting checks
  • I have included test cases
  • I have updated the documentation
  • I have added an entry to the changelog

@gnattishness
Copy link
Contributor Author

This passes the token through to _get_mix_default_branch() but it's perhaps arguable whether this is useful.
You could argue that any brownie mix (at that url) should be public anyway and shouldn't need the access token.

In that case, it would probably be worth removing the "If this issue persists, generate a Github API token and store..." error message from _get_mix_default_branch().

On the other hand, I suppose it might be useful to use a token for custom rate limiting, and it's not exposing any new info.

@gnattishness
Copy link
Contributor Author

gnattishness commented Apr 14, 2021

Let me know what you think, and I'll give it a squish and rebase before you accept 🙂

Copy link
Member

@iamdefinitelyahuman iamdefinitelyahuman left a comment

Choose a reason for hiding this comment

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

Thanks for this!

@iamdefinitelyahuman iamdefinitelyahuman merged commit e53f3d8 into eth-brownie:master Apr 16, 2021
@gnattishness gnattishness deleted the fix-github-auth branch April 16, 2021 22:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

github package install failure from private repo: GITHUB_TOKEN not used in zip download.
2 participants