Merge branch 'jc/delete-delete-refs-ff' into 'master'

ref: Delete DeleteRefsStructuredErrors feature flag

See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/5726

Merged-by: John Cai <jcai@gitlab.com>
Approved-by: Pavlo Strokov <pstrokov@gitlab.com>
Approved-by: karthik nayak <knayak@gitlab.com>
Reviewed-by: karthik nayak <knayak@gitlab.com>

3 files changed, 48 insertions, 125 deletions

diff --git a/internal/gitaly/service/ref/delete_refs.go b/internal/gitaly/service/ref/delete_refs.go
index c1dd1613a..b43bc0b11 100644
--- a/internal/gitaly/service/ref/delete_refs.go
+++ b/internal/gitaly/service/ref/delete_refs.go
@@ -11,7 +11,6 @@ import (
 	"gitlab.com/gitlab-org/gitaly/v15/internal/git/updateref"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/gitaly/service"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/gitaly/transaction"
-	"gitlab.com/gitlab-org/gitaly/v15/internal/metadata/featureflag"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/structerr"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/transaction/voting"
 	"gitlab.com/gitlab-org/gitaly/v15/proto/go/gitalypb"
@@ -38,38 +37,30 @@ func (s *server) DeleteRefs(ctx context.Context, in *gitalypb.DeleteRefsRequest)
 	}
 	defer func() {
 		if err := updater.Close(); err != nil && returnedErr == nil {
-			// Only return the error if the feature flag is active. Traditionally
-			// the error was packed into the response body so this would start
-			// returning a real error. With structured errors, an actual error
-			// is returned so this would not override it.
-			if featureflag.DeleteRefsStructuredErrors.IsEnabled(ctx) {
-				returnedErr = fmt.Errorf("close updater: %w", err)
-			}
+			returnedErr = fmt.Errorf("close updater: %w", err)
 		}
 	}()
 
 	voteHash := voting.NewVoteHash()
 
-	if featureflag.DeleteRefsStructuredErrors.IsEnabled(ctx) {
-		var invalidRefnames [][]byte
+	var invalidRefnames [][]byte
 
-		for _, ref := range refnames {
-			if err := git.ValidateRevision([]byte(ref)); err != nil {
-				invalidRefnames = append(invalidRefnames, []byte(ref))
-			}
+	for _, ref := range refnames {
+		if err := git.ValidateRevision([]byte(ref)); err != nil {
+			invalidRefnames = append(invalidRefnames, []byte(ref))
 		}
+	}
 
-		if len(invalidRefnames) > 0 {
-			return nil, structerr.NewInvalidArgument("invalid references").WithDetail(
-				&gitalypb.DeleteRefsError{
-					Error: &gitalypb.DeleteRefsError_InvalidFormat{
-						InvalidFormat: &gitalypb.InvalidRefFormatError{
-							Refs: invalidRefnames,
-						},
+	if len(invalidRefnames) > 0 {
+		return nil, structerr.NewInvalidArgument("invalid references").WithDetail(
+			&gitalypb.DeleteRefsError{
+				Error: &gitalypb.DeleteRefsError_InvalidFormat{
+					InvalidFormat: &gitalypb.InvalidRefFormatError{
+						Refs: invalidRefnames,
 					},
 				},
-			)
-		}
+			},
+		)
 	}
 
 	if err := updater.Start(); err != nil {
@@ -78,11 +69,7 @@ func (s *server) DeleteRefs(ctx context.Context, in *gitalypb.DeleteRefsRequest)
 
 	for _, ref := range refnames {
 		if err := updater.Delete(ref); err != nil {
-			if featureflag.DeleteRefsStructuredErrors.IsEnabled(ctx) {
-				return nil, structerr.NewInternal("unable to delete refs: %w", err)
-			}
-
-			return &gitalypb.DeleteRefsResponse{GitError: err.Error()}, nil
+			return nil, structerr.NewInternal("unable to delete refs: %w", err)
 		}
 
 		if _, err := voteHash.Write([]byte(ref.String() + "\n")); err != nil {
@@ -91,26 +78,20 @@ func (s *server) DeleteRefs(ctx context.Context, in *gitalypb.DeleteRefsRequest)
 	}
 
 	if err := updater.Prepare(); err != nil {
-		if featureflag.DeleteRefsStructuredErrors.IsEnabled(ctx) {
-			var alreadyLockedErr *updateref.AlreadyLockedError
-			if errors.As(err, &alreadyLockedErr) {
-				return nil, structerr.NewFailedPrecondition("cannot lock references").WithDetail(
-					&gitalypb.DeleteRefsError{
-						Error: &gitalypb.DeleteRefsError_ReferencesLocked{
-							ReferencesLocked: &gitalypb.ReferencesLockedError{
-								Refs: [][]byte{[]byte(alreadyLockedErr.Ref)},
-							},
+		var alreadyLockedErr *updateref.AlreadyLockedError
+		if errors.As(err, &alreadyLockedErr) {
+			return nil, structerr.NewFailedPrecondition("cannot lock references").WithDetail(
+				&gitalypb.DeleteRefsError{
+					Error: &gitalypb.DeleteRefsError_ReferencesLocked{
+						ReferencesLocked: &gitalypb.ReferencesLockedError{
+							Refs: [][]byte{[]byte(alreadyLockedErr.Ref)},
 						},
 					},
-				)
-			}
-
-			return nil, structerr.NewInternal("unable to prepare: %w", err)
+				},
+			)
 		}
 
-		return &gitalypb.DeleteRefsResponse{
-			GitError: fmt.Sprintf("unable to delete refs: %s", err.Error()),
-		}, nil
+		return nil, structerr.NewInternal("unable to prepare: %w", err)
 	}
 
 	vote, err := voteHash.Vote()
@@ -127,11 +108,7 @@ func (s *server) DeleteRefs(ctx context.Context, in *gitalypb.DeleteRefsRequest)
 	}
 
 	if err := updater.Commit(); err != nil {
-		if featureflag.DeleteRefsStructuredErrors.IsEnabled(ctx) {
-			return nil, structerr.NewInternal("unable to commit: %w", err)
-		}
-
-		return &gitalypb.DeleteRefsResponse{GitError: fmt.Sprintf("unable to delete refs: %s", err.Error())}, nil
+		return nil, structerr.NewInternal("unable to commit: %w", err)
 	}
 
 	if err := transaction.VoteOnContext(ctx, s.txManager, vote, voting.Committed); err != nil {
diff --git a/internal/gitaly/service/ref/delete_refs_test.go b/internal/gitaly/service/ref/delete_refs_test.go
index 4fc8a53d8..9946beea2 100644
--- a/internal/gitaly/service/ref/delete_refs_test.go
+++ b/internal/gitaly/service/ref/delete_refs_test.go
@@ -3,10 +3,8 @@
 package ref
 
 import (
-	"context"
 	"testing"
 
-	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/git"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/git/gittest"
@@ -17,7 +15,6 @@ import (
 	"gitlab.com/gitlab-org/gitaly/v15/internal/gitaly/service/repository"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/gitaly/transaction"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/metadata"
-	"gitlab.com/gitlab-org/gitaly/v15/internal/metadata/featureflag"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/structerr"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/testhelper"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/testhelper/testcfg"
@@ -32,15 +29,7 @@ import (
 func TestDeleteRefs_successful(t *testing.T) {
 	t.Parallel()
 
-	testhelper.NewFeatureSets(featureflag.DeleteRefsStructuredErrors).Run(
-		t,
-		testDeleteRefSuccessful,
-	)
-}
-
-func testDeleteRefSuccessful(t *testing.T, ctx context.Context) {
-	t.Parallel()
-
+	ctx := testhelper.Context(t)
 	cfg, client := setupRefServiceWithoutRepo(t)
 
 	testCases := []struct {
@@ -106,15 +95,7 @@ func testDeleteRefSuccessful(t *testing.T, ctx context.Context) {
 func TestDeleteRefs_transaction(t *testing.T) {
 	t.Parallel()
 
-	testhelper.NewFeatureSets(featureflag.DeleteRefsStructuredErrors).Run(
-		t,
-		testDeleteRefsTransaction,
-	)
-}
-
-func testDeleteRefsTransaction(t *testing.T, ctx context.Context) {
-	t.Parallel()
-
+	ctx := testhelper.Context(t)
 	cfg := testcfg.Build(t)
 
 	testcfg.BuildGitalyHooks(t, cfg)
@@ -189,15 +170,7 @@ func testDeleteRefsTransaction(t *testing.T, ctx context.Context) {
 func TestDeleteRefs_invalidRefFormat(t *testing.T) {
 	t.Parallel()
 
-	testhelper.NewFeatureSets(featureflag.DeleteRefsStructuredErrors).Run(
-		t,
-		testDeleteRefsInvalidRefFormat,
-	)
-}
-
-func testDeleteRefsInvalidRefFormat(t *testing.T, ctx context.Context) {
-	t.Parallel()
-
+	ctx := testhelper.Context(t)
 	_, repo, _, client := setupRefService(t, ctx)
 
 	request := &gitalypb.DeleteRefsRequest{
@@ -207,36 +180,23 @@ func testDeleteRefsInvalidRefFormat(t *testing.T, ctx context.Context) {
 
 	response, err := client.DeleteRefs(ctx, request)
 
-	if featureflag.DeleteRefsStructuredErrors.IsEnabled(ctx) {
-		require.Nil(t, response)
-		detailedErr := structerr.NewInvalidArgument("invalid references").WithDetail(
-			&gitalypb.DeleteRefsError{
-				Error: &gitalypb.DeleteRefsError_InvalidFormat{
-					InvalidFormat: &gitalypb.InvalidRefFormatError{
-						Refs: request.Refs,
-					},
+	require.Nil(t, response)
+	detailedErr := structerr.NewInvalidArgument("invalid references").WithDetail(
+		&gitalypb.DeleteRefsError{
+			Error: &gitalypb.DeleteRefsError_InvalidFormat{
+				InvalidFormat: &gitalypb.InvalidRefFormatError{
+					Refs: request.Refs,
 				},
 			},
-		)
-		testhelper.RequireGrpcError(t, detailedErr, err)
-	} else {
-		require.NoError(t, err)
-		require.Equal(t, response.GitError, `unable to delete refs: invalid reference format: "refs invalid-ref-format"`)
-	}
-}
-
-func TestDeleteRefs_refLocked(t *testing.T) {
-	t.Parallel()
-
-	testhelper.NewFeatureSets(featureflag.DeleteRefsStructuredErrors).Run(
-		t,
-		testDeleteRefsRefLocked,
+		},
 	)
+	testhelper.RequireGrpcError(t, detailedErr, err)
 }
 
-func testDeleteRefsRefLocked(t *testing.T, ctx context.Context) {
+func TestDeleteRefs_refLocked(t *testing.T) {
 	t.Parallel()
 
+	ctx := testhelper.Context(t)
 	cfg, repoProto, _, client := setupRefService(t, ctx)
 
 	request := &gitalypb.DeleteRefsRequest{
@@ -262,22 +222,17 @@ func testDeleteRefsRefLocked(t *testing.T, ctx context.Context) {
 
 	response, err := client.DeleteRefs(ctx, request)
 
-	if featureflag.DeleteRefsStructuredErrors.IsEnabled(ctx) {
-		require.Nil(t, response)
-		detailedErr := structerr.NewFailedPrecondition("cannot lock references").WithDetail(
-			&gitalypb.DeleteRefsError{
-				Error: &gitalypb.DeleteRefsError_ReferencesLocked{
-					ReferencesLocked: &gitalypb.ReferencesLockedError{
-						Refs: [][]byte{[]byte("refs/heads/master")},
-					},
+	require.Nil(t, response)
+	detailedErr := structerr.NewFailedPrecondition("cannot lock references").WithDetail(
+		&gitalypb.DeleteRefsError{
+			Error: &gitalypb.DeleteRefsError_ReferencesLocked{
+				ReferencesLocked: &gitalypb.ReferencesLockedError{
+					Refs: [][]byte{[]byte("refs/heads/master")},
 				},
 			},
-		)
-		testhelper.RequireGrpcError(t, detailedErr, err)
-	} else {
-		require.NoError(t, err)
-		assert.Contains(t, response.GetGitError(), "reference is already locked")
-	}
+		},
+	)
+	testhelper.RequireGrpcError(t, detailedErr, err)
 }
 
 func TestDeleteRefs_validation(t *testing.T) {
diff --git a/internal/metadata/featureflag/ff_delete_refs_structured_errors.go b/internal/metadata/featureflag/ff_delete_refs_structured_errors.go
deleted file mode 100644
index eca4647c7..000000000
--- a/internal/metadata/featureflag/ff_delete_refs_structured_errors.go
+++ /dev/null
@@ -1,9 +0,0 @@
-package featureflag
-
-// DeleteRefsStructuredErrors turns on metrics for pack objects
-var DeleteRefsStructuredErrors = NewFeatureFlag(
-	"delete_refs_structured_errors",
-	"v15.3.0",
-	"https://gitlab.com/gitlab-org/gitaly/-/issues/4348",
-	false,
-)