Welcome to mirror list, hosted at ThFree Co, Russian Federation.

gitlab.com/gitlab-org/gitaly.git - Unnamed repository; edit this file 'description' to name the repository.
summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
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
commitfbc9f83ab7a03f167c8c82eb2d7beb09215df364 (patch)
treec34e4668da151f8a758b2d7eef2f367fe889920d
parent6a06feda7fd01961bb332afce4d7f7b4ce4a5aad (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.go19
-rw-r--r--internal/gitaly/service/operations/branches_test.go18
-rw-r--r--internal/gitaly/service/operations/tags.go21
-rw-r--r--internal/gitaly/service/operations/tags_test.go13
-rw-r--r--internal/metadata/featureflag/feature_flags.go6
-rw-r--r--ruby/lib/gitaly_server/operations_service.rb32
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)