From b3496e14249edc90c7dd86d1f0ef9804fb46c6b2 Mon Sep 17 00:00:00 2001 From: Zeger-Jan van de Weg Date: Thu, 27 May 2021 11:16:58 +0200 Subject: UserUpdateBranch: Remove feature flag The RPC has two implementation, and this change removes the feature gates that allows the Ruby implementation to be called. As such the next release will allow the removal of that implementation. Changelog: changed --- internal/gitaly/service/operations/branches.go | 25 +--------- .../gitaly/service/operations/testhelper_test.go | 5 -- .../service/operations/update_branches_test.go | 54 ++++++++-------------- internal/metadata/featureflag/feature_flags.go | 3 -- 4 files changed, 21 insertions(+), 66 deletions(-) diff --git a/internal/gitaly/service/operations/branches.go b/internal/gitaly/service/operations/branches.go index f6f3e0e00..8b47e04ec 100644 --- a/internal/gitaly/service/operations/branches.go +++ b/internal/gitaly/service/operations/branches.go @@ -5,8 +5,6 @@ import ( "errors" "gitlab.com/gitlab-org/gitaly/internal/git" - "gitlab.com/gitlab-org/gitaly/internal/gitaly/rubyserver" - "gitlab.com/gitlab-org/gitaly/internal/metadata/featureflag" "gitlab.com/gitlab-org/gitaly/proto/go/gitalypb" "google.golang.org/grpc/codes" "google.golang.org/grpc/status" @@ -76,13 +74,6 @@ func (s *Server) UserCreateBranch(ctx context.Context, req *gitalypb.UserCreateB }, nil } -func (s *Server) UserUpdateBranch(ctx context.Context, req *gitalypb.UserUpdateBranchRequest) (*gitalypb.UserUpdateBranchResponse, error) { - if featureflag.IsDisabled(ctx, featureflag.GoUserUpdateBranch) { - return s.userUpdateBranchRuby(ctx, req) - } - return s.userUpdateBranchGo(ctx, req) -} - func validateUserUpdateBranchGo(req *gitalypb.UserUpdateBranchRequest) error { if req.User == nil { return status.Errorf(codes.InvalidArgument, "empty user") @@ -103,7 +94,7 @@ func validateUserUpdateBranchGo(req *gitalypb.UserUpdateBranchRequest) error { return nil } -func (s *Server) userUpdateBranchGo(ctx context.Context, req *gitalypb.UserUpdateBranchRequest) (*gitalypb.UserUpdateBranchResponse, error) { +func (s *Server) UserUpdateBranch(ctx context.Context, req *gitalypb.UserUpdateBranchRequest) (*gitalypb.UserUpdateBranchResponse, error) { // Validate the request if err := validateUserUpdateBranchGo(req); err != nil { return nil, err @@ -141,20 +132,6 @@ func (s *Server) userUpdateBranchGo(ctx context.Context, req *gitalypb.UserUpdat return &gitalypb.UserUpdateBranchResponse{}, nil } -func (s *Server) userUpdateBranchRuby(ctx context.Context, req *gitalypb.UserUpdateBranchRequest) (*gitalypb.UserUpdateBranchResponse, 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.UserUpdateBranch(clientCtx, req) -} - func (s *Server) UserDeleteBranch(ctx context.Context, req *gitalypb.UserDeleteBranchRequest) (*gitalypb.UserDeleteBranchResponse, error) { // That we do the branch name & user check here first only in // UserDelete but not UserCreate is "intentional", i.e. it's diff --git a/internal/gitaly/service/operations/testhelper_test.go b/internal/gitaly/service/operations/testhelper_test.go index db8338c06..ae8019253 100644 --- a/internal/gitaly/service/operations/testhelper_test.go +++ b/internal/gitaly/service/operations/testhelper_test.go @@ -61,11 +61,6 @@ func TestWithRubySidecar(t *testing.T) { testSuccessfulUserApplyPatch, testUserApplyPatchStableID, testFailedPatchApplyPatch, - testSuccessfulUserUpdateBranchRequestToDelete, - testSuccessfulGitHooksForUserUpdateBranchRequest, - testFailedUserUpdateBranchDueToHooks, - testFailedUserUpdateBranchRequest, - testSuccessfulUserUpdateBranchRequest, testSuccessfulUserRebaseConfirmableRequest, testUserRebaseConfirmableTransaction, testUserRebaseConfirmableStableCommitIDs, diff --git a/internal/gitaly/service/operations/update_branches_test.go b/internal/gitaly/service/operations/update_branches_test.go index 32e89f0a3..6a2781753 100644 --- a/internal/gitaly/service/operations/update_branches_test.go +++ b/internal/gitaly/service/operations/update_branches_test.go @@ -1,7 +1,6 @@ package operations import ( - "context" "crypto/sha1" "fmt" "testing" @@ -10,9 +9,6 @@ import ( "gitlab.com/gitlab-org/gitaly/internal/git" "gitlab.com/gitlab-org/gitaly/internal/git/gittest" "gitlab.com/gitlab-org/gitaly/internal/git/localrepo" - "gitlab.com/gitlab-org/gitaly/internal/gitaly/config" - "gitlab.com/gitlab-org/gitaly/internal/gitaly/rubyserver" - "gitlab.com/gitlab-org/gitaly/internal/metadata/featureflag" "gitlab.com/gitlab-org/gitaly/internal/testhelper" "gitlab.com/gitlab-org/gitaly/proto/go/gitalypb" "google.golang.org/grpc/codes" @@ -25,17 +21,11 @@ var ( oldrev = []byte("0b4bc9a49b562e85de7cc9e834518ea6828729b9") ) -func testSuccessfulUserUpdateBranchRequest(t *testing.T, cfg config.Cfg, rubySrv *rubyserver.Server) { - testhelper.NewFeatureSets([]featureflag.FeatureFlag{ - featureflag.ReferenceTransactions, - featureflag.GoUserUpdateBranch, - }).Run(t, func(t *testing.T, ctx context.Context) { - testSuccessfulUserUpdateBranchRequestFeatured(t, ctx, cfg, rubySrv) - }) -} +func TestSuccessfulUserUpdateBranchRequest(t *testing.T) { + ctx, cancel := testhelper.Context() + defer cancel() -func testSuccessfulUserUpdateBranchRequestFeatured(t *testing.T, ctx context.Context, cfg config.Cfg, rubySrv *rubyserver.Server) { - ctx, cfg, repoProto, repoPath, client := setupOperationsServiceWithRuby(t, ctx, cfg, rubySrv) + ctx, cfg, repoProto, repoPath, client := setupOperationsService(t, ctx) repo := localrepo.NewTestRepo(t, cfg, repoProto) @@ -106,12 +96,11 @@ func testSuccessfulUserUpdateBranchRequestFeatured(t *testing.T, ctx context.Con } } -func testSuccessfulUserUpdateBranchRequestToDelete(t *testing.T, cfg config.Cfg, rubySrv *rubyserver.Server) { - testWithFeature(t, featureflag.GoUserUpdateBranch, cfg, rubySrv, testSuccessfulUserUpdateBranchRequestToDeleteFeatured) -} +func TestSuccessfulUserUpdateBranchRequestToDelete(t *testing.T) { + ctx, cancel := testhelper.Context() + defer cancel() -func testSuccessfulUserUpdateBranchRequestToDeleteFeatured(t *testing.T, ctx context.Context, cfg config.Cfg, rubySrv *rubyserver.Server) { - ctx, cfg, repoProto, repoPath, client := setupOperationsServiceWithRuby(t, ctx, cfg, rubySrv) + ctx, cfg, repoProto, repoPath, client := setupOperationsService(t, ctx) repo := localrepo.NewTestRepo(t, cfg, repoProto) @@ -176,12 +165,11 @@ func testSuccessfulUserUpdateBranchRequestToDeleteFeatured(t *testing.T, ctx con } } -func testSuccessfulGitHooksForUserUpdateBranchRequest(t *testing.T, cfg config.Cfg, rubySrv *rubyserver.Server) { - testWithFeature(t, featureflag.GoUserUpdateBranch, cfg, rubySrv, testSuccessfulGitHooksForUserUpdateBranchRequestFeatured) -} +func TestSuccessfulGitHooksForUserUpdateBranchRequest(t *testing.T) { + ctx, cancel := testhelper.Context() + defer cancel() -func testSuccessfulGitHooksForUserUpdateBranchRequestFeatured(t *testing.T, ctx context.Context, cfg config.Cfg, rubySrv *rubyserver.Server) { - ctx, cfg, _, _, client := setupOperationsServiceWithRuby(t, ctx, cfg, rubySrv) + ctx, cfg, _, _, client := setupOperationsService(t, ctx) for _, hookName := range GitlabHooks { t.Run(hookName, func(t *testing.T) { @@ -210,12 +198,11 @@ func testSuccessfulGitHooksForUserUpdateBranchRequestFeatured(t *testing.T, ctx } } -func testFailedUserUpdateBranchDueToHooks(t *testing.T, cfg config.Cfg, rubySrv *rubyserver.Server) { - testWithFeature(t, featureflag.GoUserUpdateBranch, cfg, rubySrv, testFailedUserUpdateBranchDueToHooksFeatured) -} +func TestFailedUserUpdateBranchDueToHooks(t *testing.T) { + ctx, cancel := testhelper.Context() + defer cancel() -func testFailedUserUpdateBranchDueToHooksFeatured(t *testing.T, ctx context.Context, cfg config.Cfg, rubySrv *rubyserver.Server) { - ctx, _, repo, repoPath, client := setupOperationsServiceWithRuby(t, ctx, cfg, rubySrv) + ctx, _, repo, repoPath, client := setupOperationsService(t, ctx) request := &gitalypb.UserUpdateBranchRequest{ Repository: repo, @@ -243,12 +230,11 @@ func testFailedUserUpdateBranchDueToHooksFeatured(t *testing.T, ctx context.Cont } } -func testFailedUserUpdateBranchRequest(t *testing.T, cfg config.Cfg, rubySrv *rubyserver.Server) { - testWithFeature(t, featureflag.GoUserUpdateBranch, cfg, rubySrv, testFailedUserUpdateBranchRequestFeatured) -} +func TestFailedUserUpdateBranchRequest(t *testing.T) { + ctx, cancel := testhelper.Context() + defer cancel() -func testFailedUserUpdateBranchRequestFeatured(t *testing.T, ctx context.Context, cfg config.Cfg, rubySrv *rubyserver.Server) { - ctx, cfg, repoProto, _, client := setupOperationsServiceWithRuby(t, ctx, cfg, rubySrv) + ctx, cfg, repoProto, _, client := setupOperationsService(t, ctx) repo := localrepo.NewTestRepo(t, cfg, repoProto) diff --git a/internal/metadata/featureflag/feature_flags.go b/internal/metadata/featureflag/feature_flags.go index f119aeade..88f3d69e1 100644 --- a/internal/metadata/featureflag/feature_flags.go +++ b/internal/metadata/featureflag/feature_flags.go @@ -11,8 +11,6 @@ type FeatureFlag struct { var ( // ReferenceTransactions will handle Git reference updates via the transaction service for strong consistency ReferenceTransactions = FeatureFlag{Name: "reference_transactions", OnByDefault: true} - // GoUserUpdateBranch enables the Go implementation of UserUpdateBranch - GoUserUpdateBranch = FeatureFlag{Name: "go_user_update_branch", OnByDefault: true} // UserRebaseConfirmable GoUserRebaseConfirmable = FeatureFlag{Name: "go_user_rebase_confirmable", OnByDefault: true} // GoUpdateRemoteMirror enables the Go implementation of UpdateRemoteMirror @@ -30,7 +28,6 @@ var ( // All includes all feature flags. var All = []FeatureFlag{ ReferenceTransactions, - GoUserUpdateBranch, GoUserRebaseConfirmable, GrpcTreeEntryNotFound, GoUpdateRemoteMirror, -- cgit v1.2.3