From 4f376cf16e64a0fa673821c0eb4a56eca9ca9419 Mon Sep 17 00:00:00 2001 From: Jacob Vosmaer Date: Thu, 7 Mar 2019 17:08:06 +0000 Subject: [PATCH 1/2] Revert "Merge branch 'switch-to-embedded-hooks' into 'master'" This reverts merge request !1088 --- internal/config/config_test.go | 11 +++------- internal/config/ruby.go | 7 ------- internal/git/hooks/hooks.go | 12 +++++++++-- internal/git/hooks/hooks_test.go | 20 ++++++++++++++++--- internal/service/conflicts/testhelper_test.go | 2 -- .../service/operations/testhelper_test.go | 12 +++++++++-- .../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 +- 12 files changed, 53 insertions(+), 39 deletions(-) diff --git a/internal/config/config_test.go b/internal/config/config_test.go index 6a8bcb5d05..f02221f4e9 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 c10cb4e842..7e73ec0b25 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 d7df23a8ac..9357f0fe53 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 ce69896360..655366b7d2 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 482fc0631e..713a971b88 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 5649ed514d..3a5408f6b7 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 6525aa72e3..ef083b67de 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 c42c54c836..d69bdb002b 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 e909db2240..1637d1ffc9 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 222c93d3bb..1b2979cc27 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 1a0b7d0441..6f4bd99106 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 4364b8818a..677be7aa96 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 -- GitLab From d9af10d78fa788a4c7d35fab70f3a845849197d1 Mon Sep 17 00:00:00 2001 From: Jacob Vosmaer Date: Thu, 7 Mar 2019 18:13:35 +0100 Subject: [PATCH 2/2] changelog --- changelogs/unreleased/revert-28330762.yml | 6 ++++++ 1 file changed, 6 insertions(+) create mode 100644 changelogs/unreleased/revert-28330762.yml diff --git a/changelogs/unreleased/revert-28330762.yml b/changelogs/unreleased/revert-28330762.yml new file mode 100644 index 0000000000..6876d5bab2 --- /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 -- GitLab