Merge branch 'jc-remove-user-rebase-confirmable-ff' into 'master'

operations: Remove UserRebaseConfirmableImprovedErrorHandling FF

See merge request gitlab-org/gitaly!4746

3 files changed, 60 insertions, 111 deletions

diff --git a/internal/gitaly/service/operations/rebase.go b/internal/gitaly/service/operations/rebase.go
index 4edfe2863..af727c1ad 100644
--- a/internal/gitaly/service/operations/rebase.go
+++ b/internal/gitaly/service/operations/rebase.go
@@ -9,7 +9,6 @@ import (
 	"gitlab.com/gitlab-org/gitaly/v15/internal/git/updateref"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/git2go"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/helper"
-	"gitlab.com/gitlab-org/gitaly/v15/internal/metadata/featureflag"
 	"gitlab.com/gitlab-org/gitaly/v15/proto/go/gitalypb"
 	"google.golang.org/grpc/codes"
 	"google.golang.org/grpc/status"
@@ -68,41 +67,35 @@ 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,
-								ConflictingCommitIds: []string{
-									startRevision.String(),
-									oldrev.String(),
-								},
+		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,
+							ConflictingCommitIds: []string{
+								startRevision.String(),
+								oldrev.String(),
 							},
 						},
 					},
-				)
-				if err != nil {
-					return helper.ErrInternalf("error details: %w", err)
-				}
-
-				return detailedErr
+				},
+			)
+			if err != nil {
+				return helper.ErrInternalf("error details: %w", err)
 			}
 
-			return helper.ErrInternalf("rebasing commits: %w", err)
+			return detailedErr
 		}
 
-		return stream.Send(&gitalypb.UserRebaseConfirmableResponse{
-			GitError: err.Error(),
-		})
+		return helper.ErrInternalf("rebasing commits: %w", err)
 	}
 
 	if err := stream.Send(&gitalypb.UserRebaseConfirmableResponse{
@@ -135,25 +128,20 @@ func (s *Server) UserRebaseConfirmable(stream gitalypb.OperationService_UserReba
 		var customHookErr updateref.CustomHookError
 		switch {
 		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: customHookErr.Error(),
-							},
+			detailedErr, err := helper.ErrWithDetails(
+				helper.ErrPermissionDeniedf("access check: %q", err),
+				&gitalypb.UserRebaseConfirmableError{
+					Error: &gitalypb.UserRebaseConfirmableError_AccessCheck{
+						AccessCheck: &gitalypb.AccessCheckError{
+							ErrorMessage: customHookErr.Error(),
 						},
 					},
-				)
-				if err != nil {
-					return helper.ErrInternalf("error details: %w", err)
-				}
-				return detailedErr
+				},
+			)
+			if err != nil {
+				return helper.ErrInternalf("error details: %w", err)
 			}
-			return stream.Send(&gitalypb.UserRebaseConfirmableResponse{
-				PreReceiveError: err.Error(),
-			})
+			return detailedErr
 		case errors.Is(err, git2go.ErrInvalidArgument):
 			return fmt.Errorf("update ref: %w", err)
 		}
diff --git a/internal/gitaly/service/operations/rebase_test.go b/internal/gitaly/service/operations/rebase_test.go
index a21af7644..ba371aae7 100644
--- a/internal/gitaly/service/operations/rebase_test.go
+++ b/internal/gitaly/service/operations/rebase_test.go
@@ -3,7 +3,6 @@
 package operations
 
 import (
-	"context"
 	"errors"
 	"fmt"
 	"io"
@@ -17,7 +16,6 @@ import (
 	"gitlab.com/gitlab-org/gitaly/v15/internal/gitaly/transaction"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/helper"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/metadata"
-	"gitlab.com/gitlab-org/gitaly/v15/internal/metadata/featureflag"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/testhelper"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/testhelper/testserver"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/transaction/txinfo"
@@ -511,14 +509,8 @@ func TestUserRebaseConfirmable_abortViaApply(t *testing.T) {
 
 func TestUserRebaseConfirmable_preReceiveError(t *testing.T) {
 	t.Parallel()
-	testhelper.NewFeatureSets(featureflag.UserRebaseConfirmableImprovedErrorHandling).
-		Run(t, testUserRebaseConfirmablePreReceiveError)
-}
-
-func testUserRebaseConfirmablePreReceiveError(t *testing.T, ctx context.Context) {
-	t.Parallel()
 
-	ctx, cfg, repoProto, repoPath, client := setupOperationsService(t, ctx)
+	ctx, cfg, repoProto, repoPath, client := setupOperationsService(t, testhelper.Context(t))
 	repo := localrepo.NewTestRepo(t, cfg, repoProto)
 
 	repoCopyProto, _ := gittest.CreateRepository(ctx, t, cfg, gittest.CreateRepositoryConfig{
@@ -549,24 +541,18 @@ func testUserRebaseConfirmablePreReceiveError(t *testing.T, ctx context.Context)
 			require.NoError(t, rebaseStream.Send(applyRequest), "apply rebase")
 
 			secondResponse, err := rebaseStream.Recv()
-
-			if featureflag.UserRebaseConfirmableImprovedErrorHandling.IsEnabled(ctx) {
-				testhelper.RequireGrpcError(t, errWithDetails(t,
-					helper.ErrPermissionDeniedf(`access check: "running %s hooks: failure\n"`, hookName),
-					&gitalypb.UserRebaseConfirmableError{
-						Error: &gitalypb.UserRebaseConfirmableError_AccessCheck{
-							AccessCheck: &gitalypb.AccessCheckError{
-								ErrorMessage: "failure\n",
-							},
+			require.Nil(t, secondResponse)
+
+			testhelper.RequireGrpcError(t, errWithDetails(t,
+				helper.ErrPermissionDeniedf(`access check: "running %s hooks: failure\n"`, hookName),
+				&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)
 
 			_, err = repo.ReadCommit(ctx, git.Revision(firstResponse.GetRebaseSha()))
 			if hookName == "pre-receive" {
@@ -584,14 +570,8 @@ func testUserRebaseConfirmablePreReceiveError(t *testing.T, ctx context.Context)
 
 func TestUserRebaseConfirmable_gitError(t *testing.T) {
 	t.Parallel()
-	testhelper.NewFeatureSets(featureflag.UserRebaseConfirmableImprovedErrorHandling).
-		Run(t, testFailedUserRebaseConfirmableDueToGitError)
-}
-
-func testFailedUserRebaseConfirmableDueToGitError(t *testing.T, ctx context.Context) {
-	t.Parallel()
 
-	ctx, cfg, repoProto, repoPath, client := setupOperationsService(t, ctx)
+	ctx, cfg, repoProto, repoPath, client := setupOperationsService(t, testhelper.Context(t))
 
 	repoCopyProto, _ := gittest.CreateRepository(ctx, t, cfg, gittest.CreateRepositoryConfig{
 		Seed: gittest.SeedGitLabTest,
@@ -608,31 +588,24 @@ func testFailedUserRebaseConfirmableDueToGitError(t *testing.T, ctx context.Cont
 
 	require.NoError(t, rebaseStream.Send(headerRequest), "send header")
 
-	firstResponse, err := rebaseStream.Recv()
-	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"),
-						},
-						ConflictingCommitIds: []string{
-							sourceBranchCommitID.String(),
-							targetBranchCommitID.String(),
-						},
+	response, err := rebaseStream.Recv()
+	require.Nil(t, response)
+	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"),
+					},
+					ConflictingCommitIds: []string{
+						sourceBranchCommitID.String(),
+						targetBranchCommitID.String(),
 					},
 				},
 			},
-		), 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)
 
 	newBranchCommitID := gittest.ResolveRevision(t, cfg, repoPath, targetBranch)
 	require.Equal(t, targetBranchCommitID, newBranchCommitID, "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
deleted file mode 100644
index 1065b92e8..000000000
--- a/internal/metadata/featureflag/ff_user_rebase_confirmable_improved_error_handling.go
+++ /dev/null
@@ -1,12 +0,0 @@
-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",
-	"v14.10.0",
-	"https://gitlab.com/gitlab-org/gitaly/-/issues/4326",
-	false,
-)