Add 386 architecture mappings and GitHub Proxy support #51
Conversation
WalkthroughThe changes extend architecture mapping to support 386 aliases (i386, x86, x86_32), add corresponding test cases for 386 architecture handling, and implement GitHub Proxy asset download detection with direct HTTP GET requests triggered by multiple "https://" occurrences in AssetURL. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
github_source.go (2)
104-109: Variable naming:downloadUrlshould bedownloadURL.Go convention for acronyms is to use all caps (e.g.,
URL,HTTP,ID).🔎 Proposed fix
- var downloadUrl string + var downloadURL string if rel.AssetID == assetID { - downloadUrl = rel.AssetURL + downloadURL = rel.AssetURL } else if rel.ValidationAssetID == assetID { - downloadUrl = rel.ValidationAssetURL + downloadURL = rel.ValidationAssetURL }
116-127: Add spacing after colons in error messages for consistency.Error messages at lines 116, 121, and 127 are missing spaces after the colons.
🔎 Proposed fix
- return nil, fmt.Errorf("failed to create download request:%w", err) + return nil, fmt.Errorf("failed to create download request: %w", err)- return nil, fmt.Errorf("download failed:%w", err) + return nil, fmt.Errorf("download failed: %w", err)- return nil, fmt.Errorf("download failed, status code:%d", resp.StatusCode) + return nil, fmt.Errorf("download failed, status code: %d", resp.StatusCode)
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
arch.go(1 hunks)arch_test.go(1 hunks)github_source.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
github_source.go (1)
errors.go (1)
ErrAssetNotFound(13-13)
🔇 Additional comments (3)
arch_test.go (1)
26-27: LGTM!Test cases for the 386 architecture aliases are well-structured and align with the implementation in
arch.go. The expected values correctly include all the new aliases in the proper order.arch.go (1)
30-32: LGTM!The 386 architecture alias handling follows the established pattern and covers the common naming conventions (
i386,x86,x86_32) used in release assets.github_source.go (1)
114-130: Download logic and error handling are correct.The request properly uses context, error paths close the response body, and success path correctly leaves the body open for the caller to close.
| // Check if the AssetURL contains more than one "https://" | ||
| useGithubProxy := strings.Count(rel.AssetURL, "https://") > 1 |
There was a problem hiding this comment.
Proxy detection only checks AssetURL, but ValidationAssetURL may differ.
If rel.ValidationAssetURL uses a proxy but rel.AssetURL doesn't (or vice versa), the current logic will use the wrong download path for one of them. Consider checking both URLs or the specific URL being requested.
🔎 Proposed fix
- // Check if the AssetURL contains more than one "https://"
- useGithubProxy := strings.Count(rel.AssetURL, "https://") > 1
- // If the AssetURL contains more than 2 "https://", it means it's using a GitHub Proxy service.
+ // Determine the download URL based on asset ID first
+ var downloadURL string
+ if rel.AssetID == assetID {
+ downloadURL = rel.AssetURL
+ } else if rel.ValidationAssetID == assetID {
+ downloadURL = rel.ValidationAssetURL
+ }
+
+ // Check if the selected URL contains more than one "https://" (indicates GitHub Proxy usage)
+ useGithubProxy := downloadURL != "" && strings.Count(downloadURL, "https://") > 1
+ // If the URL contains more than 1 "https://", it means it's using a GitHub Proxy service.Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In github_source.go around lines 97-98, proxy detection only inspects
rel.AssetURL which can differ from rel.ValidationAssetURL; update the logic to
detect proxies for the actual URL being used (check both rel.AssetURL and
rel.ValidationAssetURL as appropriate) and set useGithubProxy based on the
specific URL you will request (e.g., compute proxy flags for each URL or derive
useGithubProxy = strings.Count(urlBeingRequested, "https://") > 1 so the
download/validation path uses the correct proxy decision).
Codecov Report❌ Patch coverage is
❌ Your patch check has failed because the patch coverage (14.29%) is below the target coverage (70.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #51 +/- ##
==========================================
- Coverage 77.07% 76.95% -0.12%
==========================================
Files 28 28
Lines 1435 1167 -268
==========================================
- Hits 1106 898 -208
+ Misses 279 218 -61
- Partials 50 51 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Hi! Thank you for this pull request! Sorry it took me some time to look at it. I'm a little bit concerned that the check for the double Have you tried to check if the Github API is returning the redirected url as a second argument? On line 139 in the PR, the From the GitHub documentation:
Sorry I cannot try it myself as I don't know what kind of setup you have. |
|
I didn't change your original code, It will be working as normal as it would be. My code will be work Only when the url has two "https" like https://githubporxy.com/https://raw.githubuserconten.com/... |
2 participants
I use main fork in my project，I will cancel the old pr and now reopen a new PR。
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.