Skip to content
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

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 10 additions & 2 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -75,10 +75,10 @@ fuzz:
verify: verify-gofmt verify-bom verify-lint verify-dep verify-shellcheck verify-goword \
verify-govet verify-license-header verify-mod-tidy \
verify-shellws verify-proto-annotations verify-genproto verify-yamllint \
verify-govet-shadow verify-markdown-marker verify-go-versions
verify-govet-shadow verify-markdown-marker verify-go-versions verify-go-workspace

.PHONY: fix
fix: fix-bom fix-lint fix-yamllint sync-toolchain-directive
fix: fix-bom fix-lint fix-yamllint sync-toolchain-directive update-go-workspace
./scripts/fix.sh

.PHONY: verify-gofmt
Expand Down Expand Up @@ -215,6 +215,14 @@ clean:
verify-go-versions:
./scripts/verify_go_versions.sh

.PHONY: verify-go-workspace
verify-go-workspace:
PASSES="go_workspace" ./scripts/test.sh

.PHONY: sync-toolchain-directive
sync-toolchain-directive:
./scripts/sync_go_toolchain_directive.sh

.PHONY: update-go-workspace
update-go-workspace:
./scripts/update_go_workspace.sh
10 changes: 5 additions & 5 deletions api/go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -31,9 +31,9 @@ require (
// Bad imports are sometimes causing attempts to pull that code.
// This makes the error more explicit.
replace (
go.etcd.io/etcd => ./FORBIDDEN_DEPENDENCY
go.etcd.io/etcd/api/v3 => ./FORBIDDEN_DEPENDENCY
go.etcd.io/etcd/pkg/v3 => ./FORBIDDEN_DEPENDENCY
go.etcd.io/etcd/tests/v3 => ./FORBIDDEN_DEPENDENCY
go.etcd.io/etcd/v3 => ./FORBIDDEN_DEPENDENCY
go.etcd.io/etcd => ../FORBIDDEN_DEPENDENCY
go.etcd.io/etcd/api/v3 v3.999.999 => ../FORBIDDEN_DEPENDENCY
go.etcd.io/etcd/pkg/v3 v3.999.999 => ../FORBIDDEN_DEPENDENCY
go.etcd.io/etcd/tests/v3 v3.999.999 => ../FORBIDDEN_DEPENDENCY
go.etcd.io/etcd/v3 v3.999.999 => ../FORBIDDEN_DEPENDENCY
)
8 changes: 4 additions & 4 deletions client/internal/v2/go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,8 @@ replace (
// Bad imports are sometimes causing attempts to pull that code.
// This makes the error more explicit.
replace (
go.etcd.io/etcd => ./FORBIDDEN_DEPENDENCY
go.etcd.io/etcd/pkg/v3 => ./FORBIDDED_DEPENDENCY
go.etcd.io/etcd/tests/v3 => ./FORBIDDEN_DEPENDENCY
go.etcd.io/etcd/v3 => ./FORBIDDEN_DEPENDENCY
go.etcd.io/etcd => ../../../FORBIDDEN_DEPENDENCY
go.etcd.io/etcd/pkg/v3 v3.999.999 => ../../../FORBIDDEN_DEPENDENCY
go.etcd.io/etcd/tests/v3 v3.999.999 => ../../../FORBIDDEN_DEPENDENCY
go.etcd.io/etcd/v3 v3.999.999 => ../../../FORBIDDEN_DEPENDENCY
)
8 changes: 4 additions & 4 deletions client/v3/go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -52,8 +52,8 @@ replace (
// Bad imports are sometimes causing attempts to pull that code.
// This makes the error more explicit.
replace (
go.etcd.io/etcd => ./FORBIDDEN_DEPENDENCY
go.etcd.io/etcd/pkg/v3 => ./FORBIDDEN_DEPENDENCY
go.etcd.io/etcd/v3 => ./FORBIDDEN_DEPENDENCY
go.etcd.io/tests/v3 => ./FORBIDDEN_DEPENDENCY
go.etcd.io/etcd => ../../FORBIDDEN_DEPENDENCY
go.etcd.io/etcd/pkg/v3 v3.999.999 => ../../FORBIDDEN_DEPENDENCY
go.etcd.io/etcd/v3 v3.999.999 => ../../FORBIDDEN_DEPENDENCY
go.etcd.io/tests/v3 v3.999.999 => ../../FORBIDDEN_DEPENDENCY
)
6 changes: 3 additions & 3 deletions etcdctl/go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ replace (
// Bad imports are sometimes causing attempts to pull that code.
// This makes the error more explicit.
replace (
go.etcd.io/etcd => ./FORBIDDEN_DEPENDENCY
go.etcd.io/etcd/v3 => ./FORBIDDEN_DEPENDENCY
go.etcd.io/tests/v3 => ./FORBIDDEN_DEPENDENCY
go.etcd.io/etcd => ../FORBIDDEN_DEPENDENCY
go.etcd.io/etcd/v3 v3.999.999 => ../FORBIDDEN_DEPENDENCY
go.etcd.io/tests/v3 v3.999.999 => ../FORBIDDEN_DEPENDENCY
)
6 changes: 3 additions & 3 deletions etcdutl/go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,9 @@ replace (
// Bad imports are sometimes causing attempts to pull that code.
// This makes the error more explicit.
replace (
go.etcd.io/etcd => ./FORBIDDEN_DEPENDENCY
go.etcd.io/etcd/v3 => ./FORBIDDEN_DEPENDENCY
go.etcd.io/tests/v3 => ./FORBIDDEN_DEPENDENCY
go.etcd.io/etcd => ../FORBIDDEN_DEPENDENCY
go.etcd.io/etcd/v3 v3.999.999 => ../FORBIDDEN_DEPENDENCY
go.etcd.io/tests/v3 v3.999.999 => ../FORBIDDEN_DEPENDENCY
)

require (
Expand Down
21 changes: 21 additions & 0 deletions go.work
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
// This is a generated file. Do not edit directly.

go 1.23

toolchain go1.23.6

use (
.
./api
./client/internal/v2
./client/pkg
./client/v3
./etcdctl
./etcdutl
./pkg
./server
./tests
./tools/mod
./tools/rw-heatmaps
./tools/testgrid-analysis
)
993 changes: 993 additions & 0 deletions go.work.sum

Large diffs are not rendered by default.

8 changes: 4 additions & 4 deletions pkg/go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -38,8 +38,8 @@ replace go.etcd.io/etcd/client/pkg/v3 => ../client/pkg
// Etcd contains lots of packages and dependency relationship.
// Shouldn't import unnecessary dependencies
replace (
go.etcd.io/etcd => ./FORBIDDEN_DEPENDENCY
go.etcd.io/etcd/api/v3 => ./FORBIDDEN_DEPENDENCY
go.etcd.io/etcd/tests/v3 => ./FORBIDDEN_DEPENDENCY
go.etcd.io/etcd/v3 => ./FORBIDDEN_DEPENDENCY
go.etcd.io/etcd => ../FORBIDDEN_DEPENDENCY
go.etcd.io/etcd/api/v3 v3.999.999 => ../FORBIDDEN_DEPENDENCY
go.etcd.io/etcd/tests/v3 v3.999.999 => ../FORBIDDEN_DEPENDENCY
go.etcd.io/etcd/v3 v3.999.999 => ../FORBIDDEN_DEPENDENCY
)
10 changes: 5 additions & 5 deletions scripts/release_mod.sh
Original file line number Diff line number Diff line change
Expand Up @@ -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')
Copy link
Member

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

Copy link
Member Author

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.


v3deps=$(echo "${modules}" | grep -E "${ROOT_MODULE}/.*/v3")
for dep in ${v3deps}; do
Expand Down Expand Up @@ -97,16 +97,16 @@ function push_mod_tags_cmd {

# Any module ccan be used for this
local main_version
main_version=$(go list -f '{{.Version}}' -m "${ROOT_MODULE}/api/v3")
main_version=$(go mod edit -json | jq -r '.Require[] | select(.Path == "'"${ROOT_MODULE}"'/api/v3") | .Version')
local tags=()

keyid=$(get_gpg_key) || return 2

for module in $(modules); do
local version
version=$(go list -f '{{.Version}}' -m "${module}")
version=$(go mod edit -json | jq -r '.Require[] | select(.Path == "'"${module}"'") | .Version')
local path
path=$(go list -f '{{.Path}}' -m "${module}")
path=$(go mod edit -json | jq -r '.Require[] | select(.Path == "'"${module}"'") | .Path')
local subdir="${path//${ROOT_MODULE}\//}"
local tag
if [ -z "${version}" ]; then
Expand All @@ -121,7 +121,7 @@ function push_mod_tags_cmd {
# consider main-module's tag as the latest.
run sleep 2
run git tag --local-user "${keyid}" --sign "${tag}" --message "${version}"
tags=("${tags[@]}" "${tag}")
tags+=("${tag}")
done
maybe_run git push -f "${REMOTE_REPO}" "${tags[@]}"
}
Expand Down
1 change: 1 addition & 0 deletions scripts/sync_go_toolchain_directive.sh
Original file line number Diff line number Diff line change
Expand Up @@ -12,3 +12,4 @@ source ./scripts/test_lib.sh

TARGET_GO_VERSION="${TARGET_GO_VERSION:-"$(cat "${ETCD_ROOT_DIR}/.go-version")"}"
find . -name 'go.mod' -exec go mod edit -toolchain=go"${TARGET_GO_VERSION}" {} \;
go work edit -toolchain=go"${TARGET_GO_VERSION}"
15 changes: 13 additions & 2 deletions scripts/test.sh
Original file line number Diff line number Diff line change
Expand Up @@ -483,7 +483,7 @@ function bom_pass {
run cp go.sum go.sum.tmp || return 2
run cp go.mod go.mod.tmp || return 2

output=$(GOFLAGS=-mod=mod run_go_tool github.com/appscodelabs/license-bill-of-materials \
output=$(run_go_tool github.com/appscodelabs/license-bill-of-materials \
--override-file ./bill-of-materials.override.json \
"${modules[@]}")
code="$?"
Expand All @@ -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
Copy link
Member

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.

Copy link
Member Author

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I opened #19463.

return 255
fi
run go mod edit -json | jq -r '.Require[] | .Path+","+.Version+","+if .Indirect then " (indirect)" else "" end+",'"${module}"'"'
Expand Down Expand Up @@ -617,6 +617,17 @@ function genproto_pass {
"${ETCD_ROOT_DIR}/scripts/verify_genproto.sh"
}

function go_workspace_pass {
log_callout "Ensuring go workspace is in sync."

run go mod download
if [ -n "$(git status --porcelain go.work.sum)" ]; then
log_error "Go workspace not in sync."
log_warning "Suggestion: run \"make fix\" to address the issue."
return 255
fi
}

########### MAIN ###############################################################

function run_pass {
Expand Down
42 changes: 42 additions & 0 deletions scripts/update_go_workspace.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
#!/usr/bin/env bash
#
# Copyright 2024 The etcd Authors
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.
#
# Based on k/k scripts/update-go-workspace.sh:
# https://github.com/kubernetes/kubernetes/blob/e2b96b25661849775dedf441b2f5c555392caa84/hack/update-go-workspace.sh

# This script generates go.work so that it includes all Go packages
# in this repo, with a few exceptions.

set -euo pipefail

source ./scripts/test_lib.sh

# Avoid issues and remove the workspace files.
rm -f go.work go.work.sum

# Generate the workspace.
go work init
sed -i -e '1i\// This is a generated file. Do not edit directly.\n' go.work

git ls-files -z ':(glob)**/go.mod' \
| xargs -0 -n1 dirname -z \
| xargs -0 -n1 go work edit -use

go work edit -toolchain "go$(cat .go-version)"
go work edit -go "$(go mod edit -json | jq -r .Go)"

# generate go.work.sum
go mod download
2 changes: 1 addition & 1 deletion scripts/updatebom.sh
Original file line number Diff line number Diff line change
Expand Up @@ -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" \
Copy link
Member

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?

Copy link
Member Author

@ivanvc ivanvc Feb 22, 2025

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.

Copy link
Member Author

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.

if run_go_tool "github.com/appscodelabs/license-bill-of-materials" \
--override-file ./bill-of-materials.override.json \
"${modules[@]}" > ./bill-of-materials.json.tmp; then
cp ./bill-of-materials.json.tmp ./bill-of-materials.json
Expand Down
16 changes: 16 additions & 0 deletions scripts/verify_go_versions.sh
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,20 @@ function verify_go_versions() {
fi
}

# Workaround to get go.work's toolchain, as go work edit -json doesn't return
# the toolchain as of Go 1.24. When this is fixed, we can replace these two
# checks with verify_go_versions go.work
toolchain_version="$(grep toolchain go.work | cut -d' ' -f2)"
if [[ "go${target_go_version}" != "${toolchain_version}" ]]; then
log_error "go toolchain directive out of sync for go.work, got: ${toolchain_version}"
toolchain_out_of_sync="true"
fi
go_line_version="$(go work edit -json | jq -r .Go)"
if ! printf '%s\n' "${go_line_version}" "${target_go_version}" | sort --check=silent --version-sort; then
log_error "go directive in go.work is greater than maximum allowed: go${target_go_version}"
go_line_violation="true"
fi

while read -r mod; do
verify_go_versions "${mod}";
done < <(find . -name 'go.mod')
Expand All @@ -51,3 +65,5 @@ fi
if [[ "${go_line_violation}" == "true" ]] || [[ "${toolchain_out_of_sync}" == "true" ]]; then
exit 1
fi

log_success "SUCCESS: Go toolchain directive in sync"
7 changes: 4 additions & 3 deletions server/go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,7 @@ replace (

// Bad imports are sometimes causing attempts to pull that code.
// This makes the error more explicit.
replace go.etcd.io/etcd => ./FORBIDDEN_DEPENDENCY

replace go.etcd.io/tests/v3 => ./FORBIDDEN_DEPENDENCY
replace (
go.etcd.io/etcd => ../FORBIDDEN_DEPENDENCY
go.etcd.io/tests/v3 v3.999.999 => ../FORBIDDEN_DEPENDENCY
)