-
Notifications
You must be signed in to change notification settings - Fork 9.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Minimal changes to enable a Go workspace #19423
base: main
Are you sure you want to change the base?
Conversation
Skipping CI for Draft Pull Request. |
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: ivanvc 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 |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted filessee 30 files with indirect coverage changes @@ Coverage Diff @@
## main #19423 +/- ##
==========================================
+ Coverage 68.89% 68.95% +0.05%
==========================================
Files 420 420
Lines 35753 35753
==========================================
+ Hits 24632 24652 +20
+ Misses 9698 9673 -25
- Partials 1423 1428 +5 Continue to review full report in Codecov by Sentry.
|
/test all |
d555146
to
b7e86a4
Compare
Introduce a new Go workspace that references all the current submodules. To preserve the current behavior, point all of the current FORBIDDEN_DEPENCENCY virtual dependencies to the same path. Add scripts/update_go_workspace.sh that generates the Go workspace file (go.work) based on the submodules found in the project and creates the go.work.sum file after running go mod download. Added this script to the fix Makefile target. Even though, the number of modules in the etcd repository is small, by adding an automated way of updating the go.work modules future proofs the project in case there are new modules or removal of them. Point all forbidden dependencies to a common virtual place (top-level FORBIDDEN_DEPENDENCY). For the workspace to function we need all dependencies to be consistent across the repository. That means that the current approach with FORBIDDEN_DEPENDENCY doesn't work, because they are pointing to different directories. Set the fictional v3.999.999 version for these dependencies, so they are consitent, and they don't clash with the actual depdendencies references in other modules. Co-authored-by: Tim Hockin <[email protected]> Signed-off-by: Ivan Valdes <[email protected]>
Introduce a new `verify-go-workspace` make target executed by the `verify` target. The pull-etcd-verify presubmit prow job will execute it. It ensures that the Go workspace (`go.work` and `go.work.sum`) is in sync. Signed-off-by: Ivan Valdes <[email protected]>
Running `go list -m`, now returns all the submodules from the reposiroty. Which gives false negative errors when running the check. Replace it with `go mod edit -json`. Signed-off-by: Ivan Valdes <[email protected]>
Add checking for the Go toolchain directive in the go.work file too. Signed-off-by: Ivan Valdes <[email protected]>
Remove `GOFLAGS=-mod=mod` from the execution of license-bill-of-materials. Signed-off-by: Ivan Valdes <[email protected]>
Replace `go list` with `go mod edit`. Signed-off-by: Ivan Valdes <[email protected]>
b7e86a4
to
eb81e5b
Compare
/cc @ahrtr @serathius |
@@ -508,7 +508,7 @@ function bom_pass { | |||
|
|||
function dump_deps_of_module() { | |||
local module | |||
if ! module=$(run go list -m); then | |||
if ! module=$(run go mod edit -json | jq -r .Module.Path); then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we merge it independent to migration to workspaces as a separate PR? Having less changes in the main PR will increase confidence during review.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great idea. Thanks, Marek 🙇. I'll break down this pull request even further, as most of the commits can coexist with the current codebase.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I opened #19463.
@@ -14,7 +14,7 @@ function bom_fixlet { | |||
# shellcheck disable=SC2207 | |||
modules=($(modules_for_bom)) | |||
|
|||
if GOFLAGS=-mod=mod run_go_tool "github.com/appscodelabs/license-bill-of-materials" \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GOFLAGS=-mod=mod
is this even a correct flag? Could we remove it regardless of workspace migration?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I think so. This should predate Go 1.1113.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, never mind. Actually, I believe, due to the note in bom_pass
, we may need to leave the -mod=mod
flag in the tests. But you're right. There's no need for it to be here, as test_lib.sh
doesn't define a GOFLAGS=-mod
that we need to override. I'll open a pull request for this change.
@@ -31,7 +31,7 @@ function update_module_version() { | |||
local v2version="${2}" | |||
local modules | |||
run go mod tidy | |||
modules=$(run go list -f '{{if not .Main}}{{if not .Indirect}}{{.Path}}{{end}}{{end}}' -m all) | |||
modules=$(go mod edit -json | jq -r '.Require[] | select(.Indirect == null) | .Path') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When I run the two commands now I get different results, is that different when we enable workspaces?
$ go list -f '{{if not .Main}}{{if not .Indirect}}{{.Path}}{{end}}{{end}}' -m all | sort
github.com/anishathalye/porcupine
github.com/coreos/go-semver
github.com/golang/protobuf
github.com/google/go-cmp
github.com/grpc-ecosystem/go-grpc-middleware
github.com/grpc-ecosystem/go-grpc-middleware/providers/prometheus
github.com/grpc-ecosystem/grpc-gateway/v2
github.com/prometheus/client_golang
github.com/prometheus/client_model
github.com/prometheus/common
github.com/soheilhy/cmux
github.com/stretchr/testify
go.etcd.io/etcd/api/v3
go.etcd.io/etcd/client/pkg/v3
go.etcd.io/etcd/client/v2
go.etcd.io/etcd/client/v3
go.etcd.io/etcd/etcdctl/v3
go.etcd.io/etcd/etcdutl/v3
go.etcd.io/etcd/pkg/v3
go.etcd.io/etcd/server/v3
go.etcd.io/gofail
go.etcd.io/raft/v3
golang.org/x/crypto
golang.org/x/sync
golang.org/x/time
google.golang.org/grpc
google.golang.org/protobuf
go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/otelgrpc
go.opentelemetry.io/otel
go.opentelemetry.io/otel/sdk
go.opentelemetry.io/otel/trace
go.opentelemetry.io/proto/otlp
go.uber.org/zap
$ go mod edit -json | jq -r '.Require[] | select(.Inderect == null) | .Path' | sort
github.com/anishathalye/porcupine
github.com/beorn7/perks
github.com/bgentry/speakeasy
github.com/cenkalti/backoff/v4
github.com/cespare/xxhash/v2
github.com/cheggaaa/pb/v3
github.com/coreos/go-semver
github.com/coreos/go-systemd/v22
github.com/creack/pty
github.com/davecgh/go-spew
github.com/dustin/go-humanize
github.com/fatih/color
github.com/gogo/protobuf
github.com/golang/groupcache
github.com/golang-jwt/jwt/v5
github.com/golang/protobuf
github.com/go-logr/logr
github.com/go-logr/stdr
github.com/google/btree
github.com/google/go-cmp
github.com/google/uuid
github.com/gorilla/websocket
github.com/grpc-ecosystem/go-grpc-middleware
github.com/grpc-ecosystem/go-grpc-middleware/providers/prometheus
github.com/grpc-ecosystem/go-grpc-middleware/v2
github.com/grpc-ecosystem/grpc-gateway/v2
github.com/inconshreveable/mousetrap
github.com/jonboulle/clockwork
github.com/klauspost/compress
github.com/mattn/go-colorable
github.com/mattn/go-isatty
github.com/mattn/go-runewidth
github.com/munnerz/goautoneg
github.com/olekukonko/tablewriter
github.com/pmezard/go-difflib
github.com/prometheus/client_golang
github.com/prometheus/client_model
github.com/prometheus/common
github.com/prometheus/procfs
github.com/rivo/uniseg
github.com/sirupsen/logrus
github.com/soheilhy/cmux
github.com/spf13/cobra
github.com/spf13/pflag
github.com/stretchr/testify
github.com/tmc/grpc-websocket-proxy
github.com/VividCortex/ewma
github.com/xiang90/probing
go.etcd.io/bbolt
go.etcd.io/etcd/api/v3
go.etcd.io/etcd/client/pkg/v3
go.etcd.io/etcd/client/v2
go.etcd.io/etcd/client/v3
go.etcd.io/etcd/etcdctl/v3
go.etcd.io/etcd/etcdutl/v3
go.etcd.io/etcd/pkg/v3
go.etcd.io/etcd/server/v3
go.etcd.io/gofail
go.etcd.io/raft/v3
golang.org/x/crypto
golang.org/x/net
golang.org/x/sync
golang.org/x/sys
golang.org/x/text
golang.org/x/time
google.golang.org/genproto/googleapis/api
google.golang.org/genproto/googleapis/rpc
google.golang.org/grpc
google.golang.org/protobuf
go.opentelemetry.io/auto/sdk
go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/otelgrpc
go.opentelemetry.io/otel
go.opentelemetry.io/otel/exporters/otlp/otlptrace
go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracegrpc
go.opentelemetry.io/otel/metric
go.opentelemetry.io/otel/sdk
go.opentelemetry.io/otel/trace
go.opentelemetry.io/proto/otlp
gopkg.in/natefinch/lumberjack.v2
gopkg.in/yaml.v3
go.uber.org/multierr
go.uber.org/zap
sigs.k8s.io/json
sigs.k8s.io/yaml
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This observation led me down a rabbit hole. It took me a while to review why running go list—m all
differs from the output of doing go mod edit—json
.
It made me realize I had an issue with my jq
expression. So, I revised the expression. The amended version is: .Require[] | select(.Indirect | not) | .Path
. And I wrote the following to double-check that the output matches:
$ find . -name go.mod -exec /bin/bash -c 'echo evaluating $(dirname {}); cd $(dirname {}); diff <(go list -f "{{if not .Main}}{{if not .Indirect}}{{.Path}}{{end}}{{end}}" -m all | sort) <(go mod edit -json | jq -r ".Require[] | select(.Indirect | not) | .Path" | sort)' \;
evaluating ./client/pkg
evaluating ./client/v3
evaluating ./client/internal/v2
evaluating ./etcdctl
evaluating ./etcdutl
evaluating ./pkg
evaluating ./server
evaluating ./tests
evaluating ./tools/mod
evaluating ./tools/rw-heatmaps
evaluating ./tools/testgrid-analysis
evaluating ./api
evaluating .
So, there are no differences.
My rabbit hole, investigation, solution, and verification may have been an overkill 😅, as in the context of the original function, it filters the output only to keep go.etcd.io/etcd/*/v{2,3}
modules (to do the version bump), which are always in the go.mod
. But thanks for the good catch :) I'll update the code and open a pull request targeting this change, as it can coexist with the current code without the workspace.
I will review this PR after we officially release v3.6.0, thx |
This pull request is a follow-up on #19314.
It introduces minimal changes to enable a workspace in the project. We can iterate on it later to remove the script nuances of looping over the modules to execute commands, as they can now be executed from the top level of the repository.
Part of #18409.
Please read https://github.com/etcd-io/etcd/blob/main/CONTRIBUTING.md#contribution-flow.