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
|
|
[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 |
|
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
WalkthroughThe changes replace the dependency on Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant grab/lib
participant OS/FileSystem
participant Network
Caller->>grab/lib: DownloadBatch(ctx, urls)
grab/lib->>grab/lib: GetBatch(ctx, urls, ...)
grab/lib->>Network: Download files concurrently
Network-->>grab/lib: File data streams
grab/lib->>OS/FileSystem: Write files to disk
OS/FileSystem-->>grab/lib: Write complete
grab/lib-->>Caller: DownloadResponse (via channel)
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms (17)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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 pathgithub.jpy.wang/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 referencegithub.jpy.wang/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.
|
@sebrandon1: The following test 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 dependencygithub.jpy.wang/cavaliergopher/grab/v3and addedgithub.jpy.wang/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 ofgithub.jpy.wang/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 addgithub.jpy.wang/sebrandon1/grab v0.0.4.pkg/download/download.goto use the new grab library.Summary by CodeRabbit
New Features
Bug Fixes
Chores
Documentation