diff options
author | Toon Claes <toon@gitlab.com> | 2022-03-23 17:22:31 +0300 |
---|---|---|
committer | Toon Claes <toon@gitlab.com> | 2022-03-23 17:22:31 +0300 |
commit | 57048c3d003ebf72ba8342a03b2f6d510193e49e (patch) | |
tree | fb68d89e6a0544a17ea4db9c8352b8393ec083ad /internal | |
parent | 251ee0c20a8898d8165bf5b0028b85eb11e6c61c (diff) | |
parent | 3c3f7c2a148d299aef0b6bb6ff3d1ab9a5a883ba (diff) |
Merge branch 'jc-rebase-cofirmable-structured-errors' into 'master'
repository: Structured errors for UserRebaseConfirmable
See merge request gitlab-org/gitaly!4419
Diffstat (limited to 'internal')
3 files changed, 100 insertions, 12 deletions
diff --git a/internal/gitaly/service/operations/rebase.go b/internal/gitaly/service/operations/rebase.go index 30c4438cb..e23a18fe9 100644 --- a/internal/gitaly/service/operations/rebase.go +++ b/internal/gitaly/service/operations/rebase.go @@ -9,6 +9,7 @@ import ( "gitlab.com/gitlab-org/gitaly/v14/internal/git/updateref" "gitlab.com/gitlab-org/gitaly/v14/internal/git2go" "gitlab.com/gitlab-org/gitaly/v14/internal/helper" + "gitlab.com/gitlab-org/gitaly/v14/internal/metadata/featureflag" "gitlab.com/gitlab-org/gitaly/v14/proto/go/gitalypb" "google.golang.org/grpc/codes" "google.golang.org/grpc/status" @@ -67,6 +68,34 @@ func (s *Server) UserRebaseConfirmable(stream gitalypb.OperationService_UserReba SkipEmptyCommits: true, }) if err != nil { + if featureflag.UserRebaseConfirmableImprovedErrorHandling.IsEnabled(ctx) { + var conflictErr git2go.ConflictingFilesError + if errors.As(err, &conflictErr) { + conflictingFiles := make([][]byte, 0, len(conflictErr.ConflictingFiles)) + for _, conflictingFile := range conflictErr.ConflictingFiles { + conflictingFiles = append(conflictingFiles, []byte(conflictingFile)) + } + + detailedErr, err := helper.ErrWithDetails( + helper.ErrFailedPreconditionf("rebasing commits: %w", err), + &gitalypb.UserRebaseConfirmableError{ + Error: &gitalypb.UserRebaseConfirmableError_RebaseConflict{ + RebaseConflict: &gitalypb.MergeConflictError{ + ConflictingFiles: conflictingFiles, + }, + }, + }, + ) + if err != nil { + return helper.ErrInternalf("error details: %w", err) + } + + return detailedErr + } + + return helper.ErrInternalf("rebasing commits: %w", err) + } + return stream.Send(&gitalypb.UserRebaseConfirmableResponse{ GitError: err.Error(), }) @@ -100,6 +129,22 @@ func (s *Server) UserRebaseConfirmable(stream gitalypb.OperationService_UserReba header.GitPushOptions...); err != nil { switch { case errors.As(err, &updateref.HookError{}): + 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(), + }, + }, + }, + ) + if err != nil { + return helper.ErrInternalf("error details: %w", err) + } + return detailedErr + } return stream.Send(&gitalypb.UserRebaseConfirmableResponse{ PreReceiveError: err.Error(), }) diff --git a/internal/gitaly/service/operations/rebase_test.go b/internal/gitaly/service/operations/rebase_test.go index 1d388ded7..a43f084f3 100644 --- a/internal/gitaly/service/operations/rebase_test.go +++ b/internal/gitaly/service/operations/rebase_test.go @@ -1,6 +1,8 @@ package operations import ( + "context" + "errors" "fmt" "io" "path/filepath" @@ -14,7 +16,9 @@ import ( "gitlab.com/gitlab-org/gitaly/v14/internal/git/localrepo" "gitlab.com/gitlab-org/gitaly/v14/internal/gitaly/config" "gitlab.com/gitlab-org/gitaly/v14/internal/gitaly/transaction" + "gitlab.com/gitlab-org/gitaly/v14/internal/helper" "gitlab.com/gitlab-org/gitaly/v14/internal/metadata" + "gitlab.com/gitlab-org/gitaly/v14/internal/metadata/featureflag" "gitlab.com/gitlab-org/gitaly/v14/internal/testhelper" "gitlab.com/gitlab-org/gitaly/v14/internal/testhelper/testserver" "gitlab.com/gitlab-org/gitaly/v14/internal/transaction/txinfo" @@ -507,8 +511,11 @@ func TestFailedUserRebaseConfirmableDueToApplyBeingFalse(t *testing.T) { func TestFailedUserRebaseConfirmableRequestDueToPreReceiveError(t *testing.T) { t.Parallel() - ctx := testhelper.Context(t) + testhelper.NewFeatureSets(featureflag.UserRebaseConfirmableImprovedErrorHandling). + Run(t, testFailedUserRebaseConfirmableRequestDueToPreReceiveError) +} +func testFailedUserRebaseConfirmableRequestDueToPreReceiveError(t *testing.T, ctx context.Context) { ctx, cfg, repoProto, repoPath, client := setupOperationsService(t, ctx) repo := localrepo.NewTestRepo(t, cfg, repoProto) @@ -541,11 +548,23 @@ func TestFailedUserRebaseConfirmableRequestDueToPreReceiveError(t *testing.T) { secondResponse, err := rebaseStream.Recv() - require.NoError(t, err, "receive second response") - require.Contains(t, secondResponse.PreReceiveError, "failure") - - _, err = rebaseStream.Recv() - require.Equal(t, io.EOF, err) + if featureflag.UserRebaseConfirmableImprovedErrorHandling.IsEnabled(ctx) { + testhelper.RequireGrpcError(t, errWithDetails(t, + helper.ErrPermissionDeniedf(`access check: "failure\n"`), + &gitalypb.UserRebaseConfirmableError{ + Error: &gitalypb.UserRebaseConfirmableError_AccessCheck{ + AccessCheck: &gitalypb.AccessCheckError{ + ErrorMessage: "failure\n", + }, + }, + }, + ), err) + } else { + require.NoError(t, err, "receive second response") + require.Contains(t, secondResponse.PreReceiveError, "failure") + _, err = rebaseStream.Recv() + require.Equal(t, io.EOF, err) + } _, err = repo.ReadCommit(ctx, git.Revision(firstResponse.GetRebaseSha())) if hookName == "pre-receive" { @@ -561,10 +580,13 @@ func TestFailedUserRebaseConfirmableRequestDueToPreReceiveError(t *testing.T) { } } -func TestFailedUserRebaseConfirmableDueToGitError(t *testing.T) { +func TestUserRebaseConfirmable_gitError(t *testing.T) { t.Parallel() - ctx := testhelper.Context(t) + testhelper.NewFeatureSets(featureflag.UserRebaseConfirmableImprovedErrorHandling). + Run(t, testFailedUserRebaseConfirmableDueToGitError) +} +func testFailedUserRebaseConfirmableDueToGitError(t *testing.T, ctx context.Context) { ctx, cfg, repoProto, repoPath, client := setupOperationsService(t, ctx) repoCopyProto, _ := gittest.CreateRepository(ctx, t, cfg, gittest.CreateRepositoryConfig{ @@ -578,14 +600,28 @@ func TestFailedUserRebaseConfirmableDueToGitError(t *testing.T) { require.NoError(t, err) headerRequest := buildHeaderRequest(repoProto, gittest.TestUser, "1", failedBranchName, branchSha, repoCopyProto, "master") + require.NoError(t, rebaseStream.Send(headerRequest), "send header") firstResponse, err := rebaseStream.Recv() - require.NoError(t, err, "receive first response") - require.Contains(t, firstResponse.GitError, "conflict") + if featureflag.UserRebaseConfirmableImprovedErrorHandling.IsEnabled(ctx) { + testhelper.RequireGrpcError(t, errWithDetails(t, + helper.ErrFailedPrecondition(errors.New(`rebasing commits: rebase: commit "eb8f5fb9523b868cef583e09d4bf70b99d2dd404": there are conflicting files`)), + &gitalypb.UserRebaseConfirmableError{ + Error: &gitalypb.UserRebaseConfirmableError_RebaseConflict{ + RebaseConflict: &gitalypb.MergeConflictError{ + ConflictingFiles: [][]byte{[]byte("README.md")}, + }, + }, + }, + ), err) + } else { + require.NoError(t, err, "receive first response") + require.Contains(t, firstResponse.GitError, "conflict") - _, err = rebaseStream.Recv() - require.Equal(t, io.EOF, err) + _, err = rebaseStream.Recv() + require.Equal(t, io.EOF, err) + } newBranchSha := getBranchSha(t, cfg, repoPath, failedBranchName) require.Equal(t, branchSha, newBranchSha, "branch should not change when the rebase fails due to GitError") diff --git a/internal/metadata/featureflag/ff_user_rebase_confirmable_improved_error_handling.go b/internal/metadata/featureflag/ff_user_rebase_confirmable_improved_error_handling.go new file mode 100644 index 000000000..9f49b4559 --- /dev/null +++ b/internal/metadata/featureflag/ff_user_rebase_confirmable_improved_error_handling.go @@ -0,0 +1,7 @@ +package featureflag + +// UserRebaseConfirmableImprovedErrorHandling enables proper error handling in the UserRebaseConfirmable +// RPC. When this flag is disabled many error cases were returning successfully with an error message +// embedded in the response. With this flag enabled, this is converted to return real gRPC errors with +// structured errors. +var UserRebaseConfirmableImprovedErrorHandling = NewFeatureFlag("user_rebase_confirmable_improved_error_handling", false) |