Remove Curl dependency#766
Conversation
Use build-in HTTPClient instead Signed-off-by: Kostiantyn Kostiuk <kkostiuk@redhat.com>
Summary of ChangesHello @kostyanf14, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request focuses on enhancing the project's maintainability and reducing its external footprint by eliminating the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request successfully removes the curb dependency and its native extension libcurl, replacing it with the pure Ruby httpclient gem. This is a great improvement for simplifying the setup. The implementation in downloader.rb is functional, but I've suggested a more efficient and robust approach that uses a single GET request instead of separate HEAD and GET requests to download files.
| head = client.head(url, follow_redirect: true) | ||
| raise(AutoHCKError, "Download failed with code #{head.status}") unless head.status == 200 | ||
|
|
||
| total_size = head.headers['Content-Length'].to_i | ||
|
|
||
| File.open(path, 'wb') do |file| | ||
| client.get(url, follow_redirect: true) do |chunk| | ||
| file.write(chunk) | ||
| log_progress(total_size, file.size) | ||
| end | ||
| end |
There was a problem hiding this comment.
The current implementation makes two separate HTTP requests: a HEAD to get the file size, followed by a GET to download the content. This is inefficient and may not be robust, as some servers might not support HEAD requests for all URLs.
A more efficient approach is to use a single GET request. You can obtain the response headers (including Content-Length) from the response object and then stream the body from the same response. This avoids the extra network round-trip.
response = client.get(url, follow_redirect: true)
raise(AutoHCKError, "Download failed with code #{response.status}") unless response.status == 200
total_size = response.headers['Content-Length'].to_i
File.open(path, 'wb') do |file|
# Read in 16KB chunks.
while (chunk = response.content.read(16 * 1024))
file.write(chunk)
log_progress(total_size, file.size)
end
endThere was a problem hiding this comment.
This is incorrect, get will download the file as well.
Use build-in HTTPClient instead