diff --git a/changelogs/unreleased/re-apply-hooks-change.yml b/changelogs/unreleased/re-apply-hooks-change.yml new file mode 100644 index 0000000000000000000000000000000000000000..99ce156e750cab5983f75187d19ba10519080709 --- /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 f02221f4e90854371812ece329071f120af5be3e..6a8bcb5d05fe280026b6f6ba73f7938b6df53df3 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 7e73ec0b2595b387588e5e2b6693d34e5b2779d4..c10cb4e84285ee526b90f4f30b0589814d551893 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 9357f0fe53b6c84e8a756bb8b9f78906114dad77..c77c3ed1e373ba8f7f080901f2b0d7492c1b9f2e 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 655366b7d2fe126e7dc0216d8011bd49dfc71d85..6c26574616b5c01cab5158f394f71cae663d7f05 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 713a971b889b8ad00220f24346ce627f6a131150..482fc0631e3395e4fefd72a30ac5742f17716fd4 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 3a5408f6b767eda3124165c40cd5c3ca98a9705d..5649ed514d5272d88327dc9e5481f0df63bd7ae8 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 ef083b67de63dc0120cbb7decaf83287d07b1ab3..6525aa72e3e2d2a67324cb117c085478ab5373f5 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 d69bdb002b0e2fb9736b7b51018c3e14bb529466..c42c54c8365dcd4cb0977fc40eb68009c8a99ff5 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 1637d1ffc96cc1a88593644bd51d4427243306c7..e909db22400df4cb5cbb02b314d6e4b740b8ed2c 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 1b2979cc276af7a38ce9ee5210b89b3a3704cbcd..222c93d3bb34f43ab19ea0bf697f152302c24748 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 6f4bd99106a5465b74ba4c71e7e84a1dbc6d4183..1a0b7d044101b1c3686c4fb8414548a02269581e 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 677be7aa960df678ccd407a3bb4e5127bd8e4ce8..4364b8818aafe55ef4e6e48a8af73e7c61d4381d 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