build: resolve Bazel Go deps through GOPROXY#69503
Conversation
ti-chi-bot
Bot
added
do-not-merge/needs-linked-issue
do-not-merge/needs-tests-checked
do-not-merge/release-note-label-needed
|
Hi @dillon-zheng. Thanks for your PR. I'm waiting for a pingcap 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. DetailsInstructions 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. |
|
Welcome @dillon-zheng! |
ti-chi-bot
Bot
added
the
first-time-contributor
📝 WalkthroughWalkthroughRemoves GCS-backed mirror upload functionality from ChangesMirror upload removal and GOPROXY migration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
dillon-zheng
changed the title
|
/ok-to-test |
|
@dillon-zheng: Cannot trigger testing until a trusted user reviews the PR and leaves an DetailsIn response to this:
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. |
![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 `@cmd/mirror/mirror.go`:
- Around line 282-291: Handle local-path replaces in dumpNewDepsBzl: the current
lookup of downloaded[replaced.Path] assumes every replacement was downloaded,
but local-path replaces like ./pkg/parser are skipped by downloadZips and will
not exist in that map. Update dumpNewDepsBzl (around the replaced.Path /
downloaded lookup logic) to either skip local-path replacements explicitly or
branch to handle them without requiring a downloaded entry, while keeping the
existing behavior for remote modules.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: bfbec787-2b7f-4bbc-a454-62ba5543a2a1
📒 Files selected for processing (5)
DEPS.bzlMakefilecmd/mirror/BUILD.bazelcmd/mirror/mirror.gocmd/mirror/skylarkutil.go
💤 Files with no reviewable changes (1)
- cmd/mirror/skylarkutil.go
| d, ok := downloaded[replaced.Path] | ||
| if !ok { | ||
| return fmt.Errorf("could not find downloaded module for %s@%s", replaced.Path, replaced.Version) | ||
| } | ||
| if mod.Replace != nil { | ||
| fmt.Printf(" replace = \"%s\",\n", replaced.Path) | ||
| } | ||
| fmt.Printf(` sum = "%s", | ||
| version = "%s", | ||
| `, d.Sum, d.Version) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find any replace directives, highlighting local-path (no version) replaces
rg -n '=>' go.mod pkg/parser/go.mod || echo "no replace directives found"Repository: pingcap/tidb
Length of output: 680
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the generator logic around downloadZips and dumpNewDepsBzl.
ast-grep outline cmd/mirror/mirror.go --view expanded
printf '\n--- relevant slices ---\n'
sed -n '90,160p' cmd/mirror/mirror.go
printf '\n---\n'
sed -n '250,320p' cmd/mirror/mirror.go
printf '\n--- replace handling in go.mod / parser go.mod context ---\n'
rg -n '^\s*replace\s+|=>\s+\.{1,2}/|=>\s+\./' go.mod pkg/parser/go.modRepository: pingcap/tidb
Length of output: 5229
Handle local-path replaces in dumpNewDepsBzl
go.mod already has github.com/pingcap/tidb/pkg/parser => ./pkg/parser, and downloadZips skips local-path replaces while dumpNewDepsBzl still looks up downloaded[replaced.Path]. That makes make bazel_mirror fail on the current tree unless local replaces are skipped or handled explicitly.
🤖 Prompt for 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.
In `@cmd/mirror/mirror.go` around lines 282 - 291, Handle local-path replaces in
dumpNewDepsBzl: the current lookup of downloaded[replaced.Path] assumes every
replacement was downloaded, but local-path replaces like ./pkg/parser are
skipped by downloadZips and will not exist in that map. Update dumpNewDepsBzl
(around the replaced.Path / downloaded lookup logic) to either skip local-path
replacements explicitly or branch to handle them without requiring a downloaded
entry, while keeping the existing behavior for remote modules.
70ce59f to
c7f2d4d
Compare
dillon-zheng
changed the title
|
Addressed the follow-up review points:
Additional validation after the update:
|
|
/ok-to-test |
ti-chi-bot
Bot
added
ok-to-test
|
/retest |
dillon-zheng
force-pushed
the
codex/use-goproxy-for-bazel-go-deps
branch
from
c7f2d4d to
3f295a6
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #69503 +/- ##
================================================
- Coverage 76.3257% 74.4734% -1.8524%
================================================
Files 2041 2044 +3
Lines 561045 574288 +13243
================================================
- Hits 428222 427692 -530
- Misses 131922 146352 +14430
+ Partials 901 244 -657
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
/skip-issue-check |
ti-chi-bot
Bot
added
release-note-none
|
/cc cfzjywxk |
|
/assign cfzjywxk |
|
/cc D3Hunter |
ti-chi-bot
Bot
added
lgtm
and removed
needs-1-more-lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cfzjywxk, D3Hunter, JmPotato, YangKeao The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/hold |
ti-chi-bot
Bot
added
the
do-not-merge/hold
|
Please do not remove sha256, because once it is removed, stable network downloads via goproxy cannot be guaranteed. It will also cause repeated downloads and prevent cache. Therefore, this PR must absolutely not be merged. |
|
Thanks for pointing this out. I agree that removing But this does not remove integrity verification. The generated The intent of this PR is to migrate Go module dependencies from the manually maintained pingcapmirror/static archive cache to the standard Go module resolution path. In CI, So I think the real tradeoff is:
|
|
Do whatever you want, I still stand by my point. The previous migration to the current model was due to goproxy instability. /unhold |
ti-chi-bot
Bot
removed
the
do-not-merge/hold
7 participants
Issue Number: close #69513
Summary
cmd/mirrorto generatego_repositoryentries withsumandversioninstead of mirroredurls,sha256, andstrip_prefixpingcapmirrorGo module upload path and deprecatedbazel_mirror_uploadDEPS.bzlso Bazel Go dependencies are resolved by rules_go through the configured Go module environment, e.g.GOPROXY=...|...,directTest Plan
make tidygo test ./cmd/mirrorPATH=/tmp/tidb-bazel-shim:$PATH make bazel_preparePATH=/tmp/tidb-bazel-shim:$PATH make bazel_mirror_uploadPATH=/tmp/tidb-bazel-shim:$PATH make lintgit -c core.whitespace=-tab-in-indent diff --checkDEPS.bzlhas nopingcapmirror,cache.hawkingrei.com,urls,sha256, orstrip_prefixentries for Go modulesTests
Release note