diff options
author | Toon Claes <toon@gitlab.com> | 2022-09-23 15:29:41 +0300 |
---|---|---|
committer | Toon Claes <toon@gitlab.com> | 2022-09-23 15:29:41 +0300 |
commit | d7181e813e602f80bf53e47089da92b6342b355f (patch) | |
tree | 05a06b1a20a680fa373ab386acc1fade68ab6b32 | |
parent | 95eeb6416fb68077f9de4c20ad224da07aeb487d (diff) | |
parent | 248f5d5677a11a3bb07643580c4614e4645bc31f (diff) |
Merge branch '4455-feature-flag-roll-out-user_create_branch_structured_errors' into 'master'
operations: Structured errors in UserCreateBranch
Closes #4455
See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/4883
Merged-by: Toon Claes <toon@gitlab.com>
Approved-by: Toon Claes <toon@gitlab.com>
Co-authored-by: Karthik Nayak <knayak@gitlab.com>
3 files changed, 46 insertions, 100 deletions
diff --git a/internal/gitaly/service/operations/branches.go b/internal/gitaly/service/operations/branches.go index adaa0442d..83fc9a438 100644 --- a/internal/gitaly/service/operations/branches.go +++ b/internal/gitaly/service/operations/branches.go @@ -8,7 +8,6 @@ import ( "gitlab.com/gitlab-org/gitaly/v15/internal/git/updateref" "gitlab.com/gitlab-org/gitaly/v15/internal/gitaly/hook" "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" @@ -62,30 +61,24 @@ func (s *Server) UserCreateBranch(ctx context.Context, req *gitalypb.UserCreateB var customHookErr updateref.CustomHookError if errors.As(err, &customHookErr) { - if featureflag.UserCreateBranchStructuredErrors.IsEnabled(ctx) { - detailedErr, err := helper.ErrWithDetails( - // We explicitly don't include the custom hook error itself - // in the returned error because that would also contain the - // standard output or standard error in the error message. - // It's thus needlessly verbose and duplicates information - // we have available in the structured error anyway. - helper.ErrPermissionDeniedf("creation denied by custom hooks"), - &gitalypb.UserCreateBranchError{ - Error: &gitalypb.UserCreateBranchError_CustomHook{ - CustomHook: customHookErr.Proto(), - }, + detailedErr, err := helper.ErrWithDetails( + // We explicitly don't include the custom hook error itself + // in the returned error because that would also contain the + // standard output or standard error in the error message. + // It's thus needlessly verbose and duplicates information + // we have available in the structured error anyway. + helper.ErrPermissionDeniedf("creation denied by custom hooks"), + &gitalypb.UserCreateBranchError{ + Error: &gitalypb.UserCreateBranchError_CustomHook{ + CustomHook: customHookErr.Proto(), }, - ) - if err != nil { - return nil, helper.ErrInternalf("error details: %w", err) - } - - return nil, detailedErr - } else { - return &gitalypb.UserCreateBranchResponse{ - PreReceiveError: customHookErr.Error(), - }, nil + }, + ) + if err != nil { + return nil, helper.ErrInternalf("error details: %w", err) } + + return nil, detailedErr } var updateRefError updateref.Error diff --git a/internal/gitaly/service/operations/branches_test.go b/internal/gitaly/service/operations/branches_test.go index bea777110..ab2827e06 100644 --- a/internal/gitaly/service/operations/branches_test.go +++ b/internal/gitaly/service/operations/branches_test.go @@ -18,7 +18,6 @@ import ( "gitlab.com/gitlab-org/gitaly/v15/internal/gitlab" "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/testcfg" "gitlab.com/gitlab-org/gitaly/v15/internal/testhelper/testserver" @@ -43,12 +42,8 @@ func (s *testTransactionServer) VoteTransaction(ctx context.Context, in *gitalyp func TestUserCreateBranch_successful(t *testing.T) { t.Parallel() - testhelper.NewFeatureSets(featureflag.UserCreateBranchStructuredErrors).Run(t, testUserCreateBranchSuccessful) -} - -func testUserCreateBranchSuccessful(t *testing.T, ctx context.Context) { - t.Parallel() + ctx := testhelper.Context(t) ctx, cfg, repoProto, repoPath, client := setupOperationsService(t, ctx) repo := localrepo.NewTestRepo(t, cfg, repoProto) @@ -124,12 +119,8 @@ func testUserCreateBranchSuccessful(t *testing.T, ctx context.Context) { func TestUserCreateBranch_Transactions(t *testing.T) { t.Parallel() - testhelper.NewFeatureSets(featureflag.UserCreateBranchStructuredErrors).Run(t, testUserCreateBranchTransactions) -} - -func testUserCreateBranchTransactions(t *testing.T, ctx context.Context) { - t.Parallel() + ctx := testhelper.Context(t) cfg, repo, repoPath := testcfg.BuildWithRepo(t) transactionServer := &testTransactionServer{} @@ -202,12 +193,8 @@ func testUserCreateBranchTransactions(t *testing.T, ctx context.Context) { func TestUserCreateBranch_hook(t *testing.T) { t.Parallel() - testhelper.NewFeatureSets(featureflag.UserCreateBranchStructuredErrors).Run(t, testUserCreateBranchHook) -} - -func testUserCreateBranchHook(t *testing.T, ctx context.Context) { - t.Parallel() + ctx := testhelper.Context(t) ctx, cfg, repo, repoPath, client := setupOperationsService(t, ctx) branchName := "new-branch" @@ -236,12 +223,8 @@ func testUserCreateBranchHook(t *testing.T, ctx context.Context) { func TestUserCreateBranch_startPoint(t *testing.T) { t.Parallel() - testhelper.NewFeatureSets(featureflag.UserCreateBranchStructuredErrors).Run(t, testUserCreateBranchStartPoint) -} - -func testUserCreateBranchStartPoint(t *testing.T, ctx context.Context) { - t.Parallel() + ctx := testhelper.Context(t) ctx, cfg, repoProto, repoPath, client := setupOperationsService(t, ctx) repo := localrepo.NewTestRepo(t, cfg, repoProto) @@ -313,12 +296,8 @@ func testUserCreateBranchStartPoint(t *testing.T, ctx context.Context) { func TestUserCreateBranch_hookFailure(t *testing.T) { t.Parallel() - testhelper.NewFeatureSets(featureflag.UserCreateBranchStructuredErrors).Run(t, testUserCreateBranchHookFailure) -} - -func testUserCreateBranchHookFailure(t *testing.T, ctx context.Context) { - t.Parallel() + ctx := testhelper.Context(t) ctx, _, repo, repoPath, client := setupOperationsService(t, ctx) request := &gitalypb.UserCreateBranchRequest{ @@ -335,35 +314,27 @@ func testUserCreateBranchHookFailure(t *testing.T, ctx context.Context) { for _, hookName := range gitlabPreHooks { gittest.WriteCustomHook(t, repoPath, hookName, hookContent) - response, err := client.UserCreateBranch(ctx, request) + _, err := client.UserCreateBranch(ctx, request) - if featureflag.UserCreateBranchStructuredErrors.IsEnabled(ctx) { - testhelper.RequireGrpcError(t, errWithDetails(t, - helper.ErrPermissionDeniedf("creation denied by custom hooks"), - &gitalypb.UserCreateBranchError{ - Error: &gitalypb.UserCreateBranchError_CustomHook{ - CustomHook: &gitalypb.CustomHookError{ - HookType: gitalypb.CustomHookError_HOOK_TYPE_PRERECEIVE, - Stdout: []byte(expectedObject + "\n"), - }, + testhelper.RequireGrpcError(t, errWithDetails(t, + helper.ErrPermissionDeniedf("creation denied by custom hooks"), + &gitalypb.UserCreateBranchError{ + Error: &gitalypb.UserCreateBranchError_CustomHook{ + CustomHook: &gitalypb.CustomHookError{ + HookType: gitalypb.CustomHookError_HOOK_TYPE_PRERECEIVE, + Stdout: []byte(expectedObject + "\n"), }, }, - ), err) - } else { - require.Nil(t, err) - require.Contains(t, response.PreReceiveError, expectedObject) - } + }, + ), err) + } } func TestUserCreateBranch_Failure(t *testing.T) { t.Parallel() - testhelper.NewFeatureSets(featureflag.UserCreateBranchStructuredErrors).Run(t, testUserCreateBranchFailure) -} - -func testUserCreateBranchFailure(t *testing.T, ctx context.Context) { - t.Parallel() + ctx := testhelper.Context(t) ctx, _, repo, _, client := setupOperationsService(t, ctx) testCases := []struct { @@ -788,12 +759,8 @@ func TestUserDeleteBranch_hookFailure(t *testing.T) { func TestBranchHookOutput(t *testing.T) { t.Parallel() - testhelper.NewFeatureSets(featureflag.UserCreateBranchStructuredErrors).Run(t, testBranchHookOutput) -} - -func testBranchHookOutput(t *testing.T, ctx context.Context) { - t.Parallel() + ctx := testhelper.Context(t) ctx, cfg, repo, repoPath, client := setupOperationsService(t, ctx) testCases := []struct { @@ -876,25 +843,20 @@ func testBranchHookOutput(t *testing.T, ctx context.Context) { hookFilename := gittest.WriteCustomHook(t, repoPath, hookTestCase.hookName, []byte(testCase.hookContent)) expectedError := testCase.output(hookFilename) - createResponse, err := client.UserCreateBranch(ctx, createRequest) - - if featureflag.UserCreateBranchStructuredErrors.IsEnabled(ctx) { - testhelper.RequireGrpcError(t, errWithDetails(t, - helper.ErrPermissionDeniedf("creation denied by custom hooks"), - &gitalypb.UserCreateBranchError{ - Error: &gitalypb.UserCreateBranchError_CustomHook{ - CustomHook: &gitalypb.CustomHookError{ - HookType: hookTestCase.hookType, - Stdout: []byte(testCase.expectedStdout), - Stderr: []byte(testCase.expectedStderr), - }, + _, err := client.UserCreateBranch(ctx, createRequest) + + testhelper.RequireGrpcError(t, errWithDetails(t, + helper.ErrPermissionDeniedf("creation denied by custom hooks"), + &gitalypb.UserCreateBranchError{ + Error: &gitalypb.UserCreateBranchError_CustomHook{ + CustomHook: &gitalypb.CustomHookError{ + HookType: hookTestCase.hookType, + Stdout: []byte(testCase.expectedStdout), + Stderr: []byte(testCase.expectedStderr), }, }, - ), err) - } else { - require.NoError(t, err) - require.Equal(t, expectedError, createResponse.PreReceiveError) - } + }, + ), err) gittest.Exec(t, cfg, "-C", repoPath, "branch", branchNameInput) defer gittest.Exec(t, cfg, "-C", repoPath, "branch", "-d", branchNameInput) diff --git a/internal/metadata/featureflag/ff_user_create_branch_structured_errors.go b/internal/metadata/featureflag/ff_user_create_branch_structured_errors.go deleted file mode 100644 index f1b6e965a..000000000 --- a/internal/metadata/featureflag/ff_user_create_branch_structured_errors.go +++ /dev/null @@ -1,9 +0,0 @@ -package featureflag - -// UserCreateBranchStructuredErrors will enable the use of structured errors in the UserCreateTag RPC. -var UserCreateBranchStructuredErrors = NewFeatureFlag( - "user_create_branch_structured_errors", - "v15.4.0", - "https://gitlab.com/gitlab-org/gitaly/-/issues/4227", - false, -) |