fix(EC-1778): use go-gather WithTransport options for HTTP transports#3327
Open
robnester-rh wants to merge 8 commits into
Open
fix(EC-1778): use go-gather WithTransport options for HTTP transports#3327robnester-rh wants to merge 8 commits into
robnester-rh wants to merge 8 commits into
Conversation
reference: EC-1778 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Resolve three gaps: explicit sync.OnceFunc wiring, concrete TestOCIClientConfiguration strategy, import alias conventions. reference: EC-1778 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
7 tasks: go.mod update, production rewrite, 4 test updates, final verification. TDD-sequenced with exact code and commands. reference: EC-1778 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
reference: EC-1778 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Replace removed goci.Transport/ghttp.Transport global mutations with constructed gatherer instances using WithTransport functional options. gatherFunc now dispatches via Matcher (OCI, HTTP) before falling back to the registry for git/file sources. reference: EC-1778 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Replace references to removed goci.Transport/ghttp.Transport globals with gatherer var cleanup. TestOCIClientConfiguration simplified to type assertion (transport is private). TestHTTPClientConfiguration inspects httpGatherer.Client.Transport instead of removed global. TestHTTPTracing calls httpGatherer.Gather directly since 127.0.0.1 now matches the OCI registry pattern in the new dispatch order. reference: EC-1778 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
reference: EC-1778 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThis PR migrates the CLI downloader to use ChangesDownloader WithTransport Migration to go-gather v1.2.0
Sequence DiagramsequenceDiagram
participant gatherFunc
participant initialize
participant ociGatherer
participant httpGatherer
participant registry
gatherFunc->>initialize: call _initialize()
initialize->>initialize: build base RoundTripper (with optional tracing)
initialize->>ociGatherer: NewOCIGatherer(WithTransport(retry))
initialize->>httpGatherer: NewHTTPGatherer(WithTransport(retry))
gatherFunc->>ociGatherer: Matcher(source)
alt OCI match
ociGatherer->>ociGatherer: Gather(source)
else HTTP match
gatherFunc->>httpGatherer: Matcher(source)
httpGatherer->>httpGatherer: Gather(source)
else fallback
gatherFunc->>registry: GetGatherer(source)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Tools execution failed with the following error: Failed to run tools: 13 INTERNAL: Received RST_STREAM with code 2 (Internal server error) Comment |
Review Summary by QodoMigrate downloader to go-gather v1.2.0 WithTransport API
WalkthroughsDescription• Migrate from removed goci.Transport/ghttp.Transport globals to WithTransport functional options • Update go-gather dependency from v1.1.0 to v1.2.0 • Implement Matcher-based dispatch in gatherFunc for OCI/HTTP sources • Refactor _initialize to construct gatherer instances with transport stack • Update all downloader tests to work with new gatherer variable pattern Diagramflowchart LR
A["go-gather v1.2.0"] -->|"WithTransport options"| B["_initialize"]
B -->|"constructs"| C["ociGatherer"]
B -->|"constructs"| D["httpGatherer"]
E["gatherFunc"] -->|"Matcher dispatch"| C
E -->|"Matcher dispatch"| D
E -->|"fallback"| F["registry"]
G["Transport Stack"] -->|"tracing + retry"| B
File Changes1. internal/downloader/downloader.go
|
Code Review by Qodo
1.
|
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/downloader/downloader.go`:
- Around line 55-59: The current routing checks ociGatherer.Matcher(source)
before httpGatherer.Matcher(source), which can misroute HTTP URLs (e.g.,
127.0.0.1) to the OCI gatherer; modify the gather routing so HTTP(s) URLs are
matched first—either by placing the httpGatherer.Matcher(source) case before
ociGatherer.Matcher(source) in the switch or by adding an explicit scheme check
(strings.HasPrefix(source,"http://") || strings.HasPrefix(source,"https://"))
that routes to httpGatherer.Gather(ctx, source, destination) before falling back
to ociGatherer.Gather(ctx, source, destination); update the switch in the gather
function where ociGatherer.Matcher and httpGatherer.Matcher are referenced to
implement this change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: 99df4c82-897b-45b9-b2b8-021d7cf07f26
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (5)
docs/superpowers/plans/2026-05-29-ec1778-downloader-withtransport.mddocs/superpowers/specs/2026-05-28-ec1778-downloader-withtransport-design.mdgo.modinternal/downloader/downloader.gointernal/downloader/downloader_test.go
![qodo-for-conforma[bot]](https://avatars.githubusercontent.com/in/3609154?s=60&v=4){kind=small}
…names The OCI Matcher matches localhost/127.0.0.1 as known registries, which misroutes git-over-HTTP sources hosted on localhost in acceptance tests. Dispatch to custom gatherers only for explicit oci:// or http(s):// scheme prefixes; bare hostnames fall through to the registry where git matchers are checked before OCI matchers. reference: EC-1778 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests.
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Contributor
Author
|
/retest |
1 similar comment
Contributor
Author
|
/retest |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
goci.Transport/ghttp.Transportglobal mutations withWithTransportfunctional options on constructed gatherer instancesgatherFuncnow dispatches OCI/HTTP sources viaMatcherbefore falling back to the registry for git/file sourcesTest plan
go test -race -tags=unit ./internal/downloader/— all 6 tests passmake test— all unit, integration, and generative tests pass across the CLIresolves: EC-1778
🤖 Generated with Claude Code