chore: change build producer in build + fix lint#328
Conversation
8b7edca to
4e7dfa8
Compare
There was a problem hiding this comment.
Pull request overview
This PR updates the root Go module dependency setup to stop using a local replace for the producer submodule and adjusts golangci-lint configuration to address a make lint failure by excluding an additional Redis implementation file from gochecknoglobals.
Changes:
- Remove the root
go.modlocalreplacedirective forgithub.com/llm-d/llm-d-async/producer => ./producer. - Add
go.sumentries forgithub.com/llm-d/llm-d-async/producer v0.7.4now that it’s resolved as a normal module dependency. - Update
.golangci.ymlto excludepkg/redis/sortedset_impl.gofromgochecknoglobals(via a combined path regex).
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
go.mod |
Removes the local replace for the producer submodule so it can be resolved as a normal module dependency. |
go.sum |
Records module checksums for producer v0.7.4 after switching away from local replacement. |
.golangci.yml |
Expands gochecknoglobals exclusions to include sortedset_impl.go to fix lint failures. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| replace github.com/llm-d/llm-d-async/api => ./api | ||
|
|
||
| replace github.com/llm-d/llm-d-async/producer => ./producer |
There was a problem hiding this comment.
@vishbhat
should we also remove those?
replace github.com/llm-d/llm-d-async/api => ./api
replace github.com/llm-d/llm-d-async/pipeline => ./pipeline
There was a problem hiding this comment.
I think api and pipeline are being used in some impl. We need it during dev, so that that changes in api and pipeline are immediately picked up.
For producer its fine because the only import is in e2e_test.go
There was a problem hiding this comment.
One item which we might need to handle: test/e2e/e2e_test.go still imports github.com/llm-d/llm-d-async/producer. After this change, it will resolve from the published v0.7.4 module instead of the local ./producer source. This is fine for CI, but if someone is iterating on both producer/ and e2e tests locally, they'll need to temporarily re-add the replace directive to pick up their local changes.
There was a problem hiding this comment.
you should remove replace, and use go.work to use local module for development
There was a problem hiding this comment.
go.work will like this, then you don't need hacking go.mod with replace
go 1.25.8
use (
.
./api
./pipeline
./producer
)
There was a problem hiding this comment.
correct, but i think this is how was designed at the beginning to make these modulization.
There was a problem hiding this comment.
a different topic, we should bump go to 1.26 sooner than later
There was a problem hiding this comment.
changes bit for api and pipeline to use go.work now
- root module used a local `replace github.com/llm-d/llm-d-async/producer => ./producer` can add as import dependency instead of replace by local changes Signed-off-by: Wen Zhou <wenzhou@redhat.com>
The cleanupRequestStateScript Lua script global follows the same package-level redis.NewScript pattern already grandfathered for pkg/redis/redisimpl.go. Fold sortedset_impl.go into that existing gochecknoglobals exclusion so make lint passes. Signed-off-by: Wen Zhou <wenzhou@redhat.com>
in go.mod - go.wok indicates use local api and pipeline - need explicitly set GOWORK to off for "producer" when run "go" cli since we do not want it be in go.wok (use local) but still can verify the code locally if needed - image build gets go.work and local api pipeline + download producer from pinned version Signed-off-by: Wen Zhou <wenzhou@redhat.com>
8f2303a to
f38da95
Compare
shimib
left a comment
There was a problem hiding this comment.
The go.work approach is a nice improvement over the replace directives — api/pipeline resolve to local source for dev without version-skew, and it's idiomatic. Dockerfile (drops COPY producer/, which cmd/main.go never needs) and the GOWORK=off producer targets both look right.
One suggestion, picking up the thread above: add ./producer to the workspace too —
use (
.
./api
./pipeline
./producer
)
test/e2e/e2e_test.go imports producer, so as written the e2e exercises the published v0.7.4 rather than local ./producer (as @vishbhat noted). Adding it to go.work restores the local co-dev/e2e loop at no cost — cd producer && GOWORK=off go test still validates producer standalone against its published deps, so you keep the "works as a released module" check either way. There isn't a strong reason to single producer out.
Minor (also flagged by Copilot): a targeted //nolint:gochecknoglobals on the specific cleanupRequestStateScript var in sortedset_impl.go would preserve lint signal for any future globals vs. excluding the whole file — though the file-level exclusion is consistent with the existing redisimpl.go rule, so either is fine.
- keep producer in go.work, drop it in Dockerfile build Signed-off-by: Wen Zhou <wenzhou@redhat.com>
thanks @shimib for the review
as for (2): |
|
/approve |
|
Heads-up before we cut v0.8.0: dropping the root Root cause. Proof (on current main, bumping the root requires to v0.8.0):
( Impact on releasing:
Dockerfile wrinkle. Simply re-adding the root replaces isn't sufficient on its own: this PR's Dockerfile now Options (your call — you designed the go.work migration):
Happy to open whichever PR you prefer. |
|
Option 2 (tag-first) would require a 2-step release flow, which adds complexity. Option 1 seems simpler: I prefer that option. |
llm-d#328 replaced the root replace directives with go.work and made the Dockerfile drop ./producer. But go.work overrides module *source*, not the module-graph version requirement, and `go mod tidy`/`docker build` ignore it — so any bump to a not-yet-tagged version (e.g. v0.8.0) fails to build (PR container-build, release image build, and `make set-version`). Restore the three root replace directives and revert the Dockerfile to COPY producer/ so all siblings resolve locally without needing tags. go.work is kept for IDE/dev; llm-d#328's lint fix and Makefile tweaks are retained. See llm-d#328 for the broader discussion. Signed-off-by: Shimi Bandiel <shimib@google.com>
|
Following up on the release-flow breakage above: I've had to partially revert this PR in #348 (the v0.8.0 release PR), because without it v0.8.0 can't build or release at all. What #348 reverts:
Why a targeted revert instead of a full Root cause recap for the record:
|
sounds reasonable, thanks! |
What does this PR do?
replace github.com/llm-d/llm-d-async/producer => ./producercan add as import dependency instead of replace by local changesWhy is this change needed?
/enhancement
How was this tested?
Checklist
git commit -s) per DCOmake test)make lint)Related Issues
Release note
cc @vishbhat @yizhaodev