diff options
author | Sami Hiltunen <shiltunen@gitlab.com> | 2022-07-19 21:32:07 +0300 |
---|---|---|
committer | Sami Hiltunen <shiltunen@gitlab.com> | 2022-07-19 21:32:07 +0300 |
commit | 3fc66dc23581de48bdbbf1b5a5d5ca9faf5f925b (patch) | |
tree | 8321e145a6df47989c3eeaa2c7dba5e8ee29f1f0 | |
parent | f296999a3522bdc5304f2cbc91cdf341d1d2bb3c (diff) | |
parent | d2bfdfba34c323cc9ae0f9580659d5eadc0374d6 (diff) |
Merge branch 'pks-updateref-improved-errors' into 'master'
updateref: Improve errors to include breadcrumbs
See merge request gitlab-org/gitaly!4713
-rw-r--r-- | internal/git/updateref/update_with_hooks.go | 19 | ||||
-rw-r--r-- | internal/git/updateref/update_with_hooks_test.go | 14 | ||||
-rw-r--r-- | internal/gitaly/service/conflicts/resolve_conflicts_test.go | 2 | ||||
-rw-r--r-- | internal/gitaly/service/operations/branches_test.go | 8 | ||||
-rw-r--r-- | internal/gitaly/service/operations/cherry_pick.go | 11 | ||||
-rw-r--r-- | internal/gitaly/service/operations/rebase.go | 8 | ||||
-rw-r--r-- | internal/gitaly/service/operations/rebase_test.go | 2 |
7 files changed, 34 insertions, 30 deletions
diff --git a/internal/git/updateref/update_with_hooks.go b/internal/git/updateref/update_with_hooks.go index fbd615be4..23155bc40 100644 --- a/internal/git/updateref/update_with_hooks.go +++ b/internal/git/updateref/update_with_hooks.go @@ -15,7 +15,6 @@ import ( "gitlab.com/gitlab-org/gitaly/v15/internal/gitaly/config" "gitlab.com/gitlab-org/gitaly/v15/internal/gitaly/hook" "gitlab.com/gitlab-org/gitaly/v15/internal/gitaly/storage" - "gitlab.com/gitlab-org/gitaly/v15/internal/helper" "gitlab.com/gitlab-org/gitaly/v15/internal/metadata/featureflag" "gitlab.com/gitlab-org/gitaly/v15/internal/transaction/txinfo" "gitlab.com/gitlab-org/gitaly/v15/proto/go/gitalypb" @@ -157,17 +156,17 @@ func (u *UpdaterWithHooks) UpdateReference( if tx, err := txinfo.TransactionFromContext(ctx); err == nil { transaction = &tx } else if !errors.Is(err, txinfo.ErrTransactionNotFound) { - return err + return fmt.Errorf("getting transaction: %w", err) } if reference == "" { - return helper.ErrInternalf("UpdateReference: got no reference") + return fmt.Errorf("reference cannot be empty") } if err := git.ValidateObjectID(oldrev.String()); err != nil { - return helper.ErrInternalf("UpdateReference: got invalid old value: %w", err) + return fmt.Errorf("validating old value: %w", err) } if err := git.ValidateObjectID(newrev.String()); err != nil { - return helper.ErrInternalf("UpdateReference: got invalid new value: %w", err) + return fmt.Errorf("validating new value: %w", err) } changes := fmt.Sprintf("%s %s %s\n", oldrev, newrev, reference) @@ -190,12 +189,12 @@ func (u *UpdaterWithHooks) UpdateReference( hooksPayload, err := git.NewHooksPayload(u.cfg, quarantinedRepo, transaction, &receiveHooksPayload, git.ReceivePackHooks, featureflag.FromContext(ctx)).Env() if err != nil { - return err + return fmt.Errorf("constructing hooks payload: %w", err) } var stdout, stderr bytes.Buffer if err := u.hookManager.PreReceiveHook(ctx, quarantinedRepo, pushOptions, []string{hooksPayload}, strings.NewReader(changes), &stdout, &stderr); err != nil { - return wrapHookError(err, git.PreReceiveHook, stdout.String(), stderr.String()) + return fmt.Errorf("running pre-receive hooks: %w", wrapHookError(err, git.PreReceiveHook, stdout.String(), stderr.String())) } // Now that Rails has told us that the change is okay via the pre-receive hook, we can @@ -212,12 +211,12 @@ func (u *UpdaterWithHooks) UpdateReference( // real repository anyway. hooksPayload, err = git.NewHooksPayload(u.cfg, repo, transaction, &receiveHooksPayload, git.ReceivePackHooks, featureflag.FromContext(ctx)).Env() if err != nil { - return err + return fmt.Errorf("constructing quarantined hooks payload: %w", err) } } if err := u.hookManager.UpdateHook(ctx, quarantinedRepo, reference.String(), oldrev.String(), newrev.String(), []string{hooksPayload}, &stdout, &stderr); err != nil { - return wrapHookError(err, git.UpdateHook, stdout.String(), stderr.String()) + return fmt.Errorf("running update hooks: %w", wrapHookError(err, git.UpdateHook, stdout.String(), stderr.String())) } // We are already manually invoking the reference-transaction hook, so there is no need to @@ -269,7 +268,7 @@ func (u *UpdaterWithHooks) UpdateReference( } if err := u.hookManager.PostReceiveHook(ctx, repo, pushOptions, []string{hooksPayload}, strings.NewReader(changes), &stdout, &stderr); err != nil { - return wrapHookError(err, git.PostReceiveHook, stdout.String(), stderr.String()) + 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 592f1526b..f04dac5a9 100644 --- a/internal/git/updateref/update_with_hooks_test.go +++ b/internal/git/updateref/update_with_hooks_test.go @@ -48,46 +48,46 @@ func TestUpdaterWithHooks_UpdateReference_invalidParameters(t *testing.T) { desc string ref git.ReferenceName newRev, oldRev git.ObjectID - expectedErr string + expectedErr error }{ { desc: "missing reference", oldRev: revA, newRev: revB, - expectedErr: "got no reference", + expectedErr: fmt.Errorf("reference cannot be empty"), }, { desc: "missing old rev", ref: "refs/heads/master", newRev: revB, - expectedErr: "got invalid old value", + expectedErr: fmt.Errorf("validating old value: %w", fmt.Errorf("%w: %q", git.ErrInvalidObjectID, "")), }, { desc: "missing new rev", ref: "refs/heads/master", oldRev: revB, - expectedErr: "got invalid new value", + expectedErr: fmt.Errorf("validating new value: %w", fmt.Errorf("%w: %q", git.ErrInvalidObjectID, "")), }, { desc: "invalid old rev", ref: "refs/heads/master", newRev: revA, oldRev: "foobar", - expectedErr: "got invalid old value", + expectedErr: fmt.Errorf("validating old value: %w", fmt.Errorf("%w: %q", git.ErrInvalidObjectID, "foobar")), }, { desc: "invalid new rev", ref: "refs/heads/master", newRev: "foobar", oldRev: revB, - expectedErr: "got invalid new value", + expectedErr: fmt.Errorf("validating new value: %w", fmt.Errorf("%w: %q", git.ErrInvalidObjectID, "foobar")), }, } for _, tc := range testCases { t.Run(tc.desc, func(t *testing.T) { err := updater.UpdateReference(ctx, repo, user, nil, tc.ref, tc.newRev, tc.oldRev) - require.Contains(t, err.Error(), tc.expectedErr) + require.Equal(t, tc.expectedErr, err) }) } } diff --git a/internal/gitaly/service/conflicts/resolve_conflicts_test.go b/internal/gitaly/service/conflicts/resolve_conflicts_test.go index 83ccc258e..6dfda1eab 100644 --- a/internal/gitaly/service/conflicts/resolve_conflicts_test.go +++ b/internal/gitaly/service/conflicts/resolve_conflicts_test.go @@ -891,7 +891,7 @@ func TestResolveConflictsQuarantine(t *testing.T) { })) response, err := stream.CloseAndRecv() - require.EqualError(t, err, `rpc error: code = Unknown desc = af339cb882d1e3cf8d6751651e58bbaff0265d6e + require.EqualError(t, err, `rpc error: code = Unknown desc = running pre-receive hooks: af339cb882d1e3cf8d6751651e58bbaff0265d6e tree 89fad81bbfa38070b90ca8f4c404625bf0999013 parent 29449b1d52cd77fd060a083a1de691bbaf12d8af parent 26dac52be85c92742b2c0c19eb7303de9feccb63 diff --git a/internal/gitaly/service/operations/branches_test.go b/internal/gitaly/service/operations/branches_test.go index 98444630c..3909632a0 100644 --- a/internal/gitaly/service/operations/branches_test.go +++ b/internal/gitaly/service/operations/branches_test.go @@ -458,7 +458,7 @@ func TestUserDeleteBranch_allowed(t *testing.T) { return false, "something something", nil }, expectedErr: errWithDetails(t, - helper.ErrPermissionDeniedf("deletion denied by access checks: GitLab: something something"), + helper.ErrPermissionDeniedf("deletion denied by access checks: running pre-receive hooks: GitLab: something something"), &gitalypb.UserDeleteBranchError{ Error: &gitalypb.UserDeleteBranchError_AccessCheck{ AccessCheck: &gitalypb.AccessCheckError{ @@ -479,7 +479,7 @@ func TestUserDeleteBranch_allowed(t *testing.T) { return false, "something something", fmt.Errorf("something else") }, expectedErr: errWithDetails(t, - helper.ErrPermissionDeniedf("deletion denied by access checks: GitLab: something else"), + helper.ErrPermissionDeniedf("deletion denied by access checks: running pre-receive hooks: GitLab: something else"), &gitalypb.UserDeleteBranchError{ Error: &gitalypb.UserDeleteBranchError_AccessCheck{ AccessCheck: &gitalypb.AccessCheckError{ @@ -726,7 +726,7 @@ func TestUserDeleteBranch_hookFailure(t *testing.T) { response, err := client.UserDeleteBranch(ctx, request) testhelper.RequireGrpcError(t, errWithDetails(t, - helper.ErrPermissionDeniedf("deletion denied by custom hooks: %s\n", "GL_ID=user-123"), + helper.ErrPermissionDeniedf("deletion denied by custom hooks: running %s hooks: %s\n", tc.hookName, "GL_ID=user-123"), &gitalypb.UserDeleteBranchError{ Error: &gitalypb.UserDeleteBranchError_CustomHook{ CustomHook: &gitalypb.CustomHookError{ @@ -841,7 +841,7 @@ func TestBranchHookOutput(t *testing.T) { deleteResponse, err := client.UserDeleteBranch(ctx, deleteRequest) testhelper.RequireGrpcError(t, errWithDetails(t, - helper.ErrPermissionDeniedf("deletion denied by custom hooks: %s", expectedError), + helper.ErrPermissionDeniedf("deletion denied by custom hooks: running %s hooks: %s", hookTestCase.hookName, expectedError), &gitalypb.UserDeleteBranchError{ Error: &gitalypb.UserDeleteBranchError_CustomHook{ CustomHook: &gitalypb.CustomHookError{ diff --git a/internal/gitaly/service/operations/cherry_pick.go b/internal/gitaly/service/operations/cherry_pick.go index b35a078b8..7b8994479 100644 --- a/internal/gitaly/service/operations/cherry_pick.go +++ b/internal/gitaly/service/operations/cherry_pick.go @@ -175,13 +175,15 @@ func (s *Server) UserCherryPick(ctx context.Context, req *gitalypb.UserCherryPic if err := s.updateReferenceWithHooks(ctx, req.GetRepository(), req.User, quarantineDir, referenceName, newrev, oldrev); err != nil { if featureflag.CherryPickStructuredErrors.IsEnabled(ctx) { - if errors.As(err, &updateref.CustomHookError{}) { + var customHookErr updateref.CustomHookError + + if errors.As(err, &customHookErr) { detailedErr, errGeneratingDetailedErr := helper.ErrWithDetails( helper.ErrFailedPrecondition(errors.New("access check failed")), &gitalypb.UserCherryPickError{ Error: &gitalypb.UserCherryPickError_AccessCheck{ AccessCheck: &gitalypb.AccessCheckError{ - ErrorMessage: strings.TrimSuffix(err.Error(), "\n"), + ErrorMessage: strings.TrimSuffix(customHookErr.Error(), "\n"), }, }, }) @@ -196,9 +198,10 @@ func (s *Server) UserCherryPick(ctx context.Context, req *gitalypb.UserCherryPic return nil, fmt.Errorf("update reference with hooks: %w", err) } - if errors.As(err, &updateref.CustomHookError{}) { + var customHookErr updateref.CustomHookError + if errors.As(err, &customHookErr) { return &gitalypb.UserCherryPickResponse{ - PreReceiveError: err.Error(), + PreReceiveError: customHookErr.Error(), }, nil } diff --git a/internal/gitaly/service/operations/rebase.go b/internal/gitaly/service/operations/rebase.go index 8e1340fce..1ab3458e8 100644 --- a/internal/gitaly/service/operations/rebase.go +++ b/internal/gitaly/service/operations/rebase.go @@ -130,16 +130,18 @@ func (s *Server) UserRebaseConfirmable(stream gitalypb.OperationService_UserReba branch, newrev, oldrev, - header.GitPushOptions...); err != nil { + header.GitPushOptions..., + ); err != nil { + var customHookErr updateref.CustomHookError switch { - case errors.As(err, &updateref.CustomHookError{}): + case errors.As(err, &customHookErr): if featureflag.UserRebaseConfirmableImprovedErrorHandling.IsEnabled(ctx) { detailedErr, err := helper.ErrWithDetails( helper.ErrPermissionDeniedf("access check: %q", err), &gitalypb.UserRebaseConfirmableError{ Error: &gitalypb.UserRebaseConfirmableError_AccessCheck{ AccessCheck: &gitalypb.AccessCheckError{ - ErrorMessage: err.Error(), + ErrorMessage: customHookErr.Error(), }, }, }, diff --git a/internal/gitaly/service/operations/rebase_test.go b/internal/gitaly/service/operations/rebase_test.go index 0455bee51..a21af7644 100644 --- a/internal/gitaly/service/operations/rebase_test.go +++ b/internal/gitaly/service/operations/rebase_test.go @@ -552,7 +552,7 @@ func testUserRebaseConfirmablePreReceiveError(t *testing.T, ctx context.Context) if featureflag.UserRebaseConfirmableImprovedErrorHandling.IsEnabled(ctx) { testhelper.RequireGrpcError(t, errWithDetails(t, - helper.ErrPermissionDeniedf(`access check: "failure\n"`), + helper.ErrPermissionDeniedf(`access check: "running %s hooks: failure\n"`, hookName), &gitalypb.UserRebaseConfirmableError{ Error: &gitalypb.UserRebaseConfirmableError_AccessCheck{ AccessCheck: &gitalypb.AccessCheckError{ |