diff options
author | Ævar Arnfjörð Bjarmason <avarab@gmail.com> | 2021-01-15 14:31:19 +0300 |
---|---|---|
committer | Ævar Arnfjörð Bjarmason <avarab@gmail.com> | 2021-01-19 18:23:35 +0300 |
commit | fbc9f83ab7a03f167c8c82eb2d7beb09215df364 (patch) | |
tree | c34e4668da151f8a758b2d7eef2f367fe889920d | |
parent | 6a06feda7fd01961bb332afce4d7f7b4ce4a5aad (diff) |
Remove Ruby code for 100% on user_delete_{branch,tag} in Go feature
A follow-up to a96949aef (Enable feature flag
go_user_delete_{branch,tag} by default, 2021-01-12). A relevant
message I posted in Slack[1] about it in `#g_create_gitaly`:
For removing the Ruby side of a full-on Ruby->Go feature, I've got
some questions about how/what to remove:
1. This is user_delete_{tag,branch}, removing the code in
ruby/lib/gitaly_server/operations_service.rb is obvious.
2. But that code calls rm_{tag,branch} in
ruby/lib/gitlab/git/operation_service.rb, that seems (but not sure)
to be the library gitlab.git will use to call us, but is that also
legacy code at this point? Can't find where it's still used.
3. There's also e.g. the UserDeleteTagResponse left, which is the
proto response. Is there something to configure to make the Ruby
code in ruby/proto/gitaly/operations_pb.rb not be generated on make
proto? Or is that imported into gitlab.git ...
4. Anything else? I just know about the caveat of removing this in 2x
releases, but the change to make it on by default just went out, so
OK to remove the Ruby code after that AFAIK.
So perhaps we need to remove more things here, but this is plenty for
now.
1. https://gitlab.slack.com/archives/C3ER3TQBT/p1610710253044800
-rw-r--r-- | internal/gitaly/service/operations/branches.go | 19 | ||||
-rw-r--r-- | internal/gitaly/service/operations/branches_test.go | 18 | ||||
-rw-r--r-- | internal/gitaly/service/operations/tags.go | 21 | ||||
-rw-r--r-- | internal/gitaly/service/operations/tags_test.go | 13 | ||||
-rw-r--r-- | internal/metadata/featureflag/feature_flags.go | 6 | ||||
-rw-r--r-- | ruby/lib/gitaly_server/operations_service.rb | 32 |
6 files changed, 9 insertions, 100 deletions
diff --git a/internal/gitaly/service/operations/branches.go b/internal/gitaly/service/operations/branches.go index 0ab66c533..a3552300a 100644 --- a/internal/gitaly/service/operations/branches.go +++ b/internal/gitaly/service/operations/branches.go @@ -118,11 +118,6 @@ func (s *Server) UserDeleteBranch(ctx context.Context, req *gitalypb.UserDeleteB return nil, status.Errorf(codes.InvalidArgument, "Bad Request (empty user)") } - // Implement UserDeleteBranch in Go - if featureflag.IsDisabled(ctx, featureflag.GoUserDeleteBranch) { - return s.userDeleteBranchRuby(ctx, req) - } - referenceFmt := "refs/heads/%s" if strings.HasPrefix(string(req.BranchName), "refs/") { // Not the same behavior as UserCreateBranch. This is @@ -154,17 +149,3 @@ func (s *Server) UserDeleteBranch(ctx context.Context, req *gitalypb.UserDeleteB return &gitalypb.UserDeleteBranchResponse{}, nil } - -func (s *Server) userDeleteBranchRuby(ctx context.Context, req *gitalypb.UserDeleteBranchRequest) (*gitalypb.UserDeleteBranchResponse, error) { - client, err := s.ruby.OperationServiceClient(ctx) - if err != nil { - return nil, err - } - - clientCtx, err := rubyserver.SetHeaders(ctx, s.locator, req.GetRepository()) - if err != nil { - return nil, err - } - - return client.UserDeleteBranch(clientCtx, req) -} diff --git a/internal/gitaly/service/operations/branches_test.go b/internal/gitaly/service/operations/branches_test.go index 5cc1189c4..03b9da6ec 100644 --- a/internal/gitaly/service/operations/branches_test.go +++ b/internal/gitaly/service/operations/branches_test.go @@ -442,7 +442,6 @@ func testFailedUserCreateBranchRequest(t *testing.T, ctx context.Context) { func TestSuccessfulUserDeleteBranchRequest(t *testing.T) { testhelper.NewFeatureSets([]featureflag.FeatureFlag{ featureflag.ReferenceTransactions, - featureflag.GoUserDeleteBranch, }).Run(t, testSuccessfulUserDeleteBranchRequest) } @@ -542,10 +541,9 @@ func TestSuccessfulGitHooksForUserDeleteBranchRequest(t *testing.T) { } func TestFailedUserDeleteBranchDueToValidation(t *testing.T) { - testWithFeature(t, featureflag.GoUserDeleteBranch, testFailedUserDeleteBranchDueToValidation) -} + ctx, cancel := testhelper.Context() + defer cancel() -func testFailedUserDeleteBranchDueToValidation(t *testing.T, ctx context.Context) { serverSocketPath, stop := runOperationServiceServer(t) defer stop() @@ -600,11 +598,10 @@ func testFailedUserDeleteBranchDueToValidation(t *testing.T, ctx context.Context } } -func TestUserDeleteBranch_failedDueToRefsHeadsPrefix(t *testing.T) { - testWithFeature(t, featureflag.GoUserDeleteBranch, testUserDeleteBranchFailedDueToRefsHeadsPrefix) -} +func TestUserDeleteBranchFailedDueToRefsHeadsPrefix(t *testing.T) { + ctx, cancel := testhelper.Context() + defer cancel() -func testUserDeleteBranchFailedDueToRefsHeadsPrefix(t *testing.T, ctx context.Context) { testRepo, testRepoPath, cleanupFn := testhelper.NewTestRepo(t) defer cleanupFn() @@ -656,10 +653,9 @@ func testUserDeleteBranchFailedDueToRefsHeadsPrefix(t *testing.T, ctx context.Co } func TestFailedUserDeleteBranchDueToHooks(t *testing.T) { - testWithFeature(t, featureflag.GoUserDeleteBranch, testFailedUserDeleteBranchDueToHooks) -} + ctx, cancel := testhelper.Context() + defer cancel() -func testFailedUserDeleteBranchDueToHooks(t *testing.T, ctx context.Context) { testRepo, testRepoPath, cleanupFn := testhelper.NewTestRepo(t) defer cleanupFn() diff --git a/internal/gitaly/service/operations/tags.go b/internal/gitaly/service/operations/tags.go index de5233520..d383b475e 100644 --- a/internal/gitaly/service/operations/tags.go +++ b/internal/gitaly/service/operations/tags.go @@ -18,27 +18,6 @@ import ( ) func (s *Server) UserDeleteTag(ctx context.Context, req *gitalypb.UserDeleteTagRequest) (*gitalypb.UserDeleteTagResponse, error) { - if featureflag.IsDisabled(ctx, featureflag.GoUserDeleteTag) { - return s.UserDeleteTagRuby(ctx, req) - } - return s.UserDeleteTagGo(ctx, req) -} - -func (s *Server) UserDeleteTagRuby(ctx context.Context, req *gitalypb.UserDeleteTagRequest) (*gitalypb.UserDeleteTagResponse, error) { - client, err := s.ruby.OperationServiceClient(ctx) - if err != nil { - return nil, err - } - - clientCtx, err := rubyserver.SetHeaders(ctx, s.locator, req.GetRepository()) - if err != nil { - return nil, err - } - - return client.UserDeleteTag(clientCtx, req) -} - -func (s *Server) UserDeleteTagGo(ctx context.Context, req *gitalypb.UserDeleteTagRequest) (*gitalypb.UserDeleteTagResponse, error) { if len(req.TagName) == 0 { return nil, status.Errorf(codes.InvalidArgument, "empty tag name") } diff --git a/internal/gitaly/service/operations/tags_test.go b/internal/gitaly/service/operations/tags_test.go index 04d5560f8..d7133e726 100644 --- a/internal/gitaly/service/operations/tags_test.go +++ b/internal/gitaly/service/operations/tags_test.go @@ -23,7 +23,6 @@ import ( func TestSuccessfulUserDeleteTagRequest(t *testing.T) { testhelper.NewFeatureSets([]featureflag.FeatureFlag{ featureflag.ReferenceTransactions, - featureflag.GoUserDeleteTag, }).Run(t, testSuccessfulUserDeleteTagRequest) } @@ -59,7 +58,6 @@ func testSuccessfulUserDeleteTagRequest(t *testing.T, ctx context.Context) { func TestSuccessfulGitHooksForUserDeleteTagRequest(t *testing.T) { testhelper.NewFeatureSets([]featureflag.FeatureFlag{ featureflag.ReferenceTransactions, - featureflag.GoUserDeleteTag, }).Run(t, testSuccessfulGitHooksForUserDeleteTagRequest) } @@ -733,7 +731,6 @@ func testSuccessfulUserCreateTagNestedTags(t *testing.T, ctx context.Context) { func TestUserDeleteTagsuccessfulDeletionOfPrefixedTag(t *testing.T) { testhelper.NewFeatureSets([]featureflag.FeatureFlag{ featureflag.ReferenceTransactions, - featureflag.GoUserDeleteTag, }).Run(t, testUserDeleteTagsuccessfulDeletionOfPrefixedTag) } @@ -895,13 +892,9 @@ func testSuccessfulGitHooksForUserCreateTagRequest(t *testing.T, ctx context.Con } func TestFailedUserDeleteTagRequestDueToValidation(t *testing.T) { - testhelper.NewFeatureSets([]featureflag.FeatureFlag{ - featureflag.GoUserDeleteTag, - featureflag.ReferenceTransactions, - }).Run(t, testFailedUserDeleteTagRequestDueToValidation) -} + ctx, cancel := testhelper.Context() + defer cancel() -func testFailedUserDeleteTagRequestDueToValidation(t *testing.T, ctx context.Context) { serverSocketPath, stop := runOperationServiceServer(t) defer stop() @@ -978,7 +971,6 @@ func testFailedUserDeleteTagRequestDueToValidation(t *testing.T, ctx context.Con func TestFailedUserDeleteTagDueToHooks(t *testing.T) { testhelper.NewFeatureSets([]featureflag.FeatureFlag{ - featureflag.GoUserDeleteTag, featureflag.ReferenceTransactions, }).Run(t, testFailedUserDeleteTagDueToHooks) } @@ -1240,7 +1232,6 @@ func testFailedUserCreateTagRequestDueToValidation(t *testing.T, ctx context.Con func TestTagHookOutput(t *testing.T) { testhelper.NewFeatureSets([]featureflag.FeatureFlag{ - featureflag.GoUserDeleteTag, featureflag.ReferenceTransactions, featureflag.GoUserCreateTag, }).Run(t, testTagHookOutput) diff --git a/internal/metadata/featureflag/feature_flags.go b/internal/metadata/featureflag/feature_flags.go index 20dd2a695..8777a7d38 100644 --- a/internal/metadata/featureflag/feature_flags.go +++ b/internal/metadata/featureflag/feature_flags.go @@ -23,8 +23,6 @@ var ( GoUserFFBranch = FeatureFlag{Name: "go_user_ff_branch", OnByDefault: false} // GoUserCreateBranch enables the Go implementation of UserCreateBranch GoUserCreateBranch = FeatureFlag{Name: "go_user_create_branch", OnByDefault: false} - // GoUserDeleteBranch enables the Go implementation of UserDeleteBranch - GoUserDeleteBranch = FeatureFlag{Name: "go_user_delete_branch", OnByDefault: true} // GoUserCommitFiles enables the Go implementation of UserCommitFiles GoUserCommitFiles = FeatureFlag{Name: "go_user_commit_files", OnByDefault: false} // GoResolveConflicts enables the Go implementation of ResolveConflicts @@ -32,8 +30,6 @@ var ( // GoUserUpdateSubmodule enables the Go implementation of // UserUpdateSubmodules GoUserUpdateSubmodule = FeatureFlag{Name: "go_user_update_submodule", OnByDefault: false} - // GoUserDeleteTag enables the Go implementation of UserDeleteTag - GoUserDeleteTag = FeatureFlag{Name: "go_user_delete_tag", OnByDefault: true} // GoUserCreateTag enables the Go implementation of UserCreateTag GoUserCreateTag = FeatureFlag{Name: "go_user_create_tag", OnByDefault: false} // GoUserRevert enables the Go implementation of UserRevert @@ -112,11 +108,9 @@ var All = []FeatureFlag{ GoUserMergeBranch, GoUserFFBranch, GoUserCreateBranch, - GoUserDeleteBranch, GoUserCommitFiles, GoResolveConflicts, GoUserUpdateSubmodule, - GoUserDeleteTag, GoUserCreateTag, GoUserRevert, TxApplyBfgObjectMapStream, diff --git a/ruby/lib/gitaly_server/operations_service.rb b/ruby/lib/gitaly_server/operations_service.rb index 74286ffd5..c02dcaf05 100644 --- a/ruby/lib/gitaly_server/operations_service.rb +++ b/ruby/lib/gitaly_server/operations_service.rb @@ -42,24 +42,6 @@ module GitalyServer Gitaly::UserCreateTagResponse.new(pre_receive_error: set_utf8!(e.message)) end - def user_delete_tag(request, call) - repo = Gitlab::Git::Repository.from_gitaly(request.repository, call) - transaction = Praefect::Transaction.from_metadata(call.metadata) - - gitaly_user = get_param!(request, :user) - user = Gitlab::Git::User.from_gitaly(gitaly_user) - - tag_name = get_param!(request, :tag_name) - - repo.rm_tag(tag_name, user: user, transaction: transaction) - - Gitaly::UserDeleteTagResponse.new - rescue Gitlab::Git::PreReceiveError => e - Gitaly::UserDeleteTagResponse.new(pre_receive_error: set_utf8!(e.message)) - rescue Gitlab::Git::Repository::InvalidRef, Gitlab::Git::CommitError => e - raise GRPC::FailedPrecondition.new(e.message) - end - def user_create_branch(request, call) repo = Gitlab::Git::Repository.from_gitaly(request.repository, call) target = get_param!(request, :start_point) @@ -99,20 +81,6 @@ module GitalyServer Gitaly::UserUpdateBranchResponse.new(pre_receive_error: set_utf8!(ex.message)) end - def user_delete_branch(request, call) - repo = Gitlab::Git::Repository.from_gitaly(request.repository, call) - user = Gitlab::Git::User.from_gitaly(request.user) - transaction = Praefect::Transaction.from_metadata(call.metadata) - - repo.rm_branch(request.branch_name, user: user, transaction: transaction) - - Gitaly::UserDeleteBranchResponse.new - rescue Gitlab::Git::PreReceiveError => e - Gitaly::UserDeleteBranchResponse.new(pre_receive_error: set_utf8!(e.message)) - rescue Gitlab::Git::Repository::InvalidRef, Gitlab::Git::CommitError => e - raise GRPC::FailedPrecondition.new(e.message) - end - def user_merge_to_ref(request, call) repo = Gitlab::Git::Repository.from_gitaly(request.repository, call) user = Gitlab::Git::User.from_gitaly(request.user) |