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

Implmement R10K::Util::Downloader #1243

Merged
merged 1 commit into from
Nov 10, 2021
Merged

Conversation

reidmv
Copy link
Contributor

@reidmv reidmv commented Nov 8, 2021

Chunk PR in support of (original monster PR) #1159

This PR implements a mixin for R10K objects which need to download, checksum, and save/copy files. The mixin isn't used by anything yet in this PR. A subsequent PR will introduce a consumer of the mixin, the Tarball object.

@reidmv reidmv requested a review from a team November 8, 2021 20:20
@reidmv reidmv mentioned this pull request Nov 8, 2021
Copy link
Collaborator

@Magisus Magisus left a comment

Choose a reason for hiding this comment

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

This seems alright for most use cases. If anything else comes up that we need to add (e.g. timeouts), we can do that separately I think.

lib/r10k/util/downloader.rb Outdated Show resolved Hide resolved
# @param uri [URI] The URI to download the file from
# @param redirect_limit [Integer] How many redirects to permit before failing
# @param proxy [URI, String] The URI to use as a proxy
def http_get(uri, redirect_limit: 10, proxy: nil, &block)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm actually surprised we don't already have anything like this, but I guess it makes sense. All of our current downloading goes through other libs (forge-ruby/faraday, git/rugged).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah. As a point of interest I have a Faraday version of this in another branch but at the time it was written R10K couldn't use a new enough version of Faraday to support streaming downloads, which I would prefer to do for checksumming, and testing streaming downloads with Faraday seemed hard.

Digging up the faraday code, it looked something like this in comparison:

# @param uri [URI] The URI to download from
# @param output The file or path to save to
# @return [String] The downloaded file's sha256 hex digest
def download_to_file(uri, output)
  digest = DIGEST_CLASS.new

  File.open(output, 'wb') do |output_stream|
    faraday_connection.get(uri) do |req|
      req.options.on_data = Proc.new do |chunk, _|
        output_stream.write(chunk)
        digest.update(chunk)
      end
    end
  end

  digest.hexdigest
end

# Defined separately to enable test stubbing
def faraday_connection
  Faraday.new do |faraday|
    faraday.response(:follow_redirects)
    faraday.adapter(:net_http)
    faraday.proxy(settings[:proxy]) unless settings[:proxy].nil?
  end
end

I honestly have no idea if it worked as expected, as I said testing was hard.

The alternative to streaming downloading would be to download the file first, then checksum it separately.

Since I already had the non-Faraday code and it seemed to work well, I didn't pursue it further.

Copy link
Collaborator

@Magisus Magisus Nov 10, 2021

Choose a reason for hiding this comment

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

Yeah honestly I'm totally fine not depending more directly on faraday. There are way too many layers over in that family of libraries. We're using Net::HTTP directly in Puppet, in a similar (but more elaborate) way to what you're doing here.

lib/r10k/util/downloader.rb Show resolved Hide resolved
@Magisus
Copy link
Collaborator

Magisus commented Nov 10, 2021

Took a look at the way this is used in the subsequent PR, and the API seems fine to me (fairly idiomatic anyway).

Taking methods originally defined in R10K::Tarball and placing them into
a sharable utility module.
@Magisus Magisus merged commit c718835 into puppetlabs:main Nov 10, 2021
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.

2 participants