Merge branch '696-replace-os-mkdirtemp-usages-by-t-tempdir-1' into 'main'

Resolve "Replace os.MkdirTemp() usages with t.TempDir()" (part 1)

See merge request https://gitlab.com/gitlab-org/gitlab-shell/-/merge_requests/909



Merged-by: Igor Drozdov <idrozdov@gitlab.com>
Reviewed-by: Ash McKenzie <amckenzie@gitlab.com>
Co-authored-by: Ash McKenzie <amckenzie@gitlab.com>

client/testserver/gitalyserver.go

@@ -84,11 +84,18 @@ func StartGitalyServer(t *testing.T, network string) (string, *TestGitalyServer)
 
 	switch network {
 	case "unix":
-		tempDir, _ := os.MkdirTemp("", "gitlab-shell-test-api")
-		gitalySocketPath := path.Join(tempDir, "gitaly.sock")
+		// We can't use t.TempDir() here because it will create a directory that
+		// far exceeds the 108 character limit which results in the socket failing
+		// to be created.
+		//
+		// See https://gitlab.com/gitlab-org/gitlab-shell/-/issues/696#note_1664726924
+		// for more detail.
+		tempDir, err := os.MkdirTemp("", "gitaly")
+		require.NoError(t, err)
 		t.Cleanup(func() { require.NoError(t, os.RemoveAll(tempDir)) })
 
-		err := os.MkdirAll(filepath.Dir(gitalySocketPath), 0700)
+		gitalySocketPath := path.Join(tempDir, "gitaly.sock")
+		err = os.MkdirAll(filepath.Dir(gitalySocketPath), 0700)
 		require.NoError(t, err)
 
 		addr, testServer := doStartTestServer(t, "unix", gitalySocketPath)

client/testserver/testserver.go

@@ -25,11 +25,18 @@ type TestRequestHandler struct {
 func StartSocketHttpServer(t *testing.T, handlers []TestRequestHandler) string {
 	t.Helper()
 
-	tempDir, _ := os.MkdirTemp("", "gitlab-shell-test-api")
-	t.Cleanup(func() { os.RemoveAll(tempDir) })
+	// We can't use t.TempDir() here because it will create a directory that
+	// far exceeds the 108 character limit which results in the socket failing
+	// to be created.
+	//
+	// See https://gitlab.com/gitlab-org/gitlab-shell/-/issues/696#note_1664726924
+	// for more detail.
+	tempDir, err := os.MkdirTemp("", "http")
+	require.NoError(t, err)
+	t.Cleanup(func() { require.NoError(t, os.RemoveAll(tempDir)) })
 
 	testSocket := path.Join(tempDir, "internal.sock")
-	err := os.MkdirAll(filepath.Dir(testSocket), 0700)
+	err = os.MkdirAll(filepath.Dir(testSocket), 0700)
 	require.NoError(t, err)
 
 	socketListener, err := net.Listen("unix", testSocket)

cmd/gitlab-sshd/acceptance_test.go

@@ -262,12 +262,10 @@ func buildClient(t *testing.T, addr string, hostKey ed25519.PublicKey) *ssh.Clie
 func configureSSHD(t *testing.T, apiServer string) (string, ed25519.PublicKey) {
 	t.Helper()
 
-	dir, err := os.MkdirTemp("", "gitlab-sshd-acceptance-test-")
-	require.NoError(t, err)
-	t.Cleanup(func() { os.RemoveAll(dir) })
+	tempDir := t.TempDir()
 
-	configFile := filepath.Join(dir, "config.yml")
-	hostKeyFile := filepath.Join(dir, "hostkey")
+	configFile := filepath.Join(tempDir, "config.yml")
+	hostKeyFile := filepath.Join(tempDir, "hostkey")
 
 	pub, priv, err := ed25519.GenerateKey(nil)
 	require.NoError(t, err)
@@ -279,7 +277,7 @@ func configureSSHD(t *testing.T, apiServer string) (string, ed25519.PublicKey) {
 	hostKeyData := pem.EncodeToMemory(block)
 	require.NoError(t, os.WriteFile(hostKeyFile, hostKeyData, 0400))
 
-	return dir, pub
+	return tempDir, pub
 }
 
 func startSSHD(t *testing.T, dir string) string {

internal/logger/logger_test.go

@@ -12,12 +12,10 @@ import (
 )
 
 func TestConfigure(t *testing.T) {
-	tmpFile, err := os.CreateTemp(os.TempDir(), "logtest-")
-	require.NoError(t, err)
-	defer tmpFile.Close()
+	tmpFile := createTempFile(t)
 
 	config := config.Config{
-		LogFile:   tmpFile.Name(),
+		LogFile:   tmpFile,
 		LogFormat: "json",
 	}
 
@@ -27,9 +25,7 @@ func TestConfigure(t *testing.T) {
 	log.Info("this is a test")
 	log.WithFields(log.Fields{}).Debug("debug log message")
 
-	tmpFile.Close()
-
-	data, err := os.ReadFile(tmpFile.Name())
+	data, err := os.ReadFile(tmpFile)
 	dataStr := string(data)
 	require.NoError(t, err)
 	require.Contains(t, dataStr, `"msg":"this is a test"`)
@@ -38,12 +34,10 @@ func TestConfigure(t *testing.T) {
 }
 
 func TestConfigureWithDebugLogLevel(t *testing.T) {
-	tmpFile, err := os.CreateTemp(os.TempDir(), "logtest-")
-	require.NoError(t, err)
-	defer tmpFile.Close()
+	tmpFile := createTempFile(t)
 
 	config := config.Config{
-		LogFile:   tmpFile.Name(),
+		LogFile:   tmpFile,
 		LogFormat: "json",
 		LogLevel:  "debug",
 	}
@@ -53,20 +47,16 @@ func TestConfigureWithDebugLogLevel(t *testing.T) {
 
 	log.WithFields(log.Fields{}).Debug("debug log message")
 
-	tmpFile.Close()
-
-	data, err := os.ReadFile(tmpFile.Name())
+	data, err := os.ReadFile(tmpFile)
 	require.NoError(t, err)
 	require.Contains(t, string(data), `msg":"debug log message"`)
 }
 
 func TestConfigureWithPermissionError(t *testing.T) {
-	tmpPath, err := os.MkdirTemp(os.TempDir(), "logtest-")
-	require.NoError(t, err)
-	defer os.RemoveAll(tmpPath)
+	tempDir := t.TempDir()
 
 	config := config.Config{
-		LogFile:   tmpPath,
+		LogFile:   tempDir,
 		LogFormat: "json",
 	}
 
@@ -77,13 +67,10 @@ func TestConfigureWithPermissionError(t *testing.T) {
 }
 
 func TestLogInUTC(t *testing.T) {
-	tmpFile, err := os.CreateTemp(os.TempDir(), "logtest-")
-	require.NoError(t, err)
-	defer tmpFile.Close()
-	defer os.Remove(tmpFile.Name())
+	tmpFile := createTempFile(t)
 
 	config := config.Config{
-		LogFile:   tmpFile.Name(),
+		LogFile:   tmpFile,
 		LogFormat: "json",
 	}
 
@@ -92,7 +79,7 @@ func TestLogInUTC(t *testing.T) {
 
 	log.Info("this is a test")
 
-	data, err := os.ReadFile(tmpFile.Name())
+	data, err := os.ReadFile(tmpFile)
 	require.NoError(t, err)
 
 	utc := `[0-9]{4}-[0-9]{2}-[0-9]{2}T[0-9]{2}:[0-9]{2}:[0-9]{2}Z`
@@ -101,3 +88,13 @@ func TestLogInUTC(t *testing.T) {
 	require.NoError(t, e)
 	require.True(t, r)
 }
+
+func createTempFile(t *testing.T) string {
+	t.Helper()
+
+	tmpFile, err := os.CreateTemp(t.TempDir(), "logtest-")
+	require.NoError(t, err)
+	tmpFile.Close()
+
+	return tmpFile.Name()
+}

internal/testhelper/testhelper.go

@@ -34,7 +34,7 @@ func PrepareTestRootDir(t *testing.T) {
 
 	require.NoError(t, os.MkdirAll(TestRoot, 0700))
 
-	t.Cleanup(func() { os.RemoveAll(TestRoot) })
+	t.Cleanup(func() { require.NoError(t, os.RemoveAll(TestRoot)) })
 
 	require.NoError(t, copyTestData())