go.mod: replace old 'grab' dependency #4795
Conversation
Reviewer's GuideReplaces the outdated cavaliergopher/grab/v3 dependency with a maintained fork (sebrandon1/grab v0.0.1), updates import paths in the download logic, and refreshes the vendor directory to reflect the new library structure. Class diagram for updated grab dependency usageclassDiagram
class grab.Client {
}
class grab.Request {
}
class grab.Response {
}
class Download {
+doRequest(client: grab.Client, req: grab.Request) string, error
}
Download --> grab.Client
Download --> grab.Request
Download --> grab.Response
%% Note: The grab package is now imported from github.com/sebrandon1/grab/lib instead of github.com/cavaliergopher/grab/v3
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Hi @sebrandon1. Thanks for your PR. I'm waiting for a crc-org member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @sebrandon1 - I've reviewed your changes and they look great!
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
sebrandon1
force-pushed
the
replace_grab
branch
3 times, most recently
from
d0c3911 to
3712ea0
Compare
|
/ok-to-test |
sebrandon1
force-pushed
the
replace_grab
branch
2 times, most recently
from
f4c9c1e to
98f1d62
Compare
WalkthroughReplaced vendored dependency github.com/cavaliergopher/grab/v3 with github.com/sebrandon1/grab v0.0.4, updated imports to Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller
participant grabLib as grab/lib
participant GetBatch
participant Worker as Downloader
participant FS as FileSystem
Caller->>grabLib: DownloadBatch(ctx, urls)
grabLib->>GetBatch: GetBatch(workers=..., dst=".", urls...)
note right of GetBatch #DDEBF7: starts worker pool
GetBatch->>Worker: start HTTP download (concurrent)
Worker->>FS: write file data
FS-->>Worker: write complete / error
Worker-->>GetBatch: send *Response
GetBatch-->>grabLib: responses channel
grabLib-->>Caller: forward DownloadResponse via channel
note right of Caller #E6F4EA: Caller consumes async results
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Pre-merge checks (1 passed, 2 warnings)❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. ✨ Finishing Touches
🧪 Generate unit tests
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: 2
🔭 Outside diff range comments (1)
vendor/github.com/sebrandon1/grab/lib/util.go (1)
37-38: Tighten directory permissions to the conventional 0755.
os.MkdirAll(dir, 0777)grants world-writable perms, unexpected in most download scenarios. 0755 is more typical and still lets the current user write.- if err := os.MkdirAll(dir, 0777); err != nil { + if err := os.MkdirAll(dir, 0o755); err != nil {
🧹 Nitpick comments (3)
vendor/github.com/sebrandon1/grab/lib/util.go (1)
30-37: Avoid shadowing the importedpathpackage.
The parameterpath stringinmkdirphides the imported"path"package, which can trip up future refactors (e.g. addingpath.*helpers inside the function). Renaming the parameter keeps the namespace clear.-func mkdirp(path string) error { - dir := filepath.Dir(path) +func mkdirp(dst string) error { + dir := filepath.Dir(dst)vendor/github.com/sebrandon1/grab/lib/README.md (1)
37-90: Replace hard tabs with spaces in code examples for better markdown compatibility.The code examples use hard tabs which can cause inconsistent rendering across different markdown viewers and platforms. Consider using spaces (typically 4 spaces) for indentation in markdown code blocks.
vendor/github.com/sebrandon1/grab/lib/client.go (1)
558-559: Remove redundant error check.There's a redundant check for
resp.err != nilon line 558 when it was just assigned on line 556.if err := closer.Close(); err != nil { resp.err = fmt.Errorf("cannot close writer for %q: %w", resp.Filename, err) - // if we cannot close the writer, we cannot continue - if resp.err != nil && !resp.Request.NoStore && resp.Request.deleteOnError { + if !resp.Request.NoStore && resp.Request.deleteOnError { // if we cannot close the writer, we cannot continue if err := os.Remove(resp.Filename); err != nil {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (16)
go.mod(1 hunks)pkg/download/download.go(1 hunks)vendor/github.com/cavaliergopher/grab/v3/pkg/bps/bps.go(0 hunks)vendor/github.com/cavaliergopher/grab/v3/pkg/bps/sma.go(0 hunks)vendor/github.com/sebrandon1/grab/lib/README.md(1 hunks)vendor/github.com/sebrandon1/grab/lib/client.go(7 hunks)vendor/github.com/sebrandon1/grab/lib/doc.go(2 hunks)vendor/github.com/sebrandon1/grab/lib/download.go(1 hunks)vendor/github.com/sebrandon1/grab/lib/error.go(1 hunks)vendor/github.com/sebrandon1/grab/lib/grab.go(1 hunks)vendor/github.com/sebrandon1/grab/lib/rate_limiter.go(1 hunks)vendor/github.com/sebrandon1/grab/lib/request.go(1 hunks)vendor/github.com/sebrandon1/grab/lib/response.go(5 hunks)vendor/github.com/sebrandon1/grab/lib/transfer.go(2 hunks)vendor/github.com/sebrandon1/grab/lib/util.go(1 hunks)vendor/modules.txt(1 hunks)
💤 Files with no reviewable changes (2)
- vendor/github.com/cavaliergopher/grab/v3/pkg/bps/bps.go
- vendor/github.com/cavaliergopher/grab/v3/pkg/bps/sma.go
🧰 Additional context used
🪛 LanguageTool
vendor/github.com/sebrandon1/grab/lib/README.md
[uncategorized] ~11-~11: Loose punctuation mark.
Context: ... of the file downloaded. - Err error: Any error encountered during download (...
(UNLIKELY_OPENING_PUNCTUATION)
🪛 markdownlint-cli2 (0.17.2)
vendor/github.com/sebrandon1/grab/lib/README.md
41-41: Hard tabs
Column: 1
(MD010, no-hard-tabs)
42-42: Hard tabs
Column: 1
(MD010, no-hard-tabs)
43-43: Hard tabs
Column: 1
(MD010, no-hard-tabs)
47-47: Hard tabs
Column: 1
(MD010, no-hard-tabs)
48-48: Hard tabs
Column: 1
(MD010, no-hard-tabs)
49-49: Hard tabs
Column: 1
(MD010, no-hard-tabs)
50-50: Hard tabs
Column: 1
(MD010, no-hard-tabs)
51-51: Hard tabs
Column: 1
(MD010, no-hard-tabs)
52-52: Hard tabs
Column: 1
(MD010, no-hard-tabs)
53-53: Hard tabs
Column: 1
(MD010, no-hard-tabs)
54-54: Hard tabs
Column: 1
(MD010, no-hard-tabs)
55-55: Hard tabs
Column: 1
(MD010, no-hard-tabs)
56-56: Hard tabs
Column: 1
(MD010, no-hard-tabs)
57-57: Hard tabs
Column: 1
(MD010, no-hard-tabs)
58-58: Hard tabs
Column: 1
(MD010, no-hard-tabs)
68-68: Hard tabs
Column: 1
(MD010, no-hard-tabs)
69-69: Hard tabs
Column: 1
(MD010, no-hard-tabs)
73-73: Hard tabs
Column: 1
(MD010, no-hard-tabs)
74-74: Hard tabs
Column: 1
(MD010, no-hard-tabs)
75-75: Hard tabs
Column: 1
(MD010, no-hard-tabs)
76-76: Hard tabs
Column: 1
(MD010, no-hard-tabs)
77-77: Hard tabs
Column: 1
(MD010, no-hard-tabs)
78-78: Hard tabs
Column: 1
(MD010, no-hard-tabs)
79-79: Hard tabs
Column: 1
(MD010, no-hard-tabs)
80-80: Hard tabs
Column: 1
(MD010, no-hard-tabs)
81-81: Hard tabs
Column: 1
(MD010, no-hard-tabs)
82-82: Hard tabs
Column: 1
(MD010, no-hard-tabs)
83-83: Hard tabs
Column: 1
(MD010, no-hard-tabs)
84-84: Hard tabs
Column: 1
(MD010, no-hard-tabs)
85-85: Hard tabs
Column: 1
(MD010, no-hard-tabs)
86-86: Hard tabs
Column: 1
(MD010, no-hard-tabs)
87-87: Hard tabs
Column: 1
(MD010, no-hard-tabs)
88-88: Hard tabs
Column: 1
(MD010, no-hard-tabs)
⏰ Context from checks skipped due to timeout of 90000ms (17)
- GitHub Check: build-qe (linux, arm64)
- GitHub Check: build (windows-2022, 1.23)
- GitHub Check: build-qe (darwin, amd64)
- GitHub Check: build-qe (windows, amd64)
- GitHub Check: build-qe (linux, amd64)
- GitHub Check: build-qe (darwin, arm64)
- GitHub Check: build-installer (windows-2022, 1.23)
- GitHub Check: build (macOS-14, 1.23)
- GitHub Check: build (macOS-13, 1.23)
- GitHub Check: build (macOS-13, 1.23)
- GitHub Check: build (ubuntu-22.04, 1.23)
- GitHub Check: build (ubuntu-latest, 1.23)
- GitHub Check: build (windows-2022, 1.23)
- GitHub Check: build (macOS-14, 1.23)
- GitHub Check: verify-devcontainer
- GitHub Check: Run OKD bundle with crc (1.23)
- GitHub Check: build (ubuntu-latest, 1.23)
🔇 Additional comments (17)
vendor/github.com/sebrandon1/grab/lib/util.go (1)
1-1: Package rename is consistent with new import path – looks good.
The switch topackage libaligns with the new module pathproxy.laosky.tk/sebrandon1/grab/liband avoids the historical/v3import confusion.vendor/github.com/sebrandon1/grab/lib/error.go (1)
1-1: Package rename acknowledged.
The change topackage libis consistent across the vendored fork – no issues spotted.vendor/github.com/sebrandon1/grab/lib/rate_limiter.go (1)
1-1: Package rename looks correct.
Nothing else changed in this file; interface remains compatible.vendor/github.com/sebrandon1/grab/lib/grab.go (1)
1-1: Package declaration updated successfully.
Importers already referenceproxy.laosky.tk/sebrandon1/grab/libwith aliasgrab, so the rename won’t break callers.vendor/github.com/sebrandon1/grab/lib/request.go (1)
1-1: Package rename verified.
All downstream types (Client,Response, etc.) remain in the same package, so type references are intact.go.mod (1)
45-45: Verify the new fork's maintenance and legitimacy.The migration to
github.com/sebrandon1/grab v0.0.3introduces a dependency on a personal fork. Please ensure:
- The fork is actively maintained and trustworthy
- Version v0.0.3 is stable and production-ready
- The fork provides clear advantages over the original library
#!/bin/bash # Description: Verify the new grab fork's maintenance status and compare with original # Check the new fork's repository details gh repo view sebrandon1/grab --json createdAt,updatedAt,stargazerCount,forkCount,hasIssuesEnabled # Check recent activity gh api repos/sebrandon1/grab/commits --jq '.[0:5] | .[] | {date: .commit.author.date, message: .commit.message}' # Compare with original repository gh repo view cavaliergopher/grab --json createdAt,updatedAt,stargazerCount,forkCountpkg/download/download.go (1)
22-22: LGTM! Import change correctly implemented.The import statement properly:
- Uses an explicit alias to maintain API compatibility
- Points to the new fork's
/libsubpackage- Aligns with the dependency change in go.mod
vendor/github.com/sebrandon1/grab/lib/doc.go (1)
2-2: LGTM! Package rename correctly implemented.The package documentation and declaration have been properly updated from
grabtolib, maintaining consistency with the new import path structure.Also applies to: 63-63
vendor/modules.txt (1)
665-667: LGTM! Vendor file correctly updated.The vendor modules file properly reflects the migration to the new grab fork with the correct module path and subpackage structure.
vendor/github.com/sebrandon1/grab/lib/response.go (6)
1-1: LGTM! Package name correctly updated.The package declaration has been properly changed to
libto match the new package structure.
6-6: Good addition of fmt import.The
fmtimport is needed for the enhanced error logging in the defer functions.
172-172: Excellent modernization using time.Since().Using
time.Since(c.Start)is more idiomatic and readable thantime.Now().Sub(c.Start).
207-207: Good migration from deprecated ioutil.Correctly updated from
ioutil.NopClosertoio.NopCloserfollowing Go's deprecation of the ioutil package.
228-235: Excellent improvement in error handling.The enhanced defer function properly handles and logs file close errors instead of silently ignoring them. The migration from
ioutil.ReadAlltoio.ReadAllis also correct.
249-251: Good error handling improvement.The defer function now explicitly handles the file close operation, which is better than the previous simple
defer f.Close()that ignored potential errors.vendor/github.com/sebrandon1/grab/lib/client.go (2)
84-84: Good improvement: buffered Done channel prevents potential deadlocks.Making the Done channel buffered is a good defensive programming practice that prevents potential deadlocks if the channel is closed before all readers are ready.
350-356: Excellent error handling improvements throughout the file.The consistent use of
fmt.Errorfwith%wverb for error wrapping provides better error context and maintains the error chain. The updated seek constants and proper resource cleanup in error paths improve code robustness.Also applies to: 463-466, 512-516, 555-567, 580-584
| type gauge interface { | ||
| Sample(t time.Time, n int64) | ||
| BPS() float64 | ||
| } |
There was a problem hiding this comment.
The gauge interface is defined but never implemented, leaving transfer rate tracking non-functional.
The code defines a gauge interface and includes a BPS() method that depends on it, but the gauge is always initialized to nil. This makes the transfer rate tracking functionality non-operational. If this is intentional, consider removing the gauge-related code entirely. Otherwise, implement the gauge interface to restore transfer rate tracking.
Would you like me to help implement the gauge interface or remove the unused code?
Also applies to: 28-28, 96-101
🤖 Prompt for AI Agents
In vendor/github.com/sebrandon1/grab/lib/transfer.go around lines 10-13, 28, and
96-101, the gauge interface is defined but never implemented or initialized,
causing transfer rate tracking to be non-functional. To fix this, either
implement the gauge interface with the required Sample and BPS methods and
initialize the gauge variable properly to enable transfer rate tracking, or
remove all gauge-related code if this functionality is not needed to avoid dead
code.
| func DownloadBatch(ctx context.Context, urls []string) (<-chan DownloadResponse, error) { | ||
| ch := make(chan DownloadResponse) | ||
| respch, err := GetBatch(0, ".", urls...) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| go func() { | ||
| for resp := range respch { | ||
| ch <- DownloadResponse{ | ||
| Filename: resp.Filename, | ||
| Err: resp.Err(), | ||
| } | ||
| } | ||
| close(ch) | ||
| }() | ||
| return ch, nil | ||
| } |
There was a problem hiding this comment.
Add context cancellation handling to prevent potential goroutine leaks.
The goroutine that forwards responses doesn't handle context cancellation. If the context is cancelled while downloads are in progress, the goroutine will continue running until all responses are processed, potentially causing resource leaks.
Consider this improved implementation:
func DownloadBatch(ctx context.Context, urls []string) (<-chan DownloadResponse, error) {
ch := make(chan DownloadResponse)
respch, err := GetBatch(0, ".", urls...)
if err != nil {
return nil, err
}
go func() {
- for resp := range respch {
- ch <- DownloadResponse{
- Filename: resp.Filename,
- Err: resp.Err(),
+ defer close(ch)
+ for {
+ select {
+ case resp, ok := <-respch:
+ if !ok {
+ return
+ }
+ select {
+ case ch <- DownloadResponse{
+ Filename: resp.Filename,
+ Err: resp.Err(),
+ }:
+ case <-ctx.Done():
+ return
+ }
+ case <-ctx.Done():
+ return
}
}
- close(ch)
}()
return ch, nil
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func DownloadBatch(ctx context.Context, urls []string) (<-chan DownloadResponse, error) { | |
| ch := make(chan DownloadResponse) | |
| respch, err := GetBatch(0, ".", urls...) | |
| if err != nil { | |
| return nil, err | |
| } | |
| go func() { | |
| for resp := range respch { | |
| ch <- DownloadResponse{ | |
| Filename: resp.Filename, | |
| Err: resp.Err(), | |
| } | |
| } | |
| close(ch) | |
| }() | |
| return ch, nil | |
| } | |
| func DownloadBatch(ctx context.Context, urls []string) (<-chan DownloadResponse, error) { | |
| ch := make(chan DownloadResponse) | |
| respch, err := GetBatch(0, ".", urls...) | |
| if err != nil { | |
| return nil, err | |
| } | |
| go func() { | |
| defer close(ch) | |
| for { | |
| select { | |
| case resp, ok := <-respch: | |
| if !ok { | |
| return | |
| } | |
| select { | |
| case ch <- DownloadResponse{ | |
| Filename: resp.Filename, | |
| Err: resp.Err(), | |
| }: | |
| case <-ctx.Done(): | |
| return | |
| } | |
| case <-ctx.Done(): | |
| return | |
| } | |
| } | |
| }() | |
| return ch, nil | |
| } |
🤖 Prompt for AI Agents
In vendor/github.com/sebrandon1/grab/lib/download.go lines 14 to 30, the
goroutine forwarding responses from respch to ch does not handle context
cancellation, risking goroutine leaks if the context is cancelled. Modify the
goroutine to select on both the context's Done channel and the respch channel,
exiting early and closing ch if the context is cancelled to prevent resource
leaks.
|
/retest |
cfergeau
force-pushed
the
replace_grab
branch
from
fa4b8e1 to
b8490df
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
pkg/download/download.go (2)
117-124: Add bounded cancellation to InMemory to avoid indefinite hangs.If the remote stalls, this path has no context/timeout. Consider a conservative timeout.
Apply this diff:
grabReq, err := grab.NewRequest("", uri) if err != nil { return nil, fmt.Errorf("failed to create grab request: %w", err) } // do not write the downloaded file to disk grabReq.NoStore = true + // bound the request to avoid indefinite hangs + ctx, cancel := context.WithTimeout(context.Background(), 2*time.Minute) + defer cancel() + grabReq = grabReq.WithContext(ctx)
75-79: Avoid panic on nil ctx; fallback to context.Background().Prevents surprising crashes and keeps behavior predictable.
- if ctx == nil { - panic("ctx is nil, this should not happen") - } - req = req.WithContext(ctx) + if ctx == nil { + ctx = context.Background() + } + req = req.WithContext(ctx)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (2)
go.mod(1 hunks)pkg/download/download.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- go.mod
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (17)
- GitHub Check: build (macOS-14, 1.24)
- GitHub Check: build (macOS-13, 1.24)
- GitHub Check: build-qe (linux, arm64)
- GitHub Check: Run OKD bundle with crc (1.24)
- GitHub Check: build-qe (linux, amd64)
- GitHub Check: build-qe (windows, amd64)
- GitHub Check: build-qe (darwin, arm64)
- GitHub Check: build-installer (windows-2022, 1.24)
- GitHub Check: build (macOS-14, 1.24)
- GitHub Check: build-qe (darwin, amd64)
- GitHub Check: build (ubuntu-22.04, 1.24)
- GitHub Check: build (ubuntu-latest, 1.24)
- GitHub Check: build (windows-2022, 1.24)
- GitHub Check: build (macOS-13, 1.24)
- GitHub Check: build (ubuntu-latest, 1.24)
- GitHub Check: build (windows-2022, 1.24)
- GitHub Check: verify-devcontainer
🔇 Additional comments (2)
pkg/download/download.go (2)
22-22: Import switch to the fork looks correct.Changing to grab "github.com/sebrandon1/grab/lib" preserves the alias, minimizing churn. No functional differences are introduced in this file by the import change itself.
25-33: grab fork API parity verified
All required symbols (Client.UserAgent/HTTPClient, Request.WithContext/SetChecksum/NoStore, Response.Done/Size/BytesComplete/Filename/Err/Open) are present in the vendored fork and go.mod correctly requires github.com/sebrandon1/grab v0.0.4 with no leftover imports or replace directives.
|
I force pushed to rebase the PR. |
|
@cfergeau I'll do a rebase and update to my branch quick. |
I’ve done the rebase. I’ve seen there’s a 0.0.5 release, but the update to go 1.25 is not something we want to do in crc just yet, so I’ve kept 0.0.4 |
sebrandon1
force-pushed
the
replace_grab
branch
from
b8490df to
9e92ae3
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
vendor/github.com/sebrandon1/grab/lib/README.md (2)
41-60: Silence markdownlint noise from tabs in code blocks (MD010).These lines contain hard tabs inside fenced Go code. Prefer excluding vendor/** from markdownlint or disabling MD010 for fenced code to avoid failing CI on vendored docs rather than editing upstream text.
If you keep linting vendor, update the linter config accordingly; otherwise confirm vendor/** is excluded.
68-90: Same MD010 tab issue in the second code block.Apply the same linter exclusion/override here rather than modifying vendored content.
vendor/github.com/sebrandon1/grab/lib/test_helpers.go (4)
1-10: Test helpers live in non-test file under vendor.These helpers import testing and compile into the vendored package. Not harmful, but ideally they’d be *_test.go upstream so they don’t ship in normal builds. No action needed in this PR; consider proposing upstream.
44-53: Minor consistency: derive ContentLength from body.The default response hardcodes 12. Consider using len("test content") for consistency with createMockHTTPResponse.
- ContentLength: 12, + ContentLength: int64(len("test content")),
145-148: Include status code in Response.Status string.Status typically includes both code and text (e.g., "404 Not Found"). Current code sets only the text.
-func createErrorResponse(statusCode int, content string) *http.Response { - status := http.StatusText(statusCode) - return createMockHTTPResponse(status, statusCode, content, nil) -} +func createErrorResponse(statusCode int, content string) *http.Response { + status := fmt.Sprintf("%d %s", statusCode, http.StatusText(statusCode)) + return createMockHTTPResponse(status, statusCode, content, nil) +}Note: would require
fmtimport.
150-165: Prefer t.Cleanup for restoring globals to be subtest/parallel-friendly.Defer works, but t.Cleanup composes better with subtests and early returns.
func withMockClient(t *testing.T, mockClient *mockHTTPClient, fn func()) { t.Helper() originalClient := DefaultClient DefaultClient = &Client{ HTTPClient: mockClient, UserAgent: "test-agent", } - defer func() { - DefaultClient = originalClient - }() + t.Cleanup(func() { DefaultClient = originalClient }) fn() }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (17)
go.mod(1 hunks)pkg/download/download.go(1 hunks)vendor/github.com/cavaliergopher/grab/v3/pkg/bps/bps.go(0 hunks)vendor/github.com/cavaliergopher/grab/v3/pkg/bps/sma.go(0 hunks)vendor/github.com/sebrandon1/grab/lib/README.md(1 hunks)vendor/github.com/sebrandon1/grab/lib/client.go(7 hunks)vendor/github.com/sebrandon1/grab/lib/doc.go(2 hunks)vendor/github.com/sebrandon1/grab/lib/download.go(1 hunks)vendor/github.com/sebrandon1/grab/lib/error.go(1 hunks)vendor/github.com/sebrandon1/grab/lib/grab.go(1 hunks)vendor/github.com/sebrandon1/grab/lib/rate_limiter.go(1 hunks)vendor/github.com/sebrandon1/grab/lib/request.go(1 hunks)vendor/github.com/sebrandon1/grab/lib/response.go(5 hunks)vendor/github.com/sebrandon1/grab/lib/test_helpers.go(1 hunks)vendor/github.com/sebrandon1/grab/lib/transfer.go(2 hunks)vendor/github.com/sebrandon1/grab/lib/util.go(1 hunks)vendor/modules.txt(1 hunks)
💤 Files with no reviewable changes (2)
- vendor/github.com/cavaliergopher/grab/v3/pkg/bps/bps.go
- vendor/github.com/cavaliergopher/grab/v3/pkg/bps/sma.go
🚧 Files skipped from review as they are similar to previous changes (13)
- vendor/github.com/sebrandon1/grab/lib/util.go
- vendor/github.com/sebrandon1/grab/lib/request.go
- vendor/github.com/sebrandon1/grab/lib/response.go
- vendor/github.com/sebrandon1/grab/lib/transfer.go
- vendor/github.com/sebrandon1/grab/lib/download.go
- vendor/github.com/sebrandon1/grab/lib/doc.go
- pkg/download/download.go
- vendor/github.com/sebrandon1/grab/lib/rate_limiter.go
- vendor/github.com/sebrandon1/grab/lib/error.go
- vendor/github.com/sebrandon1/grab/lib/grab.go
- vendor/github.com/sebrandon1/grab/lib/client.go
- vendor/modules.txt
- go.mod
🧰 Additional context used
🧬 Code graph analysis (1)
vendor/github.com/sebrandon1/grab/lib/test_helpers.go (1)
vendor/github.com/sebrandon1/grab/lib/client.go (3)
DefaultClient(63-63)Client(30-47)HTTPClient(17-19)
🪛 markdownlint-cli2 (0.17.2)
vendor/github.com/sebrandon1/grab/lib/README.md
41-41: Hard tabs
Column: 1
(MD010, no-hard-tabs)
42-42: Hard tabs
Column: 1
(MD010, no-hard-tabs)
43-43: Hard tabs
Column: 1
(MD010, no-hard-tabs)
47-47: Hard tabs
Column: 1
(MD010, no-hard-tabs)
48-48: Hard tabs
Column: 1
(MD010, no-hard-tabs)
49-49: Hard tabs
Column: 1
(MD010, no-hard-tabs)
50-50: Hard tabs
Column: 1
(MD010, no-hard-tabs)
51-51: Hard tabs
Column: 1
(MD010, no-hard-tabs)
52-52: Hard tabs
Column: 1
(MD010, no-hard-tabs)
53-53: Hard tabs
Column: 1
(MD010, no-hard-tabs)
54-54: Hard tabs
Column: 1
(MD010, no-hard-tabs)
55-55: Hard tabs
Column: 1
(MD010, no-hard-tabs)
56-56: Hard tabs
Column: 1
(MD010, no-hard-tabs)
57-57: Hard tabs
Column: 1
(MD010, no-hard-tabs)
58-58: Hard tabs
Column: 1
(MD010, no-hard-tabs)
68-68: Hard tabs
Column: 1
(MD010, no-hard-tabs)
69-69: Hard tabs
Column: 1
(MD010, no-hard-tabs)
73-73: Hard tabs
Column: 1
(MD010, no-hard-tabs)
74-74: Hard tabs
Column: 1
(MD010, no-hard-tabs)
75-75: Hard tabs
Column: 1
(MD010, no-hard-tabs)
76-76: Hard tabs
Column: 1
(MD010, no-hard-tabs)
77-77: Hard tabs
Column: 1
(MD010, no-hard-tabs)
78-78: Hard tabs
Column: 1
(MD010, no-hard-tabs)
79-79: Hard tabs
Column: 1
(MD010, no-hard-tabs)
80-80: Hard tabs
Column: 1
(MD010, no-hard-tabs)
81-81: Hard tabs
Column: 1
(MD010, no-hard-tabs)
82-82: Hard tabs
Column: 1
(MD010, no-hard-tabs)
83-83: Hard tabs
Column: 1
(MD010, no-hard-tabs)
84-84: Hard tabs
Column: 1
(MD010, no-hard-tabs)
85-85: Hard tabs
Column: 1
(MD010, no-hard-tabs)
86-86: Hard tabs
Column: 1
(MD010, no-hard-tabs)
87-87: Hard tabs
Column: 1
(MD010, no-hard-tabs)
88-88: Hard tabs
Column: 1
(MD010, no-hard-tabs)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (15)
- GitHub Check: build-qe (darwin, arm64)
- GitHub Check: build-qe (linux, amd64)
- GitHub Check: build-qe (linux, arm64)
- GitHub Check: build-installer (windows-2022, 1.24)
- GitHub Check: build (windows-2022, 1.24)
- GitHub Check: Run OKD bundle with crc (1.24)
- GitHub Check: build (windows-2022, 1.24)
- GitHub Check: build (macOS-14, 1.24)
- GitHub Check: build (ubuntu-latest, 1.24)
- GitHub Check: verify-devcontainer
- GitHub Check: build (macOS-14, 1.24)
- GitHub Check: build (ubuntu-latest, 1.24)
- GitHub Check: build (macOS-13, 1.24)
- GitHub Check: build (ubuntu-22.04, 1.24)
- GitHub Check: build (macOS-13, 1.24)
🔇 Additional comments (1)
vendor/github.com/sebrandon1/grab/lib/README.md (1)
41-44: All legacy imports removed
No instances ofproxy.laosky.tk/cavaliergopher/grab(or/v3) remain in the repo after running the provided regex search.
|
@sebrandon1: The following tests failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
Changes:
grabrepo and stripped all extraneous/outdated things out of it.lib/and accurately documents how to use it. Changed the references incrcto use the new lib.grab.Dependency Updates:
go.mod: Removed the dependencyproxy.laosky.tk/cavaliergopher/grab/v3and addedproxy.laosky.tk/sebrandon1/grab v0.0.4, indicating a shift to a forked version of the library. [1] [2]Code Adjustments:
pkg/download/download.go: Updated the import statement to use the forked librarygrab "github.com/sebrandon1/grab/lib"instead ofproxy.laosky.tk/cavaliergopher/grab/v3.Summary by Sourcery
Replace the outdated
cavaliergopher/grab/v3dependency with a custom, maintained forksebrandon1/graband update code references and vendor files accordingly.Enhancements:
github.com/sebrandon1/grab.Chores:
cavaliergopher/grab/v3dependency from go.mod and addproxy.laosky.tk/sebrandon1/grab v0.0.4.pkg/download/download.goto use the new grab library.Summary by CodeRabbit
New Features
Bug Fixes
Chores
Documentation