crc icon indicating copy to clipboard operation
crc copied to clipboard

go.mod: replace old 'grab' dependency

Open sebrandon1 opened this issue 5 months ago • 5 comments
trafficstars

Changes:

  • Forked and created my own maintained version of the grab repo and stripped all extraneous/outdated things out of it.
  • Old repo was stuck on Go 1.14 with multiple go.mod files within different folders. Very confusing and an old way of organizing a Go project.
  • New repo organization puts all public functions inside of lib/ and accurately documents how to use it. Changed the references in crc to use the new lib.
  • New repo has a binary that you can build and download files via grab.
  • New repo has dependencies managed by dependabot.

Dependency Updates:

  • go.mod: Removed the dependency github.com/cavaliergopher/grab/v3 and added github.com/sebrandon1/grab v0.0.3, 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 library grab "github.com/sebrandon1/grab/lib" instead of github.com/cavaliergopher/grab/v3.

Summary by Sourcery

Replace the outdated cavaliergopher/grab/v3 dependency with a custom, maintained fork sebrandon1/grab and update code references and vendor files accordingly.

Enhancements:

  • Migrate to a streamlined fork of the grab library under github.com/sebrandon1/grab.
  • Refresh the vendor directory to include the new grab fork and remove old package files.

Chores:

  • Remove the old cavaliergopher/grab/v3 dependency from go.mod and add github.com/sebrandon1/grab v0.0.3.
  • Update import paths in pkg/download/download.go to use the new grab library.

sebrandon1 avatar Jun 16 '25 20:06 sebrandon1

Reviewer's Guide

Replaces 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 usage

classDiagram
    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

Change Details Files
Updated module dependencies to use forked grab
  • Removed github.com/cavaliergopher/grab/v3 from go.mod
  • Added github.com/sebrandon1/grab v0.0.1 to go.mod
go.mod
Adjusted import in download package to new library path
  • Replaced import of cavaliergopher/grab/v3
  • Imported sebrandon1/grab/lib under alias grab
pkg/download/download.go
Refreshed vendor directory for the new fork
  • Removed old grab/v3 files from vendor
  • Added vendor files for sebrandon1/grab/lib, including README and download.go
  • Updated vendor/modules.txt to include the new dependency
vendor/github.com/cavaliergopher/grab/v3/*
vendor/github.com/sebrandon1/grab/lib/*
vendor/modules.txt
go.sum

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an issue from a review comment by replying to it. You can also reply to a review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull request title to generate a title at any time. You can also comment @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in the pull request body to generate a PR summary at any time exactly where you want it. You can also comment @sourcery-ai summary on the pull request to (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the pull request to resolve all Sourcery comments. Useful if you've already addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull request to dismiss all existing Sourcery reviews. Especially useful if you want to start fresh with a new review - don't forget to comment @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

  • Contact our support team for questions or feedback.
  • Visit our documentation for detailed guides and information.
  • Keep in touch with the Sourcery team by following us on X/Twitter, LinkedIn or GitHub.

sourcery-ai[bot] avatar Jun 16 '25 20:06 sourcery-ai[bot]

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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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.

openshift-ci[bot] avatar Jun 16 '25 20:06 openshift-ci[bot]

/ok-to-test

praveenkumar avatar Jun 17 '25 04:06 praveenkumar

Walkthrough

Replaced vendored dependency github.com/cavaliergopher/grab/v3 with github.com/sebrandon1/grab v0.0.4, updated imports to .../grab/lib, added a new lib package (including DownloadBatch), improved resource/error handling, and removed the prior BPS gauge and its vendor code.

Changes

Cohort / File(s) Change Summary
Module files
go.mod, vendor/modules.txt
Replaced module github.com/cavaliergopher/grab/v3 with github.com/sebrandon1/grab (v0.0.4); updated vendor/module listings.
Project import
pkg/download/download.go
Switched import from github.com/cavaliergopher/grab/v3 to github.com/sebrandon1/grab/lib (aliased grab); no other logic changes.
Removed vendor: bps
vendor/github.com/cavaliergopher/grab/v3/pkg/bps/*
Deleted BPS package files (bps.go, sma.go) — removed Gauge, SampleFunc, Watch, and SMA implementation.
Added vendored grab/lib package
vendor/github.com/sebrandon1/grab/lib/...
Added new package files and README; package renamed to lib; introduced DownloadBatch and DownloadResponse; many files updated for error handling and package rename.
Transfer / rate logic
vendor/.../grab/lib/transfer.go
Removed dependency on external bps package, added internal gauge interface, set gauge to nil, and removed the background BPS Watch goroutine.
Client & resource handling
vendor/.../grab/lib/client.go, .../response.go, .../download.go
Improved error capture on closing bodies/writers, checked/trapped Truncate and remove errors, updated seek constants, and added DownloadBatch helper.
Package docs & renames
vendor/.../grab/lib/doc.go, error.go, grab.go, rate_limiter.go, request.go, util.go, README.md
Changed package declaration from grablib, adjusted imports/usages, and added README documenting new lib API.
Tests / helpers
vendor/.../grab/lib/test_helpers.go
Added testing utilities for mock HTTP client, temp directory management, response builders, and test client helpers.

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

I hopped through modules, clipped an old vine,
Switched my grab-paths to a friendlier line.
I tidy the bytes and guard every close,
Batch downloads now sprout where the carrot grows.
🐇📦

Pre-merge checks (1 passed, 2 warnings)

❌ Failed checks (2 warnings)
Check name Status Explanation Resolution
Description Check ⚠️ Warning The pull request description does not follow the required repository template: it lacks the “## Description” heading with issue links (Fixes/Relates to), the “## Type of change” section with checkboxes, the “## Proposed changes” breakdown, the “## Testing” plan, and the contribution checklist. Please update the description to use the repository’s template by adding a “## Description” section summarizing the PR and linking relevant issues, fill in the “Fixes” or “Relates to” lines, complete the “## Type of change” checkboxes, provide “## Proposed changes” and “## Testing” details, and include the contribution checklist.
Docstring Coverage ⚠️ Warning Docstring coverage is 63.64% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (1 passed)
Check name Status Explanation
Title Check ✅ Passed The title succinctly conveys the central change—replacing the grab dependency in go.mod—and is clear, concise, and focused on the main purpose of the changeset.

[!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.

  • Built-in checks – Quickly apply ready-made checks to enforce title conventions, require pull request descriptions that follow templates, validate linked issues for compliance, and more.
  • Custom agentic checks – Define your own rules using CodeRabbit’s advanced agentic capabilities to enforce organization-specific policies and workflows. For example, you can instruct CodeRabbit’s agent to verify that API documentation is updated whenever API schema files are modified in a PR. Note: Upto 5 custom checks are currently allowed during the preview period. Pricing for this feature will be announced in a few weeks.

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 Docstrings
🧪 Generate unit tests
  • [ ] Create PR with unit tests
  • [ ] Post copyable unit tests in a comment

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

coderabbitai[bot] avatar Jun 23 '25 22:06 coderabbitai[bot]

/retest

sebrandon1 avatar Jul 07 '25 15:07 sebrandon1

I force pushed to rebase the PR. Imo we could give it a go (ie merge it)

cfergeau avatar Sep 09 '25 13:09 cfergeau

@cfergeau I'll do a rebase and update to my branch quick.

sebrandon1 avatar Sep 09 '25 13:09 sebrandon1

@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

cfergeau avatar Sep 09 '25 13:09 cfergeau

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: Once this PR has been reviewed and has the lgtm label, please assign cfergeau for approval. For more information see the Code Review Process.

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 /approve in a comment Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci[bot] avatar Sep 09 '25 14:09 openshift-ci[bot]

@sebrandon1: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/security 9e92ae3b2e2e331f80720880d31f882aa7312d5c link false /test security
ci/prow/e2e-crc 9e92ae3b2e2e331f80720880d31f882aa7312d5c link true /test e2e-crc

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.

openshift-ci[bot] avatar Sep 09 '25 16:09 openshift-ci[bot]

What about this PR?

gbraad avatar Oct 13 '25 01:10 gbraad