Merge branch 'cleanup_module-updater' into 'master'

Easier major version upgrades

Closes #4894

See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/5505

Merged-by: James Fargher <proglottis@gmail.com>
Approved-by: Sami Hiltunen <shiltunen@gitlab.com>
Reviewed-by: Sami Hiltunen <shiltunen@gitlab.com>
Co-authored-by: James Fargher <jfargher@gitlab.com>

8 files changed, 50 insertions, 44 deletions

diff --git a/Makefile b/Makefile
index 8d1a0629a..3bc9d17ce 100644
--- a/Makefile
+++ b/Makefile
@@ -70,7 +70,7 @@ GOLANGCI_LINT_OPTIONS ?=
 GOLANGCI_LINT_CONFIG  ?= ${SOURCE_DIR}/.golangci.yml
 
 # Build information
-GITALY_PACKAGE    := gitlab.com/gitlab-org/gitaly/v15
+GITALY_PACKAGE    := $(shell go list -m)
 GITALY_VERSION    := $(shell ${GIT} describe --match v* 2>/dev/null | sed 's/^v//' || cat ${SOURCE_DIR}/VERSION 2>/dev/null || echo unknown)
 GO_LDFLAGS        := -X ${GITALY_PACKAGE}/internal/version.version=${GITALY_VERSION}
 SERVER_BUILD_TAGS := tracer_static,tracer_static_jaeger,tracer_static_stackdriver,continuous_profiler_stackdriver
diff --git a/internal/praefect/datastore/glsql/doc.go b/internal/praefect/datastore/glsql/doc.go
index 3e68b4bf2..655995fd6 100644
--- a/internal/praefect/datastore/glsql/doc.go
+++ b/internal/praefect/datastore/glsql/doc.go
@@ -23,7 +23,7 @@
 //   go test \
 //    -v \
 //    -count=1 \
-//    gitlab.com/gitlab-org/gitaly/v15/internal/praefect/datastore/glsql \
+//    ./internal/praefect/datastore/glsql \
 //    -run=^TestOpenDB$
 //
 // Once it is finished successfully you can be sure other tests would be able to
diff --git a/internal/praefect/datastore/queue_bm_test.go b/internal/praefect/datastore/queue_bm_test.go
index 7c8bee2fc..de7f60f29 100644
--- a/internal/praefect/datastore/queue_bm_test.go
+++ b/internal/praefect/datastore/queue_bm_test.go
@@ -11,22 +11,18 @@ import (
 )
 
 func BenchmarkPostgresReplicationEventQueue_Acknowledge(b *testing.B) {
-	// go test -tags=postgres -test.run=~ -test.bench=BenchmarkPostgresReplicationEventQueue_Acknowledge/small -benchtime=1000x gitlab.com/gitlab-org/gitaly/v15/internal/praefect/datastore
 	b.Run("small", func(b *testing.B) {
 		benchmarkPostgresReplicationEventQueueAcknowledge(b, map[JobState]int{JobStateReady: 10, JobStateInProgress: 10, JobStateFailed: 10})
 	})
 
-	// go test -tags=postgres -test.run=~ -test.bench=BenchmarkPostgresReplicationEventQueue_Acknowledge/medium -benchtime=100x gitlab.com/gitlab-org/gitaly/v15/internal/praefect/datastore
 	b.Run("medium", func(b *testing.B) {
 		benchmarkPostgresReplicationEventQueueAcknowledge(b, map[JobState]int{JobStateReady: 1_000, JobStateInProgress: 100, JobStateFailed: 100})
 	})
 
-	// go test -tags=postgres -test.run=~ -test.bench=BenchmarkPostgresReplicationEventQueue_Acknowledge/big -benchtime=10x gitlab.com/gitlab-org/gitaly/v15/internal/praefect/datastore
 	b.Run("big", func(b *testing.B) {
 		benchmarkPostgresReplicationEventQueueAcknowledge(b, map[JobState]int{JobStateReady: 100_000, JobStateInProgress: 100, JobStateFailed: 100})
 	})
 
-	// go test -tags=postgres -test.run=~ -test.bench=BenchmarkPostgresReplicationEventQueue_Acknowledge/huge -benchtime=1x gitlab.com/gitlab-org/gitaly/v15/internal/praefect/datastore
 	b.Run("huge", func(b *testing.B) {
 		benchmarkPostgresReplicationEventQueueAcknowledge(b, map[JobState]int{JobStateReady: 1_000_000, JobStateInProgress: 100, JobStateFailed: 100})
 	})
diff --git a/internal/praefect/datastore/repository_store_bm_test.go b/internal/praefect/datastore/repository_store_bm_test.go
index 3f982e417..cf595e06c 100644
--- a/internal/praefect/datastore/repository_store_bm_test.go
+++ b/internal/praefect/datastore/repository_store_bm_test.go
@@ -13,27 +13,22 @@ import (
 
 // The test setup takes a lot of time, so it is better to run each sub-benchmark separately with limit on number of repeats.
 func BenchmarkPostgresRepositoryStore_GetConsistentStorages(b *testing.B) {
-	// go test -tags=postgres -test.bench=BenchmarkPostgresRepositoryStore_GetConsistentStorages/extra-small -benchtime=5000x gitlab.com/gitlab-org/gitaly/v15/internal/praefect/datastore
 	b.Run("extra-small", func(b *testing.B) {
 		benchmarkGetConsistentStorages(b, 3, 1000)
 	})
 
-	// go test -tags=postgres -test.bench=BenchmarkPostgresRepositoryStore_GetConsistentStorages/small -benchtime=1000x gitlab.com/gitlab-org/gitaly/v15/internal/praefect/datastore
 	b.Run("small", func(b *testing.B) {
 		benchmarkGetConsistentStorages(b, 3, 10_000)
 	})
 
-	// go test -tags=postgres -test.bench=BenchmarkPostgresRepositoryStore_GetConsistentStorages/medium -benchtime=50x gitlab.com/gitlab-org/gitaly/v15/internal/praefect/datastore
 	b.Run("medium", func(b *testing.B) {
 		benchmarkGetConsistentStorages(b, 3, 100_000)
 	})
 
-	// go test -tags=postgres -test.bench=BenchmarkPostgresRepositoryStore_GetConsistentStorages/large -benchtime=10x gitlab.com/gitlab-org/gitaly/v15/internal/praefect/datastore
 	b.Run("large", func(b *testing.B) {
 		benchmarkGetConsistentStorages(b, 3, 1_000_000)
 	})
 
-	// go test -tags=postgres -test.bench=BenchmarkPostgresRepositoryStore_GetConsistentStorages/huge -benchtime=1x gitlab.com/gitlab-org/gitaly/v15/internal/praefect/datastore
 	b.Run("huge", func(b *testing.B) {
 		benchmarkGetConsistentStorages(b, 6, 1_000_000)
 	})
diff --git a/internal/testhelper/leakage.go b/internal/testhelper/leakage.go
index 0b78353f7..a944efb73 100644
--- a/internal/testhelper/leakage.go
+++ b/internal/testhelper/leakage.go
@@ -29,13 +29,13 @@ func mustHaveNoGoroutines() {
 		// eventually, but the pragmatic approach is to just wait until we remove
 		// the Ruby sidecar altogether.
 		goleak.IgnoreTopFunction("google.golang.org/grpc.(*ccBalancerWrapper).watcher"),
-		goleak.IgnoreTopFunction("gitlab.com/gitlab-org/gitaly/v15/internal/gitaly/rubyserver/balancer.(*builder).monitor"),
+		goleak.IgnoreTopFunction(PkgPath("internal/gitaly/rubyserver/balancer.(*builder).monitor")),
 		// labkit's logger spawns a Goroutine which cannot be closed when calling
 		// `Initialize()`.
 		goleak.IgnoreTopFunction("gitlab.com/gitlab-org/labkit/log.listenForSignalHangup"),
 		// The backchannel code is somehow stock on closing its connections. I have no clue
 		// why that is, but we should investigate.
-		goleak.IgnoreTopFunction("gitlab.com/gitlab-org/gitaly/v15/internal/backchannel.clientHandshake.serve.func4"),
+		goleak.IgnoreTopFunction(PkgPath("internal/backchannel.clientHandshake.serve.func4")),
 	); err != nil {
 		panic(fmt.Errorf("goroutines running: %w", err))
 	}
diff --git a/internal/testhelper/testcfg/binaries.go b/internal/testhelper/testcfg/binaries.go
index 001860586..c9f4a360b 100644
--- a/internal/testhelper/testcfg/binaries.go
+++ b/internal/testhelper/testcfg/binaries.go
@@ -113,7 +113,7 @@ func BuildBinary(tb testing.TB, targetDir, sourcePath string) string {
 			"build",
 			"-buildvcs=false",
 			"-tags", strings.Join(buildTags, ","),
-			"-ldflags", fmt.Sprintf("-X gitlab.com/gitlab-org/gitaly/v15/internal/version.version=%s", version.GetVersion()),
+			"-ldflags", fmt.Sprintf("-X %s/version.version=%s", testhelper.PkgPath("internal"), version.GetVersion()),
 			"-o", sharedBinaryPath,
 			sourcePath,
 		)
@@ -139,5 +139,5 @@ func BuildBinary(tb testing.TB, targetDir, sourcePath string) string {
 }
 
 func gitalyCommandPath(command string) string {
-	return fmt.Sprintf("gitlab.com/gitlab-org/gitaly/v15/cmd/%s", command)
+	return fmt.Sprintf("%s/cmd/%s", testhelper.PkgPath(), command)
 }
diff --git a/internal/testhelper/testhelper.go b/internal/testhelper/testhelper.go
index 8e4609dfb..bd0d279b0 100644
--- a/internal/testhelper/testhelper.go
+++ b/internal/testhelper/testhelper.go
@@ -17,7 +17,9 @@ import (
 	"net/http"
 	"os"
 	"os/exec"
+	"path"
 	"path/filepath"
+	"reflect"
 	"syscall"
 	"testing"
 	"time"
@@ -399,3 +401,14 @@ func SkipQuarantinedTest(t *testing.T, issue string, tests ...string) {
 		t.Skipf("This test has been quarantined. Please see %s for more information.", issue)
 	}
 }
+
+// pkgPath is used to determine the package path using reflection.
+type pkgPath struct{}
+
+// PkgPath returns the gitaly module package path, including major version
+// number. paths will be path joined to the returned package path.
+func PkgPath(paths ...string) string {
+	internalPkgPath := path.Dir(reflect.TypeOf(pkgPath{}).PkgPath())
+	rootPkgPath := path.Dir(internalPkgPath)
+	return path.Join(append([]string{rootPkgPath}, paths...)...)
+}
diff --git a/tools/module-updater/main.go b/tools/module-updater/main.go
index 608980d4c..f2a51afc4 100644
--- a/tools/module-updater/main.go
+++ b/tools/module-updater/main.go
@@ -161,48 +161,50 @@ func rewriteImports(moduleAbsRootPath, prev, next string) error {
 		}
 		seen[file.Name()] = struct{}{}
 
-		astFile, err := parser.ParseFile(fileSet, file.Name(), nil, parser.ParseComments)
-		if err != nil {
+		if err := rewriteFileImports(fileSet, file.Name(), prev, next); err != nil {
 			rerr = err
 			return false
 		}
 
-		for _, imprt := range astFile.Imports {
-			oldImport, err := strconv.Unquote(imprt.Path.Value)
-			if err != nil {
-				rerr = fmt.Errorf("unquote import: %s :%w", imprt.Path.Value, err)
-				return false
-			}
+		return true
+	})
 
-			if newImport := strings.Replace(oldImport, prev, next, 1); newImport != oldImport {
-				imprt.EndPos = imprt.End()
-				imprt.Path.Value = strconv.Quote(newImport)
-			}
-		}
+	return rerr
+}
+
+func rewriteFileImports(fileSet *token.FileSet, filename, prev, next string) (retErr error) {
+	astFile, err := parser.ParseFile(fileSet, filename, nil, parser.ParseComments)
+	if err != nil {
+		return err
+	}
 
-		f, err := os.Create(file.Name())
+	for _, imprt := range astFile.Imports {
+		oldImport, err := strconv.Unquote(imprt.Path.Value)
 		if err != nil {
-			rerr = fmt.Errorf("open file %q: %w", file.Name(), err)
-			return false
+			return fmt.Errorf("unquote import: %s :%w", imprt.Path.Value, err)
 		}
 
-		ferr := format.Node(f, fileSet, astFile)
-		cerr := f.Close()
-
-		if ferr != nil {
-			rerr = fmt.Errorf("rewrite file %q: %w", file.Name(), err)
-			return false
+		if newImport := strings.Replace(oldImport, prev, next, 1); newImport != oldImport {
+			imprt.EndPos = imprt.End()
+			imprt.Path.Value = strconv.Quote(newImport)
 		}
+	}
 
-		if cerr != nil {
-			rerr = fmt.Errorf("close file %q: %w", file.Name(), err)
-			return false
+	f, err := os.Create(filename)
+	if err != nil {
+		return fmt.Errorf("open file %q: %w", filename, err)
+	}
+	defer func() {
+		if err := f.Close(); retErr == nil && err != nil {
+			retErr = fmt.Errorf("close file %q: %w", filename, err)
 		}
+	}()
 
-		return true
-	})
+	if err := format.Node(f, fileSet, astFile); err != nil {
+		return fmt.Errorf("rewrite file %q: %w", filename, err)
+	}
 
-	return rerr
+	return nil
 }
 
 func normaliseDesiredImportReplacement(module, from, to string) (string, string, error) {
@@ -289,7 +291,7 @@ func rewriteProto(moduleAbsRootPath, prev, next string) error {
 		for _, line := range lines {
 			tokens := bytes.Fields(line)
 			pckg := bytes.Join(tokens, []byte{'~'})
-			if !bytes.HasPrefix(pckg, []byte(`option~go_package~=~"`+prev+`/proto/go/gitalypb";`)) {
+			if !bytes.HasPrefix(pckg, []byte(`option~go_package~=~"`+prev+`/proto/`)) {
 				modified = append(modified, line)
 				continue
 			}