diff options
author | Patrick Steinhardt <psteinhardt@gitlab.com> | 2022-07-20 08:57:08 +0300 |
---|---|---|
committer | Patrick Steinhardt <psteinhardt@gitlab.com> | 2022-07-20 08:57:08 +0300 |
commit | 2c392d6c5f7c7536fb00f0dbc980e78bb8bc4a48 (patch) | |
tree | f90ab6fb3c89bbcc4b99b436d823d15d7d33ac8f | |
parent | 3fc66dc23581de48bdbbf1b5a5d5ca9faf5f925b (diff) | |
parent | 13a18236e716994437345474f1879afca64e3b3f (diff) |
Merge branch 'pks-updateref-ignore-post-receive-custom-hook-error' into 'master'
updateref: Ignore failures of custom post-receive hooks
Closes #3595
See merge request gitlab-org/gitaly!4714
-rw-r--r-- | internal/git/updateref/update_with_hooks.go | 19 | ||||
-rw-r--r-- | internal/git/updateref/update_with_hooks_test.go | 3 | ||||
-rw-r--r-- | internal/gitaly/service/operations/merge_test.go | 56 |
3 files changed, 52 insertions, 26 deletions
diff --git a/internal/git/updateref/update_with_hooks.go b/internal/git/updateref/update_with_hooks.go index 23155bc40..fe5cf75df 100644 --- a/internal/git/updateref/update_with_hooks.go +++ b/internal/git/updateref/update_with_hooks.go @@ -7,6 +7,7 @@ import ( "fmt" "strings" + "github.com/grpc-ecosystem/go-grpc-middleware/logging/logrus/ctxlogrus" "gitlab.com/gitlab-org/gitaly/v15/internal/git" "gitlab.com/gitlab-org/gitaly/v15/internal/git/catfile" "gitlab.com/gitlab-org/gitaly/v15/internal/git/localrepo" @@ -268,7 +269,23 @@ func (u *UpdaterWithHooks) UpdateReference( } if err := u.hookManager.PostReceiveHook(ctx, repo, pushOptions, []string{hooksPayload}, strings.NewReader(changes), &stdout, &stderr); err != nil { - return fmt.Errorf("running post-receive hooks: %w", wrapHookError(err, git.PostReceiveHook, stdout.String(), stderr.String())) + // CustomHook errors are returned in case a custom hook has returned an error code. + // The post-receive hook has special semantics though. Quoting githooks(5): + // + // This hook does not affect the outcome of git receive-pack, as it is called + // after the real work is done. + // + // This means that even if the hook returns an error, then that error should not + // impact whatever git-receive-pack(1) has been doing. And given that we emulate + // behaviour of this command here, we need to behave the same. + var customHookErr hook.CustomHookError + if errors.As(err, &customHookErr) { + // Only log the error when we've got a custom-hook error, but otherwise + // ignore it and continue with whatever we have been doing. + ctxlogrus.Extract(ctx).WithError(err).Error("custom post-receive hook returned an error") + } else { + return fmt.Errorf("running post-receive hooks: %w", wrapHookError(err, git.PostReceiveHook, stdout.String(), stderr.String())) + } } return nil diff --git a/internal/git/updateref/update_with_hooks_test.go b/internal/git/updateref/update_with_hooks_test.go index f04dac5a9..2b759f689 100644 --- a/internal/git/updateref/update_with_hooks_test.go +++ b/internal/git/updateref/update_with_hooks_test.go @@ -230,7 +230,7 @@ func TestUpdaterWithHooks_UpdateReference(t *testing.T) { expectedErr: "reference-transaction failure", }, { - desc: "post-receive error", + desc: "post-receive error is ignored", preReceive: func(t *testing.T, ctx context.Context, repo *gitalypb.Repository, pushOptions, env []string, stdin io.Reader, stdout, stderr io.Writer) error { return nil }, @@ -245,7 +245,6 @@ func TestUpdaterWithHooks_UpdateReference(t *testing.T) { require.NoError(t, err) return errors.New("ignored") }, - expectedErr: "post-receive failure", expectedRefDeletion: true, }, } diff --git a/internal/gitaly/service/operations/merge_test.go b/internal/gitaly/service/operations/merge_test.go index 8eebfbce6..33265a342 100644 --- a/internal/gitaly/service/operations/merge_test.go +++ b/internal/gitaly/service/operations/merge_test.go @@ -440,25 +440,26 @@ func TestUserMergeBranch_failingHooks(t *testing.T) { hookContent := []byte("#!/bin/sh\necho 'stdout' && echo 'stderr' >&2\nexit 1") for _, tc := range []struct { - hookName string - hookType gitalypb.CustomHookError_HookType - shouldUpdate bool + hookName string + hookType gitalypb.CustomHookError_HookType + shouldFail bool }{ { - hookName: "pre-receive", - hookType: gitalypb.CustomHookError_HOOK_TYPE_PRERECEIVE, - shouldUpdate: false, + hookName: "pre-receive", + hookType: gitalypb.CustomHookError_HOOK_TYPE_PRERECEIVE, + shouldFail: true, }, { - hookName: "update", - hookType: gitalypb.CustomHookError_HOOK_TYPE_UPDATE, - shouldUpdate: false, + hookName: "update", + hookType: gitalypb.CustomHookError_HOOK_TYPE_UPDATE, + shouldFail: true, }, { hookName: "post-receive", hookType: gitalypb.CustomHookError_HOOK_TYPE_POSTRECEIVE, - // The post-receive hook runs after references have been updated. - shouldUpdate: true, + // The post-receive hook runs after references have been updated and any + // failures of it are ignored. + shouldFail: false, }, } { t.Run(tc.hookName, func(t *testing.T) { @@ -485,22 +486,31 @@ func TestUserMergeBranch_failingHooks(t *testing.T) { require.NoError(t, mergeBidi.CloseSend(), "close send") secondResponse, err := mergeBidi.Recv() - testhelper.RequireGrpcError(t, errWithDetails(t, - helper.ErrPermissionDeniedf("stderr\n"), - &gitalypb.UserMergeBranchError{ - Error: &gitalypb.UserMergeBranchError_CustomHook{ - CustomHook: &gitalypb.CustomHookError{ - HookType: tc.hookType, - Stdout: []byte("stdout\n"), - Stderr: []byte("stderr\n"), + if tc.shouldFail { + testhelper.RequireGrpcError(t, errWithDetails(t, + helper.ErrPermissionDeniedf("stderr\n"), + &gitalypb.UserMergeBranchError{ + Error: &gitalypb.UserMergeBranchError_CustomHook{ + CustomHook: &gitalypb.CustomHookError{ + HookType: tc.hookType, + Stdout: []byte("stdout\n"), + Stderr: []byte("stderr\n"), + }, }, }, - }, - ), err) - require.Nil(t, secondResponse) + ), err) + require.Nil(t, secondResponse) + } else { + testhelper.ProtoEqual(t, &gitalypb.UserMergeBranchResponse{ + BranchUpdate: &gitalypb.OperationBranchUpdate{ + CommitId: firstResponse.CommitId, + }, + }, secondResponse) + require.NoError(t, err) + } currentBranchHead := gittest.Exec(t, cfg, "-C", repoPath, "rev-parse", mergeBranchName) - if tc.shouldUpdate { + if !tc.shouldFail { require.Equal(t, firstResponse.CommitId, text.ChompBytes(currentBranchHead), "branch head updated") } else { require.Equal(t, mergeBranchHeadBefore, text.ChompBytes(currentBranchHead), "branch head updated") |