diff options
author | John Cai <jcai@gitlab.com> | 2023-05-06 01:40:21 +0300 |
---|---|---|
committer | John Cai <jcai@gitlab.com> | 2023-05-06 01:40:21 +0300 |
commit | 91b69d050acf344c09a9238f24a75c4938001113 (patch) | |
tree | 86aa65c05d15a943b7783a676580af22ae411c5f | |
parent | 679da328e78d5167b786459b372b25ad2aec6682 (diff) | |
parent | 11155290780444ff1f10d168c5cf33330cb09aa8 (diff) |
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>
-rw-r--r-- | internal/gitaly/service/ref/delete_refs.go | 75 | ||||
-rw-r--r-- | internal/gitaly/service/ref/delete_refs_test.go | 89 | ||||
-rw-r--r-- | internal/metadata/featureflag/ff_delete_refs_structured_errors.go | 9 |
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, -) |