Update lifecycle.DownloadBinary to work with local file #5398
Conversation
Signed-off-by: khanhtc1202 <khanhtc1202@gmail.com>
khanhtc1202
requested review from
nghialv,
kentakozuka,
ffjlabo,
t-kikuc and
Warashi
as code owners
| return "", fmt.Errorf("HTTP GET %s failed with error %d", url, resp.StatusCode) | ||
| } | ||
| switch u.Scheme { | ||
| case "http", "https": |
There was a problem hiding this comment.
This makes the test easier since I don't want to mock the HTTPS server. The validation logic in the piped configuration remains to only allow the "https" and "file" schemes.
There was a problem hiding this comment.
📝 It is difficult to test with https server because DownloadBinary uses http.Client{} internally and can't inject for now. So we can't use httptest.NewTLSServer.
There was a problem hiding this comment.
@t-kikuc For the question you asked 🙏
There was a problem hiding this comment.
I got it.
binary.go allows http and https, PipedPlugin disallows http.
Lines 1328 to 1330 in dcecc2d
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #5398 +/- ##
==========================================
+ Coverage 25.74% 25.76% +0.02%
==========================================
Files 449 449
Lines 48401 48416 +15
==========================================
+ Hits 12459 12474 +15
+ Misses 34971 34969 -2
- Partials 971 973 +2 ☔ View full report in Codecov by Sentry. |
| return "", fmt.Errorf("HTTP GET %s failed with error %d", url, resp.StatusCode) | ||
| } | ||
| switch u.Scheme { | ||
| case "http", "https": |
There was a problem hiding this comment.
[ASK] Are we gonna support http too? or only https?
There was a problem hiding this comment.
No we don't, just place http there to make testing process simpler 🙏
What this PR does:
Update lifecycle.DownloadBinary logic to work with local files. After this pr, pipedv1 can fetch plugins from a remote source (via HTTP protocol) or local files from another file path.
Why we need it:
Which issue(s) this PR fixes:
Part of #4980
Follow #5394
Does this PR introduce a user-facing change?: