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

Drupal can't use composer install because it requires a Github Auth token on installation #4884

Closed
klausi opened this issue Feb 5, 2016 · 104 comments

Comments

@klausi
Copy link

klausi commented Feb 5, 2016

See also https://www.drupal.org/node/1475510

Problem: when running composer install the user is prompted to input a Github auth token. That is a serious burden for 3000+ Drupal core contributors that will need to run composer install and a significant waste of developer time. Some users do not want to create a Github account.

It seems that composer only prompts for a Github auth token in interactive shells? I have never seen that prompt on Travis CI for PHP projects for example.

Proposed solution: make composer install on interactive shells behave the same as in non interactive ones: always fall back to install from source (git clone) and never prompt the user for a Github token. Users should be able to opt-in for faster downloads with Github auth tokens, but this should never be an interactive usability WTF.

@klausi klausi changed the title Drupal can't use composer because it requires a Github Auth token on installation Drupal can't use composer install because it requires a Github Auth token on installation Feb 5, 2016
@stof
Copy link
Contributor

stof commented Feb 5, 2016

It only asks for it when reaching the API rate limit, as authenticated calls have a much higher rate limit.
Never asking for it would make the users unaware they should authenticate against github to be able to keep using downloads.

A better solution would be to improve the prompt to better explain why we ask for it, and to allow to skip the authentication by giving an empty token (i.e. just hitting enter), which would also be explained in the message.

@neclimdul
Copy link

@stof I think you might be on the right track. It would by default leave some people with a odd mix of dist and source checkouts without an explanation. Could we carry that token through all additional requests during the run and/or expose this behavior as an option?

@catch56
Copy link

catch56 commented Feb 5, 2016

"It only asks for it when reaching the API rate limit"
Yes, that happens with every composer install of Drupal core because in total there are more than 60 downloads (phpunit etc. with --require-dev is quite a lot by itself)

"and to allow to skip the authentication by giving an empty token (i.e. just hitting enter), which would also be explained in the message."

It does that already, but it prompts on every individual download, so you get into a few seconds for a package, same error message again, press enter, over and over.

For me I'd show a warning message but not actually interrupt the downloads. composer install takes long enough that I'm not usually sitting holding my breath in front of the terminal window for it to finish. So I'd rather it took longer and actually happened, than go back and see it got stuck half way through. If it's still running, there's the warning message explaining why it's taking so long.

@stof
Copy link
Contributor

stof commented Feb 5, 2016

well, once you give it a token, it won't ask anymore.
what we could do to improve things is remembering that you rejecting giving a github token so that we don't ask again in the same composer run.

@klausi
Copy link
Author

klausi commented Feb 5, 2016

We do not want to force composer users to create a github account. The install process can throw warnings with bells and whistles and Github advertisements, but it should get the job done without prompting or bothering the user.

@alcohol
Copy link
Member

alcohol commented Feb 5, 2016

If you run in non-interactive mode (-n) it will switch from dist to source when hitting the limit without asking for a token I believe. Just a quick work-around until we change the flow.

@Seldaek
Copy link
Member

Seldaek commented Feb 5, 2016

I can see the problem.. having some error output in every run is however not nice either. I wonder if Drupal would have enough weight to have GitHub reconsider the 60/h limit without authentication (it is quite low tbh..).

Alternatively I suppose we could indeed remove the prompt even in interactive mode, but then we need even more output to tell people how to configure it if they do create a token.

@Seldaek Seldaek added this to the Backwards Compatible milestone Feb 5, 2016
@catch56
Copy link

catch56 commented Feb 5, 2016

@Seldaek I think it's worth a github issue - either to raise the limit or for them to actually put the zip requests through a CDN properly so people aren't hitting the API raw every time.

However "We do not want to force composer users to create a github account." 100% agreed with this - this is the reason I reverted the Drupal core commit until it was discussed a bit more, and relying on them to raise their API limit doesn't really help with that dependency.

@skyred
Copy link

skyred commented Feb 5, 2016

At the moment, Composer drupal-update can easily hit the Githubt rate limit. I am using Docker Hub for automated build. The rate limit applies on IP address, so more then often the builds are fails.

@skyred
Copy link

skyred commented Feb 5, 2016

Using interactive promptdoesn't help automated build. Github personal token only works if the automated build if NOT open sourced. If it is open sourced, Github will automatically revoke the public personal token.

@wimleers
Copy link
Contributor

wimleers commented Feb 5, 2016

requests through a CDN

My thoughts exactly. But they probably don't want to do this, to avoid 1) big bandwidth cost, 2) using GitHub to distribute pirated content.

@stof
Copy link
Contributor

stof commented Feb 5, 2016

Using interactive promptdoesn't help automated build. Github personal token only works if the automated build if NOT open sourced. If it is open sourced, Github will automatically revoke the public personal token.

Well, it depends how you store the token. When using Travis, they have a concept of secure environment variables which means that the token won't be public.
And I'm not sure they would revoke a token without any scope.

@mradcliffe
Copy link

Well, it depends how you store the token. When using Travis, they have a concept of secure environment variables which means that the token won't be public.
And I'm not sure they would revoke a token without any scope.

Using secure environment variable for a public Travis build is broken. That is probably a Travis bug, or maybe it is expected behavior for open source builds on Travis.

Edit: I stand corrected per @bartfeenstra's correction. Thank you.

@neclimdul
Copy link

Gitlab CI has variables to though I haven't looked into if they have the level of "secure" that Travis does. As far as I can tell docker hub does not though but that's probably a limitation of docker build itself. You'll probably want the -n option @Seldaek mentioned if composer is detecting an interactive shell. None of the environmental variable discussion is really pertinent to improving the workflow for users without an API key though which is the core of the issue.

@bartfeenstra
Copy link

Secure variables work, but not in pull requests through forks. As a consequence, builds for any such PR will fail, because the token cannot be decrypted.

@cs278
Copy link
Contributor

cs278 commented Feb 5, 2016

I think it's worth a github issue - either to raise the limit or for them to actually put the zip requests through a CDN properly so people aren't hitting the API raw every time.

@catch56 The reason for using the API for downloads is GitHub guarantee that the endpoint will not change, historically Composer used the download URLs from the website but those have changed overtime. If GitHub stopped rate limiting API download calls that'd really help.

@stof
Copy link
Contributor

stof commented Feb 5, 2016

@bartfeenstra it won't fail. It will fallback to the slower source install (cloning a git repo is slower than downloading a zip archive of the code at a specific commit, as it is much bigger)

@Crell
Copy link

Crell commented Feb 5, 2016

What if we look at this from the other direction: The main issue with downloading sources for vendor dependencies is you're downloading a lot more data, since you get the full history even if you don't need it. What if we allowed for shallow clones, or even defaulted to that? Downloading the latest commit should be in the same neighborhood as downloading a static snapshot, and just one or two commits back is not going to add a great deal of cost. Certainly far less than the full history of a project like Drupal, PHPUnit, or Symfony HttpKernel. (Let's face it, it's not like having the code snapshot of Drupal 4.1 or PHPUnit 2 is at all useful to 99.999999% of people installing Drupal or Symfony today.)

That would make "skip this noise, just grab sources" a much cheaper and therefore more viable option.

@Seldaek
Copy link
Member

Seldaek commented Feb 5, 2016

@Crell yes we should look at #2833 (and maybe #4685 would help too on that front). Not sure if it's workable though to be honest as we might end up having to fetch every time.

@cs278
Copy link
Contributor

cs278 commented Feb 6, 2016

@Seldaek #4685 should be fairly safe as it only uses the cache for the initial clone, although it requires Git 2.3 but should be possible to replicate that behaviour for older Git's. gitster/git@d35c802

@damienmckenna
Copy link

For what it's worth OSX 10.11 is bundled with git 2.3.x these days, and it probably wouldn't be too much of a big deal to strongly recommend using 2.3 or newer for people wanting to work on Drupal core.

@chx
Copy link
Contributor

chx commented Feb 7, 2016

I wonder if Drupal would have enough weight to have GitHub reconsider the 60/h limit without authentication (it is quite low tbh..).

I have no idea. What would you like it to be? I have a call with Jono Bacon next week and I will mention. Absolutely no promises but I'll mention it to him.

@Seldaek
Copy link
Member

Seldaek commented Feb 8, 2016

@chx well the main point that would be nice is to get the ability to use the zipball URLs without API limit (or with a higher one.. the exact number probably depends on who is running composer), because really this isn't an API call per se.

@neclimdul
Copy link

earlier @cs278 mentioned using the non rate limited zips in the past. Could we consider using those, falling back on the api, then falling back on the source? That way if they move again things still "work" until the user updates to a release where composer has fixed the URLs.

If the API rate limit message also funnelled people to a useful wiki/help page then I think it could also clear things up for people. "Oh I suddenly saw this because github changed things. No big deal, I can update."

@ryanaslett
Copy link
Contributor

I came here to say what neclimdul said already. +1

@catch56
Copy link

catch56 commented Feb 18, 2016

That sounds good to me as well trying to use the static zips then falling back to the API @Seldaek would that be an acceptable solution for you too?

@Seldaek
Copy link
Member

Seldaek commented Feb 18, 2016

I guess we could do this yes, as long as there's a fallback. It can probably be hooked in after FileDownloader:87 once we have $urls we could go through them and add the direct URLs before the api one if one is found. And I guess for future proofing it, in case one of these fails we probably should stop trying for the other URLs. Just in case github removes support for that composer won't try everything with a timeout/error for every package.

@neclimdul
Copy link

Sounds like pretty robust logic to me. Good catch on the timeout bit, that probably would get annoying.

@pscheit
Copy link
Contributor

pscheit commented Nov 26, 2016

how is this possible, that i hit this in in november? :o

Reading ./composer.lock
Loading composer repositories with package information
Installing dependencies (including require-dev) from lock file
Reading ./composer.lock
Resolving dependencies through SAT
Dependency resolution completed in 0.006 seconds
Analyzed 189 packages to resolve dependencies
Analyzed 528 rules to resolve dependencies
  - Removing asika/pdf2text (1.0.1)
  - Installing asika/pdf2text (1.0.5)
Downloading https://api.github.com/repos/asika32764/php-pdf-2-text/zipball/a14ea95695a277e385dbc03caeddb91c5e10319f
    Downloading: Connecting...
Failed: [Composer\Downloader\TransportException] 401: Could not authenticate against github.com

                                             
  [Composer\Downloader\TransportException]   
  Could not authenticate against github.com  
                                             

Exception trace:
() at phar:///usr/local/bin/composer.phar/src/Composer/Util/RemoteFilesystem.php:580

it's a machine with fixed ip. strangely I can wget this file without problems?

@Seldaek
Copy link
Member

Seldaek commented Nov 26, 2016 via email

@pscheit
Copy link
Contributor

pscheit commented Nov 27, 2016

nope. None of this is used in this project. But it seems to work / won't work, so maybe it's something api limit related.

soullivaneuh added a commit to soullivaneuh/sonata-dev-kit that referenced this issue Oct 7, 2017
soullivaneuh added a commit to soullivaneuh/IsoCodes that referenced this issue Nov 2, 2017
soullivaneuh added a commit to soullivaneuh/IsoCodesValidator that referenced this issue Nov 2, 2017
soullivaneuh added a commit to soullivaneuh/IsoCodesValidator that referenced this issue Nov 2, 2017
soullivaneuh added a commit to soullivaneuh/IsoCodesValidator that referenced this issue Nov 2, 2017
soullivaneuh added a commit to soullivaneuh/IsoCodesValidator that referenced this issue Nov 2, 2017
elrido added a commit to PrivateBin/PrivateBin that referenced this issue Jan 2, 2018
ezzatron added a commit to eloquent/phpstan-phony that referenced this issue Jul 2, 2018
hackzilla added a commit to hackzilla/password-generator that referenced this issue Apr 28, 2020
fnogatz added a commit to fnogatz/magento2-matomo that referenced this issue May 1, 2021
- Remove unnecessary environment variable GITHUB_OAUTH_TOKEN (former API limit has been [removed](composer/composer#4884 (comment)) in 2016)
- Remove manual doctrine/instantiator downgrade
- We support only PHP 7.3 and 7.4 as well as M2 starting from v2.3.3 (former versions reached EOL)
fnogatz added a commit to fnogatz/magento2-matomo that referenced this issue May 2, 2021
* Update Travis CI config

- Remove unnecessary environment variable GITHUB_OAUTH_TOKEN (former API limit has been [removed](composer/composer#4884 (comment)) in 2016)
- Remove manual doctrine/instantiator downgrade
- We support only PHP 7.3 and 7.4 as well as M2 starting from v2.3.3 (former versions reached EOL)

* Fix PHP Fatal Error with PHPUnit

setUp() must be compatible with PHPUnit\Framework\TestCase::setUp() – add missing declaration of "void" as return value

* Trigger Travis CI

* CI: Remove testing for M2 v2.3.3 and v2.3.4

Magento version prior to v2.3.5 depend on xdebug<3, so these tests fail with a "Call to undefined function xdebug_disable()" error. We simply stick to the latest two versions of Magento v2.3.x.
shawnbenito126 added a commit to shawnbenito126/Drupal-ansible-role-drush that referenced this issue Oct 20, 2022
Rate limits have been removed from github: composer/composer#4884 (comment)
rooksean916 added a commit to rooksean916/role-drush-Drupal that referenced this issue Oct 21, 2022
Rate limits have been removed from github: composer/composer#4884 (comment)
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

No branches or pull requests