From 49c66f0eaa1410dc0ae6d6687d1ea1baecfd6aa4 Mon Sep 17 00:00:00 2001 From: Jacob Vosmaer Date: Thu, 14 Mar 2019 17:03:02 +0000 Subject: Revert "Revert "Merge branch 'switch-to-embedded-hooks' into 'master'"" This reverts commit 4f376cf16e64a0fa673821c0eb4a56eca9ca9419. --- changelogs/unreleased/re-apply-hooks-change.yml | 5 +++++ internal/config/config_test.go | 11 +++++++--- internal/config/ruby.go | 7 ++++++ internal/git/hooks/hooks.go | 14 ++++++------ internal/git/hooks/hooks_test.go | 29 +++++++++++-------------- internal/service/conflicts/testhelper_test.go | 2 ++ internal/service/operations/testhelper_test.go | 12 ++-------- internal/service/smarthttp/receive_pack_test.go | 6 ++--- internal/service/ssh/receive_pack_test.go | 6 ++--- internal/service/wiki/testhelper_test.go | 3 +++ ruby/lib/gitlab/git/hook.rb | 4 ++++ ruby/lib/gitlab/git/repository.rb | 7 +++--- ruby/spec/lib/gitlab/git/repository_spec.rb | 2 +- 13 files changed, 59 insertions(+), 49 deletions(-) create mode 100644 changelogs/unreleased/re-apply-hooks-change.yml diff --git a/changelogs/unreleased/re-apply-hooks-change.yml b/changelogs/unreleased/re-apply-hooks-change.yml new file mode 100644 index 000000000..99ce156e7 --- /dev/null +++ b/changelogs/unreleased/re-apply-hooks-change.yml @@ -0,0 +1,5 @@ +--- +title: Re-apply MR 1088 (Git hooks change) +merge_request: 1130 +author: +type: other diff --git a/internal/config/config_test.go b/internal/config/config_test.go index f02221f4e..6a8bcb5d0 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -416,6 +416,7 @@ 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"}, } @@ -424,11 +425,15 @@ func TestConfigureRuby(t *testing.T) { Config.Ruby = Ruby{Dir: tc.dir} err := ConfigureRuby() - if tc.ok { - require.NoError(t, err) - } else { + if !tc.ok { 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 7e73ec0b2..c10cb4e84 100644 --- a/internal/config/ruby.go +++ b/internal/config/ruby.go @@ -2,6 +2,7 @@ package config import ( "fmt" + "path/filepath" "time" ) @@ -53,5 +54,11 @@ 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 9357f0fe5..c77c3ed1e 100644 --- a/internal/git/hooks/hooks.go +++ b/internal/git/hooks/hooks.go @@ -10,18 +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. 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. +// Path returns the path where the global git hooks are located. If the +// environment variable GITALY_TESTING_NO_GIT_HOOKS is set to "1", Path +// will return an empty directory, which has the effect that no Git hooks +// will run at all. func Path() string { if len(Override) > 0 { return Override } - if os.Getenv("GITALY_USE_EMBEDDED_HOOKS") == "1" { - return path.Join(config.Config.Ruby.Dir, "git-hooks") + if os.Getenv("GITALY_TESTING_NO_GIT_HOOKS") == "1" { + return "/var/empty" } - return path.Join(config.Config.GitlabShell.Dir, "hooks") + return path.Join(config.Config.Ruby.Dir, "git-hooks") } diff --git a/internal/git/hooks/hooks_test.go b/internal/git/hooks/hooks_test.go index 655366b7d..6c2657461 100644 --- a/internal/git/hooks/hooks_test.go +++ b/internal/git/hooks/hooks_test.go @@ -1,7 +1,6 @@ package hooks import ( - "fmt" "os" "testing" @@ -10,31 +9,29 @@ import ( ) func TestPath(t *testing.T) { - defer func(rubyDir, shellDir string) { + defer func(rubyDir string) { config.Config.Ruby.Dir = rubyDir - config.Config.GitlabShell.Dir = shellDir - }(config.Config.Ruby.Dir, config.Config.GitlabShell.Dir) + }(config.Config.Ruby.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 = "" }() require.Equal(t, "/override/hooks", Path()) }) + + t.Run("when an env override", func(t *testing.T) { + key := "GITALY_TESTING_NO_GIT_HOOKS" + + os.Setenv(key, "1") + defer os.Unsetenv(key) + + require.Equal(t, "/var/empty", Path()) + }) + } diff --git a/internal/service/conflicts/testhelper_test.go b/internal/service/conflicts/testhelper_test.go index 713a971b8..482fc0631 100644 --- a/internal/service/conflicts/testhelper_test.go +++ b/internal/service/conflicts/testhelper_test.go @@ -7,6 +7,7 @@ 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" @@ -24,6 +25,7 @@ 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 3a5408f6b..5649ed514 100644 --- a/internal/service/operations/testhelper_test.go +++ b/internal/service/operations/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/config" "gitlab.com/gitlab-org/gitaly/internal/git/hooks" "gitlab.com/gitlab-org/gitaly/internal/rubyserver" "gitlab.com/gitlab-org/gitaly/internal/testhelper" @@ -49,17 +48,10 @@ func testMain(m *testing.M) int { if err != nil { log.Fatal(err) } - - 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) + hooks.Override = hookDir + testhelper.ConfigureGitalySSH() testhelper.ConfigureRuby() diff --git a/internal/service/smarthttp/receive_pack_test.go b/internal/service/smarthttp/receive_pack_test.go index ef083b67d..6525aa72e 100644 --- a/internal/service/smarthttp/receive_pack_test.go +++ b/internal/service/smarthttp/receive_pack_test.go @@ -128,10 +128,8 @@ func TestFailedReceivePackRequestDueToHooksFailure(t *testing.T) { require.NoError(t, err) defer os.RemoveAll(hookDir) - defer func(oldDir string) { - config.Config.GitlabShell.Dir = hookDir - }(config.Config.GitlabShell.Dir) - config.Config.GitlabShell.Dir = hookDir + defer func() { hooks.Override = "" }() + hooks.Override = 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 d69bdb002..c42c54c83 100644 --- a/internal/service/ssh/receive_pack_test.go +++ b/internal/service/ssh/receive_pack_test.go @@ -148,10 +148,8 @@ func TestReceivePackPushHookFailure(t *testing.T) { require.NoError(t, err) defer os.RemoveAll(hookDir) - defer func(oldDir string) { - config.Config.GitlabShell.Dir = oldDir - }(config.Config.GitlabShell.Dir) - config.Config.GitlabShell.Dir = hookDir + defer func() { hooks.Override = "" }() + hooks.Override = 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 1637d1ffc..e909db224 100644 --- a/internal/service/wiki/testhelper_test.go +++ b/internal/service/wiki/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/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" @@ -39,6 +40,8 @@ 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 1b2979cc2..222c93d3b 100644 --- a/ruby/lib/gitlab/git/hook.rb +++ b/ruby/lib/gitlab/git/hook.rb @@ -7,6 +7,10 @@ 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 6f4bd9910..1a0b7d044 100644 --- a/ruby/lib/gitlab/git/repository.rb +++ b/ruby/lib/gitlab/git/repository.rb @@ -62,10 +62,9 @@ module Gitlab repo = Rugged::Repository.init_at(repo_path, true) repo.close - # 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? + # 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) 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 677be7aa9..4364b8818 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.directory } + let(:target_hooks_dir) { Gitlab::Git::Hook.legacy_hooks_directory } let(:existing_target) { File.join(repo_path, 'foobar') } before do -- cgit v1.2.3