Merge branch 'revert-28330762' into 'master'

Revert !1088 "Stop using gitlab-shell hooks -- but keep using gitlab-shell   config"

See merge request gitlab-org/gitaly!1117

13 files changed, 59 insertions, 39 deletions

diff --git a/changelogs/unreleased/revert-28330762.yml b/changelogs/unreleased/revert-28330762.yml
new file mode 100644
index 000000000..6876d5bab
--- /dev/null
+++ b/changelogs/unreleased/revert-28330762.yml
@@ -0,0 +1,6 @@
+---
+title: Revert !1088 "Stop using gitlab-shell hooks -- but keep using gitlab-shell
+  config"
+merge_request: 1117
+author:
+type: fixed
diff --git a/internal/config/config_test.go b/internal/config/config_test.go
index 6a8bcb5d0..f02221f4e 100644
--- a/internal/config/config_test.go
+++ b/internal/config/config_test.go
@@ -416,7 +416,6 @@ func TestConfigureRuby(t *testing.T) {
 		{dir: "", desc: "empty"},
 		{dir: "/does/not/exist", desc: "does not exist"},
 		{dir: tmpFile, desc: "exists but is not a directory"},
-		{dir: ".", ok: true, desc: "relative path"},
 		{dir: tmpDir, ok: true, desc: "ok"},
 	}
 
@@ -425,15 +424,11 @@ func TestConfigureRuby(t *testing.T) {
 			Config.Ruby = Ruby{Dir: tc.dir}
 
 			err := ConfigureRuby()
-			if !tc.ok {
+			if tc.ok {
+				require.NoError(t, err)
+			} else {
 				require.Error(t, err)
-				return
 			}
-
-			require.NoError(t, err)
-
-			dir := Config.Ruby.Dir
-			require.True(t, filepath.IsAbs(dir), "expected %q to be absolute path", dir)
 		})
 	}
 }
diff --git a/internal/config/ruby.go b/internal/config/ruby.go
index c10cb4e84..7e73ec0b2 100644
--- a/internal/config/ruby.go
+++ b/internal/config/ruby.go
@@ -2,7 +2,6 @@ package config
 
 import (
 	"fmt"
-	"path/filepath"
 	"time"
 )
 
@@ -54,11 +53,5 @@ func ConfigureRuby() error {
 		Config.Ruby.NumWorkers = minWorkers
 	}
 
-	var err error
-	Config.Ruby.Dir, err = filepath.Abs(Config.Ruby.Dir)
-	if err != nil {
-		return err
-	}
-
 	return validateIsDirectory(Config.Ruby.Dir, "gitaly-ruby.dir")
 }
diff --git a/internal/git/hooks/hooks.go b/internal/git/hooks/hooks.go
index d7df23a8a..9357f0fe5 100644
--- a/internal/git/hooks/hooks.go
+++ b/internal/git/hooks/hooks.go
@@ -1,6 +1,7 @@
 package hooks
 
 import (
+	"os"
 	"path"
 
 	"gitlab.com/gitlab-org/gitaly/internal/config"
@@ -9,11 +10,18 @@ import (
 // Override allows tests to control where the hooks directory is.
 var Override string
 
-// Path returns the path where the global git hooks are located.
+// Path returns the path where the global git hooks are located. As a
+// transitional mechanism, GITALY_USE_EMBEDDED_HOOKS=1 will cause
+// Gitaly's embedded hooks to be used instead of the legacy gitlab-shell
+// hooks.
 func Path() string {
 	if len(Override) > 0 {
 		return Override
 	}
 
-	return path.Join(config.Config.Ruby.Dir, "git-hooks")
+	if os.Getenv("GITALY_USE_EMBEDDED_HOOKS") == "1" {
+		return path.Join(config.Config.Ruby.Dir, "git-hooks")
+	}
+
+	return path.Join(config.Config.GitlabShell.Dir, "hooks")
 }
diff --git a/internal/git/hooks/hooks_test.go b/internal/git/hooks/hooks_test.go
index ce6989636..655366b7d 100644
--- a/internal/git/hooks/hooks_test.go
+++ b/internal/git/hooks/hooks_test.go
@@ -1,6 +1,8 @@
 package hooks
 
 import (
+	"fmt"
+	"os"
 	"testing"
 
 	"github.com/stretchr/testify/require"
@@ -8,15 +10,27 @@ import (
 )
 
 func TestPath(t *testing.T) {
-	defer func(rubyDir string) {
+	defer func(rubyDir, shellDir string) {
 		config.Config.Ruby.Dir = rubyDir
-	}(config.Config.Ruby.Dir)
+		config.Config.GitlabShell.Dir = shellDir
+	}(config.Config.Ruby.Dir, config.Config.GitlabShell.Dir)
 	config.Config.Ruby.Dir = "/bazqux/gitaly-ruby"
+	config.Config.GitlabShell.Dir = "/foobar/gitlab-shell"
+
+	hooksVar := "GITALY_USE_EMBEDDED_HOOKS"
+	t.Run(fmt.Sprintf("with %s=1", hooksVar), func(t *testing.T) {
+		os.Setenv(hooksVar, "1")
+		defer os.Unsetenv(hooksVar)
 
-	t.Run("default", func(t *testing.T) {
 		require.Equal(t, "/bazqux/gitaly-ruby/git-hooks", Path())
 	})
 
+	t.Run(fmt.Sprintf("without %s=1", hooksVar), func(t *testing.T) {
+		os.Unsetenv(hooksVar)
+
+		require.Equal(t, "/foobar/gitlab-shell/hooks", Path())
+	})
+
 	t.Run("with an override", func(t *testing.T) {
 		Override = "/override/hooks"
 		defer func() { Override = "" }()
diff --git a/internal/service/conflicts/testhelper_test.go b/internal/service/conflicts/testhelper_test.go
index 482fc0631..713a971b8 100644
--- a/internal/service/conflicts/testhelper_test.go
+++ b/internal/service/conflicts/testhelper_test.go
@@ -7,7 +7,6 @@ import (
 
 	log "github.com/sirupsen/logrus"
 	"gitlab.com/gitlab-org/gitaly-proto/go/gitalypb"
-	"gitlab.com/gitlab-org/gitaly/internal/git/hooks"
 	"gitlab.com/gitlab-org/gitaly/internal/rubyserver"
 	"gitlab.com/gitlab-org/gitaly/internal/testhelper"
 	"google.golang.org/grpc"
@@ -25,7 +24,6 @@ func testMain(m *testing.M) int {
 
 	var err error
 
-	hooks.Override = "/does/not/exist"
 	testhelper.ConfigureRuby()
 	RubyServer, err = rubyserver.Start()
 	if err != nil {
diff --git a/internal/service/operations/testhelper_test.go b/internal/service/operations/testhelper_test.go
index 5649ed514..3a5408f6b 100644
--- a/internal/service/operations/testhelper_test.go
+++ b/internal/service/operations/testhelper_test.go
@@ -11,6 +11,7 @@ import (
 	log "github.com/sirupsen/logrus"
 	"github.com/stretchr/testify/require"
 	"gitlab.com/gitlab-org/gitaly-proto/go/gitalypb"
+	"gitlab.com/gitlab-org/gitaly/internal/config"
 	"gitlab.com/gitlab-org/gitaly/internal/git/hooks"
 	"gitlab.com/gitlab-org/gitaly/internal/rubyserver"
 	"gitlab.com/gitlab-org/gitaly/internal/testhelper"
@@ -48,9 +49,16 @@ func testMain(m *testing.M) int {
 	if err != nil {
 		log.Fatal(err)
 	}
-	defer os.RemoveAll(hookDir)
 
-	hooks.Override = hookDir
+	defer func(oldDir string) {
+		config.Config.GitlabShell.Dir = oldDir
+	}(config.Config.GitlabShell.Dir)
+	config.Config.GitlabShell.Dir = hookDir
+
+	if err := os.MkdirAll(hooks.Path(), 0755); err != nil {
+		log.Fatal(err)
+	}
+	defer os.RemoveAll(hookDir)
 
 	testhelper.ConfigureGitalySSH()
 
diff --git a/internal/service/smarthttp/receive_pack_test.go b/internal/service/smarthttp/receive_pack_test.go
index 6525aa72e..ef083b67d 100644
--- a/internal/service/smarthttp/receive_pack_test.go
+++ b/internal/service/smarthttp/receive_pack_test.go
@@ -128,8 +128,10 @@ func TestFailedReceivePackRequestDueToHooksFailure(t *testing.T) {
 	require.NoError(t, err)
 	defer os.RemoveAll(hookDir)
 
-	defer func() { hooks.Override = "" }()
-	hooks.Override = hookDir
+	defer func(oldDir string) {
+		config.Config.GitlabShell.Dir = hookDir
+	}(config.Config.GitlabShell.Dir)
+	config.Config.GitlabShell.Dir = hookDir
 
 	require.NoError(t, os.MkdirAll(hooks.Path(), 0755))
 
diff --git a/internal/service/ssh/receive_pack_test.go b/internal/service/ssh/receive_pack_test.go
index c42c54c83..d69bdb002 100644
--- a/internal/service/ssh/receive_pack_test.go
+++ b/internal/service/ssh/receive_pack_test.go
@@ -148,8 +148,10 @@ func TestReceivePackPushHookFailure(t *testing.T) {
 	require.NoError(t, err)
 	defer os.RemoveAll(hookDir)
 
-	defer func() { hooks.Override = "" }()
-	hooks.Override = hookDir
+	defer func(oldDir string) {
+		config.Config.GitlabShell.Dir = oldDir
+	}(config.Config.GitlabShell.Dir)
+	config.Config.GitlabShell.Dir = hookDir
 
 	require.NoError(t, os.MkdirAll(hooks.Path(), 0755))
 
diff --git a/internal/service/wiki/testhelper_test.go b/internal/service/wiki/testhelper_test.go
index e909db224..1637d1ffc 100644
--- a/internal/service/wiki/testhelper_test.go
+++ b/internal/service/wiki/testhelper_test.go
@@ -11,7 +11,6 @@ import (
 	log "github.com/sirupsen/logrus"
 	"github.com/stretchr/testify/require"
 	"gitlab.com/gitlab-org/gitaly-proto/go/gitalypb"
-	"gitlab.com/gitlab-org/gitaly/internal/git/hooks"
 	gitlog "gitlab.com/gitlab-org/gitaly/internal/git/log"
 	"gitlab.com/gitlab-org/gitaly/internal/helper"
 	"gitlab.com/gitlab-org/gitaly/internal/rubyserver"
@@ -40,8 +39,6 @@ var rubyServer *rubyserver.Server
 func testMain(m *testing.M) int {
 	defer testhelper.MustHaveNoChildProcess()
 
-	hooks.Override = "/does/not/exist"
-
 	var err error
 	testhelper.ConfigureRuby()
 	rubyServer, err = rubyserver.Start()
diff --git a/ruby/lib/gitlab/git/hook.rb b/ruby/lib/gitlab/git/hook.rb
index 222c93d3b..1b2979cc2 100644
--- a/ruby/lib/gitlab/git/hook.rb
+++ b/ruby/lib/gitlab/git/hook.rb
@@ -7,10 +7,6 @@ module Gitlab
         Gitlab.config.git.hooks_directory
       end
 
-      def self.legacy_hooks_directory
-        File.join(Gitlab.config.gitlab_shell.path, 'hooks')
-      end
-
       GL_PROTOCOL = 'web'
       attr_reader :name, :path, :repository
 
diff --git a/ruby/lib/gitlab/git/repository.rb b/ruby/lib/gitlab/git/repository.rb
index 1a0b7d044..6f4bd9910 100644
--- a/ruby/lib/gitlab/git/repository.rb
+++ b/ruby/lib/gitlab/git/repository.rb
@@ -62,9 +62,10 @@ module Gitlab
           repo = Rugged::Repository.init_at(repo_path, true)
           repo.close
 
-          # TODO: stop symlinking to the old hooks location in or after GitLab 12.0.
-          # https://gitlab.com/gitlab-org/gitaly/issues/1392
-          create_hooks(repo_path, Gitlab::Git::Hook.legacy_hooks_directory)
+          # TODO: remove this when self healing hooks has been merged at least
+          # one release ago: https://gitlab.com/gitlab-org/gitaly/merge_requests/886
+          symlink_hooks_to = Gitlab::Git::Hook.directory
+          create_hooks(repo_path, symlink_hooks_to) if symlink_hooks_to.present?
         end
 
         def create_hooks(repo_path, global_hooks_path)
diff --git a/ruby/spec/lib/gitlab/git/repository_spec.rb b/ruby/spec/lib/gitlab/git/repository_spec.rb
index 4364b8818..677be7aa9 100644
--- a/ruby/spec/lib/gitlab/git/repository_spec.rb
+++ b/ruby/spec/lib/gitlab/git/repository_spec.rb
@@ -40,7 +40,7 @@ describe Gitlab::Git::Repository do # rubocop:disable Metrics/BlockLength
   describe '.create_hooks' do
     let(:repo_path) { File.join(storage_path, 'hook-test.git') }
     let(:hooks_dir) { File.join(repo_path, 'hooks') }
-    let(:target_hooks_dir) { Gitlab::Git::Hook.legacy_hooks_directory }
+    let(:target_hooks_dir) { Gitlab::Git::Hook.directory }
     let(:existing_target) { File.join(repo_path, 'foobar') }
 
     before do