Enable Go workspace #19314

ivanvc · 2025-02-01T06:39:32Z

I know that this may be like dropping a bomb, and I also know that the pull request has many commits and may not be easy to review.

However, after speaking with @thockin at KubeCon Salt Lake City and based on his experience doing this for the k/k repository, I don't think there's an easy way to break this pull request into smaller ones, as it would leave the repository in a broken state. Thanks for sending the first draft and simplifying how to use/update the forbidden dependencies.

However, I've been opening several pull requests targeting the fixes for findings I've found while editing the files, from removal of linters by duplications to addressing linting issues that were dodged in our previous approach.

I've been maintaining this parallel branch for at least a month now. It took me a while to write the commit messages correctly. So, it is easy to keep it up to the main branch; it just requires editing the first commit, where I introduce the go.work and go.sum.

This pull request introduces two new functionalities:

Auto-generating the go.work file based on the submodules in the repository. This may sound like overkill with the small number of submodules we have, but it future-proofs the project if we need to add or delete any of them.
Regenerate the go.work.sum based on the submodules go.mod/go.sum.

Both of these new additions are executed by make fix, so this will become a must when doing the dependency update. However, a new verify check will fail if the file go.work.sum is out of date.

Fixes #18409.

Please read https://github.com/etcd-io/etcd/blob/main/CONTRIBUTING.md#contribution-flow.

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. Co-authored-by: Tim Hockin <thockin@google.com> Signed-off-by: Ivan Valdes <ivan@vald.es>

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 <ivan@vald.es>

Update the `scripts/verify_go_versions.sh` script to include checks for the Go workspace toolchain version. It ensures that the `go.work` toolchain and Go directive versions are verified and in sync. Signed-off-by: Ivan Valdes <ivan@vald.es>

Update the `verify-lint` target to use Go workspaces, ensuring that linting is consistent across all workspace modules. Removing the need to change directories to execute Go commands in submodule directories. Update `scripts/test_lib.sh` to include functions for handling workspace modules. Signed-off-by: Ivan Valdes <ivan@vald.es>

Update the `verify-gofmt` make target to improve the efficiency of formatting checks. It removes the need to change directories to execute `gofmt` in submodule directories. The script `scripts/test.sh` can now use `gofmt` directly on all Go source files. Add a new function `go_source_files` in `scripts/test_lib.sh` to retrieve all Go source files from the repository. Signed-off-by: Ivan Valdes <ivan@vald.es>

k8s-ci-robot · 2025-02-01T06:39:37Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: ivanvc
Once this PR has been reviewed and has the lgtm label, please assign wenjiaswe for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

ivanvc · 2025-02-01T06:43:15Z

There's a failure in the BOM check. I'll draft the pull request after it finishes running all of the checks. I want to see if anything else fails.

codecov · 2025-02-01T06:58:00Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 68.88%. Comparing base (b197332) to head (9396f68).
Report is 14 commits behind head on main.

Additional details and impacted files

see 20 files with indirect coverage changes

@@            Coverage Diff             @@
##             main   #19314      +/-   ##
==========================================
+ Coverage   68.83%   68.88%   +0.05%     
==========================================
  Files         420      420              
  Lines       35701    35701              
==========================================
+ Hits        24574    24594      +20     
+ Misses       9701     9682      -19     
+ Partials     1426     1425       -1

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b197332...9396f68. Read the comment docs.

ivanvc · 2025-02-01T07:02:44Z

Noting that the coverage output is also wrong, making the CodeCov check fail.

Update the `verify-bom` make target to use workspace modules. It removes the need to change directories to execute Go commands in submodule directories. Add functions `workspace_modules` and `workspace_modules_without_tools` in `scripts/test_lib.sh` to handle workspace modules. Remove directory change requirements in the `tool_get_bin` function. Signed-off-by: Ivan Valdes <ivan@vald.es>

Remove the need to iterate over directories to execute `goword` by leveraging the Go workspace. Rename the function from `goword_for_package` to `goword_check` (to emphazise the removal of running per package [or submodule]) Standardize variable naming in bash (snake_case). Signed-off-by: Ivan Valdes <ivan@vald.es>

Update `license_header_check` to remove the iteration over directories by leveraging the Go workspace. Rename the function from `license_header_per_module` to `license_header_check` (to emphazise the removal of the need to run over each submodule). Signed-off-by: Ivan Valdes <ivan@vald.es>

Update the function to use `run_for_all_modules` instead of `run_for_modules` to ensure `go vet` is run for all modules. Signed-off-by: Ivan Valdes <ivan@vald.es>

Rename modifies the `mod_tidy_for_module` function to `mod_tidy_check` to emphasize the use of the Go workspace. However, this check still needs to run per each submodule, as `go mod tidy` doesn't accept an argument. Signed-off-by: Ivan Valdes <ivan@vald.es>

Update the function to use `go list $(workspace_relative_modules)` for getting the list of packages. Replace the array `skip_pkgs` with a Bash native associative array for better readability, and simplicity of the code. Remove the `govet_shadow_per_package` function, as it is not necessary to iterate over the submodules anymore. Signed-off-by: Ivan Valdes <ivan@vald.es>

Update the `update_module_version` function to use `go mod tidy` and `jq` to process module dependencies. Modify the `push_mod_tags_cmd` function to use `jq` for extracting module version and path information. Improve the logic to handle module updates and tagging more efficiently. Signed-off-by: Ivan Valdes <ivan@vald.es>

Modify the `bom_fix` function to call `bom_fixlet` directly instead of running it for a specific module. Update the `bom_fixlet` function to use a Bash compatible replacement for reading module lines. Simplify the logic for generating the bill of materials. Signed-off-by: Ivan Valdes <ivan@vald.es>

Update the `mod_tidy_fix` function to iterate over modules and run `go mod tidy` for each module directory. As it is not possible to execute `go mod tidy` at the top-level of the repository. Modify the `gofmt` call to leverage the Go workspace by removing the iteration over submodules. Signed-off-by: Ivan Valdes <ivan@vald.es>

Update the `govuln_pass` function in the `scripts/test.sh` file to use `run_for_all_modules` instead of `run_for_modules`. Signed-off-by: Ivan Valdes <ivan@vald.es>

Update the `etcd_build` function to remove unnecessary `cd` commands and directly specify the output paths for `go build` leveraging the Go workspace. Signed-off-by: Ivan Valdes <ivan@vald.es>

Update the `integration_extra`, `integration_pass`, `e2e_pass`, `robustness_pass`, `integration_e2e_pass`, `grpcproxy_pass`, and `grpcproxy_integration_pass` functions to remove `run_for_module` and directly call `go_test`. Simplify the test script by referencing the correct directories. Signed-off-by: Ivan Valdes <ivan@vald.es>

Update the `cov_pass` function in the `scripts/test.sh` file to remove `run_for_module` and directly call `go_test`. Remove the `modules` function from `scripts/test_lib.sh`, as it is not needed anymore as the rest of the functions are now using the Go workspace. Update the implemenation of "not_test_pacakges" to not include the tools submodules, as the original implementation doesn't include them. Signed-off-by: Ivan Valdes <ivan@vald.es>

Update the `scripts/sync_go_toolchain_directive.sh` file to include the `go work edit -toolchain=go` command. Signed-off-by: Ivan Valdes <ivan@vald.es>

Update the `YAMLFMT_VERSION` variable in the Makefile to remove the unnecessary `cd tools/mod` command, leveraging the Go workspace. Signed-off-by: Ivan Valdes <ivan@vald.es>

Update the `GOFAIL_VERSION` variable in the Makefile to remove the unnecessary `cd tools/mod` command, leveraging the Go workspace. And remove the change in directories when installing it. Signed-off-by: Ivan Valdes <ivan@vald.es>

ivanvc · 2025-02-02T06:21:39Z

/test pull-etcd-verify

k8s-ci-robot · 2025-02-02T07:04:08Z

@ivanvc: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-etcd-integration-1-cpu-amd64	`9396f68`	link	true	`/test pull-etcd-integration-1-cpu-amd64`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

serathius · 2025-02-03T08:33:29Z

scripts/fix.sh

-run_for_modules mod_tidy_fix || exit 2
-run_for_modules run ${GO_CMD} fmt || exit 2
+run_for_all_modules mod_tidy_fix || exit 2
+run run gofmt -w . || exit 2


serathius · 2025-02-03T08:36:04Z

tests/robustness/makefile.mk

@@ -111,7 +111,7 @@ $(GOPATH)/bin/gofail: tools/mod/go.mod tools/mod/go.sum
 	  (cd server; go get go.etcd.io/gofail@${GOFAIL_VERSION}); \
 	  (cd etcdctl; go get go.etcd.io/gofail@${GOFAIL_VERSION}); \
 	  (cd etcdutl; go get go.etcd.io/gofail@${GOFAIL_VERSION}); \
-	  (cd tools/mod; go get go.etcd.io/gofail@${GOFAIL_VERSION}); \
+	  (go get go.etcd.io/gofail@${GOFAIL_VERSION}); \


Please don't change how older etcd versions are built.

Good point. My bad.

serathius · 2025-02-03T08:38:07Z

scripts/build_lib.sh

@@ -26,36 +26,33 @@ etcd_build() {

  run rm -f "${out}/etcd"
  (
-    cd ./server


Is this change required for workspace to work or is it a cleanup thanks to usage of workspace? Thought that workspaces would still allow us execute from subdirectories using local path. I would recommend to defer changes that are not critical for workspace to work.

It's a clean up. I'll revert it.

ahrtr · 2025-02-03T15:29:29Z

api/go.mod

-	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.0.0 => ../FORBIDDEN_DEPENDENCY


why v3.0.0?

Also I don't see a need to change ./FORBIDDEN_DEPENDENCY to ../FORBIDDEN_DEPENDENCY, although not very important.

why v3.0.0?

For the workspace to work, dependencies need to be consistent. The problem is that we have the same dependency used and also in a replacement for the forbidden dependencies, then go complains with the following error:

conflicting replacements found for go.etcd.io/etcd/api/v3@v3.6.0-alpha.0 in workspace modules defined by [...]/etcd/etcd/go.mod and [...]/etcd/etcd/api/go.mod

It could technically be anything, but if we set it to a version like 3.6.0, we would need to change that when we release the 3.6.0 version.

Also I don't see a need to change ./FORBIDDEN_DEPENDENCY to ../FORBIDDEN_DEPENDENCY, although not very important.

In my first attempt to enable the Go workspace, I couldn't figure out why the forbidden dependency trick wasn't working. I removed it and implemented how k/k does the forbidden dependencies. However, Tim pointed out that it works if you point this virtual dependency to the exact location across the different submodules. So, the top-level points them to ./FORBIDDEN_DEPENDENCY, for the first-level (i.e., ./api), it should point to ../FORBIDDEN_DEPENDENCY and so on.

I removed it and implemented how k/k does the forbidden dependencies.

How does K8s implement the forbidden dependencies? I don't see any forbidden directive in K8s's go.mod files at all.

It could technically be anything, but if we set it to a version like 3.6.0, we would need to change that when we release the 3.6.0 version.

Even if we use "v3.0.0", we also need to manually update it when we release new major release, i.e. v4.0.0, but it's less frequent.

Can we raise a question in golang community to ask for best practice?

How does K8s implement the forbidden dependencies? I don't see any forbidden directive in K8s's go.mod files at all.

They use an internal tool import-boss, which works with .import-restrictions files.

ahrtr · 2025-02-03T15:30:32Z

go.work.sum

@@ -0,0 +1,986 @@
+bitbucket.org/creachadair/shell v0.0.7 h1:Z96pB6DkSb7F3Y3BBnJeOZH2gazyMTWlvecSD4vDqfk=


This file isn't up to date. If you remove this file and re-generate it (run make build), then you will see that lots of entries will be cleaned up.

This is because while running make build, the tools/ submodules are not touched. Then, it deletes those dependencies. Running make update-go-workspace reconciles the changes.

ahrtr · 2025-02-03T15:38:47Z

scripts/fix.sh

+  local modules=("$@")
+  for module in "${modules[@]}"; do
+    local module_dir
+    module_dir="${module%/...}"
+    run_for_module "${module_dir}" run "${GO_CMD}" mod tidy
+  done


mod_tidy_fix is supposed to run on a single module, and the loop is the responsibility of the caller.

Do not get time to read through the whole PR yet, is it possible not to change such logic to make the review easier? If this is a change unrelated to go workspace, then we should do it separately.

ahrtr · 2025-02-03T15:43:04Z

It might be a little risky to merge this into main branch right before we release 3.6. We may want to only merge it into 3.7.

Also please only focus this PR only the necessary change related to go workspace. If it isn't necessary or any improvement, let's do it separately in follow PR.

ivanvc · 2025-02-03T18:25:50Z

It might be a little risky to merge this into main branch right before we release 3.6. We may want to only merge it into 3.7.

I agree. I was trying to rush it to get it for 3.6.0.

Also please only focus this PR only the necessary change related to go workspace. If it isn't necessary or any improvement, let's do it separately in follow PR.

I have an idea of what I could do. This pull request was:

Enabling the Go workspace.
Untangling the build and test scripts to use the workspace without changing directories (as now commands work from the top level).

If we go only with the first option, would we consider enabling the workspace for 3.6?

ahrtr · 2025-02-03T19:49:42Z

If we go only with the first option, would we consider enabling the workspace for 3.6?

Everything is working well without go workspace, so I suggest not to rush to integration workspace in such a short period before releasing v3.6.0.

ivanvc · 2025-02-04T00:16:43Z

I'll close this pull request and re-work this feature by targeting the minimal code needed to enable the workspace without adding improvements.

ivanvc and others added 5 commits January 31, 2025 22:25

k8s-ci-robot added area/clientv3 area/etcdctl area/etcdutl area/robustness-testing area/testing size/XXL labels Feb 1, 2025

ivanvc changed the title ~~Enable Go workspace~~ [do not review] Enable Go workspace Feb 1, 2025

ivanvc marked this pull request as draft February 1, 2025 07:30

k8s-ci-robot added the do-not-merge/work-in-progress label Feb 1, 2025

ivanvc force-pushed the enable-go-workspace branch 2 times, most recently from 2ced4c4 to efdb0d3 Compare February 1, 2025 07:38

ivanvc added 7 commits February 1, 2025 00:30

Migrate verify-govet make target to use the Go workspace

dad115f

Update the function to use `run_for_all_modules` instead of `run_for_modules` to ensure `go vet` is run for all modules. Signed-off-by: Ivan Valdes <ivan@vald.es>

ivanvc force-pushed the enable-go-workspace branch 3 times, most recently from 80bee1c to 551b795 Compare February 2, 2025 05:25

ivanvc force-pushed the enable-go-workspace branch from 551b795 to bd95d0a Compare February 2, 2025 05:50

ivanvc added 9 commits February 1, 2025 22:13

Migrate run-govulncheck to use the Go workspace

73bb43f

Update the `govuln_pass` function in the `scripts/test.sh` file to use `run_for_all_modules` instead of `run_for_modules`. Signed-off-by: Ivan Valdes <ivan@vald.es>

Migrate build script to use the Go workspace

23ae621

Update the `etcd_build` function to remove unnecessary `cd` commands and directly specify the output paths for `go build` leveraging the Go workspace. Signed-off-by: Ivan Valdes <ivan@vald.es>

Migrate sync-toolchain-directive make target to use the Go workspace

277ffd7

Update the `scripts/sync_go_toolchain_directive.sh` file to include the `go work edit -toolchain=go` command. Signed-off-by: Ivan Valdes <ivan@vald.es>

Migrate fix-yamllint to use the Go workspace

a639591

Update the `YAMLFMT_VERSION` variable in the Makefile to remove the unnecessary `cd tools/mod` command, leveraging the Go workspace. Signed-off-by: Ivan Valdes <ivan@vald.es>

Migrate robustness makefile to use the Go workspace

9396f68

Update the `GOFAIL_VERSION` variable in the Makefile to remove the unnecessary `cd tools/mod` command, leveraging the Go workspace. And remove the change in directories when installing it. Signed-off-by: Ivan Valdes <ivan@vald.es>

ivanvc force-pushed the enable-go-workspace branch from bd95d0a to 9396f68 Compare February 2, 2025 06:21

ivanvc changed the title ~~[do not review] Enable Go workspace~~ Enable Go workspace Feb 2, 2025

ivanvc marked this pull request as ready for review February 2, 2025 06:42

k8s-ci-robot removed the do-not-merge/work-in-progress label Feb 2, 2025

serathius reviewed Feb 3, 2025

View reviewed changes

ahrtr reviewed Feb 3, 2025

View reviewed changes

ivanvc closed this Feb 4, 2025

ivanvc mentioned this pull request Feb 18, 2025

Minimal changes to enable a Go workspace #19423

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable Go workspace #19314

Enable Go workspace #19314

ivanvc commented Feb 1, 2025

k8s-ci-robot commented Feb 1, 2025

ivanvc commented Feb 1, 2025

codecov bot commented Feb 1, 2025 •

edited

Loading

ivanvc commented Feb 1, 2025

ivanvc commented Feb 2, 2025

k8s-ci-robot commented Feb 2, 2025

serathius Feb 3, 2025

ivanvc Feb 3, 2025

serathius Feb 3, 2025

ivanvc Feb 3, 2025

serathius Feb 3, 2025 •

edited

Loading

ivanvc Feb 3, 2025

ahrtr Feb 3, 2025

ahrtr Feb 3, 2025

ivanvc Feb 3, 2025

ahrtr Feb 3, 2025

ivanvc Feb 3, 2025

ahrtr Feb 3, 2025

ivanvc Feb 3, 2025

ahrtr Feb 3, 2025

ahrtr commented Feb 3, 2025

ivanvc commented Feb 3, 2025

ahrtr commented Feb 3, 2025

ivanvc commented Feb 4, 2025

		@@ -0,0 +1,986 @@
		bitbucket.org/creachadair/shell v0.0.7 h1:Z96pB6DkSb7F3Y3BBnJeOZH2gazyMTWlvecSD4vDqfk=

Enable Go workspace #19314

Enable Go workspace #19314

Conversation

ivanvc commented Feb 1, 2025

k8s-ci-robot commented Feb 1, 2025

ivanvc commented Feb 1, 2025

codecov bot commented Feb 1, 2025 • edited Loading

Codecov Report

ivanvc commented Feb 1, 2025

ivanvc commented Feb 2, 2025

k8s-ci-robot commented Feb 2, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

serathius Feb 3, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahrtr commented Feb 3, 2025

ivanvc commented Feb 3, 2025

ahrtr commented Feb 3, 2025

ivanvc commented Feb 4, 2025

codecov bot commented Feb 1, 2025 •

edited

Loading

serathius Feb 3, 2025 •

edited

Loading