Use bundler UA when downloading gems #7092
Conversation
segiddins
force-pushed
the
user-bundler-ua-when-downloading-gems
branch
from
ea16900
to
8bf2e75
Compare
segiddins
force-pushed
the
user-bundler-ua-when-downloading-gems
branch
from
8bf2e75
to
0a245e7
Compare
segiddins
force-pushed
the
user-bundler-ua-when-downloading-gems
branch
from
0a245e7
to
5bc01ee
Compare
| end | ||
| end | ||
| end | ||
| end |
There was a problem hiding this comment.
Why are we adding this class? If we use a dedicated fetcher instance per remote and no longer use Gem::RemoteFetcher.fetcher, do we still need the cleanup?
There was a problem hiding this comment.
needed because Gem::RemoteFetcher only adds the supplied headers, and we need to be able to completely set the UA header, otherwise all the parsing we do of bundler UAs won't work
There was a problem hiding this comment.
I don't fully understand. Previously we were using a single instance and were using the headers setter to set headers, right? Was that incorrect already? Is there any test that fails if we use don't add this class, in case that illustrates its need?
There was a problem hiding this comment.
Yes, the artifice test. The header needs to be cleared so the custom UA is set rather than appended to the rubygems UA
There was a problem hiding this comment.
I don't know for sure that this class is the best approach, but I feel confident that it won't be difficult to refactor later, if needed, since it adds no new interface. We can revisit, but I think this fix should go in so the parallel fetching code is not broken on master.
There was a problem hiding this comment.
Cool, cool, I'm just curious but did not mean to block this fix.
| @@ -4,7 +4,7 @@ | |||
|
|
|||
| class EndpointMirrorSource < Endpoint | |||
| get "/gems/:id" do | |||
| if request.env["HTTP_X_GEMFILE_SOURCE"] == "https://server.example.org/" | |||
| if request.env["HTTP_X_GEMFILE_SOURCE"] == "https://server.example.org/" && request.env["HTTP_USER_AGENT"].start_with?("bundler") | |||
There was a problem hiding this comment.
This is tied to spec/realworld/gemfile_source_header_spec.rb spec, right? And you have expanded it to also check that a proper Bundler user agent is set. It's still not a very straighforward spec, but we I would add this to the spec description:
and maybe also rename the file.
segiddins
force-pushed
the
user-bundler-ua-when-downloading-gems
branch
2 times, most recently
from
6d47788
to
f1016f9
Compare
martinemde
force-pushed
the
user-bundler-ua-when-downloading-gems
branch
from
f1016f9
to
7ea0be0
Compare
Gem::RemoteFetcher uses Gem::Request, which adds the RubyGems UA. Gem::RemoteFetcher is used to download gems, as well as the full index. We would like the bundler UA to be used whenever bundler is making requests. This PR also avoids unsafely mutating the headers hash on the shared `Gem::RemoteFetcher.fetcher` instance, which could cause corruption or incorrect headers when making parallel requests. Instead, we create one remote fetcher per rubygems remote, which is similar to the connection segregation bundler is already doing
martinemde
force-pushed
the
user-bundler-ua-when-downloading-gems
branch
from
7ea0be0
to
f0e8dac
Compare
martinemde
changed the title
Gem::RemoteFetcher uses Gem::Request, which adds the RubyGems UA.
Gem::RemoteFetcher is used to download gems, as well as the full index.
We would like the bundler UA to be used whenever bundler is making
requests.
This PR also avoids unsafely mutating the headers hash on the shared
Gem::RemoteFetcher.fetcherinstance, which could cause corruption orincorrect headers when making parallel requests. Instead, we create one
remote fetcher per rubygems remote, which is similar to the connection
segregation bundler is already doing