[go] Avoid duplicate bundler license downloads - #2453
Conversation
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The focused implementation matches the stated behavior and includes appropriate unit coverage.
Review tier: Balanced
Findings: None
What changed in this PR
Updates the Go CLI bundler to resolve npm tarballs through registry metadata rather than constructed URLs.
Changes:
- Adds validated
dist.tarballmetadata resolution. - Downloads the shared CLI license once after platform bundles finish.
- Adds metadata resolution and validation tests.
| File | Description |
|---|---|
go/cmd/bundler/main.go |
Resolves tarball URLs and deduplicates license downloads. |
go/cmd/bundler/main_test.go |
Tests metadata URL resolution and missing tarballs. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
Thanks for this. The unit tests are solid and the license-download dedup is a real win. On the metadata-resolution part: this changes only Go to decouple from the hardcoded npm tarball URL pattern, while .NET, Python, and Rust keep using that same pattern. If the URL convention is at genuine risk of changing, it needs to be fixed everywhere; if not, the metadata resolution adds complexity without benefit. Cross-language consistency is the blockerThe exact same hardcoded URL-construction pattern is deliberately mirrored in three other language SDKs:
Fixing only Go creates inconsistency: if npm's convention ever changes, only Go survives; the other three break silently. This directly opposes the repo's stated goal of consistent patterns across all 6 language SDKs. Proposed changes
SuggestionSplit the license-dedup optimization into its own focused PR so it can merge independently. |
aac4c8a to
0b208dc
Compare
|
Thanks, agreed. I checked the published npm metadata before deciding: all 812 I have force-pushed a repurposed version of this PR that drops metadata resolution and retains only the license-download deduplication. If tarball URL handling needs to change later, it should be handled consistently across Go, .NET, Python, and Rust. |
SteveSandersonMS
left a comment
There was a problem hiding this comment.
Thanks for the quick update. I re-reviewed the revised PR and validated it locally. The metadata-resolution divergence is gone, and the remaining license-download dedup is focused, low-risk, and useful. This version looks good to merge.
Summary
@github/copilotlicense once, after all requested platform bundles are built.Validation
go test ./cmd/bundler -count=1go test ./internal/embeddedcli -count=1go vet ./cmd/bundler