Merge pull request #30924 from cjwagner/unify-git

Unify v1 and v2 git clients.

Author: k8s-ci-robot

prow/cmd/config-bootstrapper/main_test.go

@@ -39,10 +39,6 @@ var (
 	defaultBranch    = localgit.DefaultBranch("")
 )
 
-func TestRun(t *testing.T) {
-	testRun(localgit.New, t)
-}
-
 func TestRunV2(t *testing.T) {
 	testRun(localgit.NewV2, t)
 }

prow/config/inrepoconfig_test.go

@@ -625,10 +625,6 @@ postsubmits: [{"name": "oli", "spec": {"containers": [{}]}}]`),
 	}
 }
 
-func TestDefaultProwYAMLGetter_RejectsJustOrg(t *testing.T) {
-	testDefaultProwYAMLGetter_RejectsJustOrg(localgit.New, t)
-}
-
 func TestDefaultProwYAMLGetter_RejectsJustOrgV2(t *testing.T) {
 	testDefaultProwYAMLGetter_RejectsJustOrg(localgit.NewV2, t)
 }

prow/external-plugins/cherrypicker/server_test.go

@@ -263,11 +263,6 @@ func makeFakeRepoWithCommit(clients localgit.Clients, t *testing.T) (*localgit.L
 	return lg, c
 }
 
-func TestCherryPickIC(t *testing.T) {
-	t.Parallel()
-	testCherryPickIC(localgit.New, t)
-}
-
 func TestCherryPickICV2(t *testing.T) {
 	t.Parallel()
 	testCherryPickIC(localgit.NewV2, t)
@@ -346,11 +341,6 @@ func testCherryPickIC(clients localgit.Clients, t *testing.T) {
 	}
 }
 
-func TestCherryPickPR(t *testing.T) {
-	t.Parallel()
-	testCherryPickPR(localgit.New, t)
-}
-
 func TestCherryPickPRV2(t *testing.T) {
 	t.Parallel()
 	testCherryPickPR(localgit.NewV2, t)
@@ -501,11 +491,6 @@ func testCherryPickPR(clients localgit.Clients, t *testing.T) {
 	}
 }
 
-func TestCherryPickOfCherryPickPR(t *testing.T) {
-	t.Parallel()
-	testCherryPickOfCherryPickPR(localgit.New, t)
-}
-
 func TestCherryPickOfCherryPickPRV2(t *testing.T) {
 	t.Parallel()
 	testCherryPickOfCherryPickPR(localgit.NewV2, t)
@@ -630,11 +615,6 @@ func testCherryPickOfCherryPickPR(clients localgit.Clients, t *testing.T) {
 	}
 }
 
-func TestCherryPickPRWithLabels(t *testing.T) {
-	t.Parallel()
-	testCherryPickPRWithLabels(localgit.New, t)
-}
-
 func TestCherryPickPRWithLabelsV2(t *testing.T) {
 	t.Parallel()
 	testCherryPickPRWithLabels(localgit.NewV2, t)
@@ -913,11 +893,6 @@ func TestCherryPickCreateIssue(t *testing.T) {
 	}
 }
 
-func TestCherryPickPRAssignments(t *testing.T) {
-	t.Parallel()
-	testCherryPickPRAssignments(localgit.New, t)
-}
-
 func TestCherryPickPRAssignmentsV2(t *testing.T) {
 	t.Parallel()
 	testCherryPickPRAssignments(localgit.NewV2, t)

prow/flagutil/git.go

@@ -31,7 +31,6 @@ import (
 	utilerrors "k8s.io/apimachinery/pkg/util/errors"
 
 	"k8s.io/test-infra/prow/config/secret"
-	"k8s.io/test-infra/prow/git"
 	gitv2 "k8s.io/test-infra/prow/git/v2"
 	"k8s.io/test-infra/prow/github"
 )
@@ -337,57 +336,34 @@ func (o *GitHubOptions) GitHubClientWithAccessToken(token string) (github.Client
 // TODO(chaodaiG): move this logic to somewhere more appropriate instead of in
 // github.go.
 func (o *GitHubOptions) GitClientFactory(cookieFilePath string, cacheDir *string, dryRun, persistCache bool) (gitv2.ClientFactory, error) {
-	var gitClientFactory gitv2.ClientFactory
-	if cookieFilePath != "" && o.TokenPath == "" && o.AppPrivateKeyPath == "" {
-		opts := gitv2.ClientFactoryOpts{
-			CookieFilePath: cookieFilePath,
-			Persist:        &persistCache,
-		}
-		if cacheDir != nil && *cacheDir != "" {
-			opts.CacheDirBase = cacheDir
-		}
-		var err error
-		gitClientFactory, err = gitv2.NewClientFactory(opts.Apply)
-		if err != nil {
-			return nil, fmt.Errorf("failed to create git client from cookieFile: %v\n(cookieFile is only for Gerrit)", err)
-		}
-	} else {
-		gitClient, err := o.GitClient(dryRun)
-		if err != nil {
-			return nil, fmt.Errorf("Error getting git client: %w", err)
-		}
-		gitClientFactory = gitv2.ClientFactoryFrom(gitClient)
+	opts := gitv2.ClientFactoryOpts{
+		CookieFilePath: cookieFilePath,
+		Host:           o.Host,
+		Persist:        &persistCache,
 	}
-
-	return gitClientFactory, nil
-}
-
-// GitClient returns a Git client.
-func (o *GitHubOptions) GitClient(dryRun bool) (client *git.Client, err error) {
-	client, err = git.NewClientWithHost(o.Host)
-	if err != nil {
-		return nil, err
+	if cacheDir != nil && *cacheDir != "" {
+		opts.CacheDirBase = cacheDir
 	}
 
-	// We must capture the value of client here to prevent issues related
-	// to the use of named return values when an error is encountered.
-	// Without this, we risk a nil pointer dereference.
-	defer func(client *git.Client) {
+	if cookieFilePath == "" && (o.TokenPath != "" || o.AppPrivateKeyPath != "") {
+		// Make a client with auth suitable for GitHub
+		user, generator, err := o.getGitHubAuthentication(dryRun)
 		if err != nil {
-			client.Clean()
+			return nil, fmt.Errorf("failed to get git authentication: %w", err)
 		}
-	}(client)
+		opts.Username = func() (string, error) { return user, nil }
+		opts.Token = generator
+	}
+	// If the client is for Gerrit we're already set with the cookie filepath.
 
-	user, generator, err := o.getGitAuthentication(dryRun)
+	gitClientFactory, err := gitv2.NewClientFactory(opts.Apply)
 	if err != nil {
-		return nil, fmt.Errorf("failed to get git authentication: %w", err)
+		return nil, fmt.Errorf("failed to create git client factory: %w", err)
 	}
-	client.SetCredentials(user, generator)
-
-	return client, nil
+	return gitClientFactory, nil
 }
 
-func (o *GitHubOptions) getGitAuthentication(dryRun bool) (string, git.GitTokenGenerator, error) {
+func (o *GitHubOptions) getGitHubAuthentication(dryRun bool) (string, gitv2.TokenGetter, error) {
 	// the client must have been created at least once for us to have generators
 	if o.userGenerator == nil {
 		if _, err := o.GitHubClient(dryRun); err != nil {
@@ -399,7 +375,7 @@ func (o *GitHubOptions) getGitAuthentication(dryRun bool) (string, git.GitTokenG
 	if err != nil {
 		return "", nil, fmt.Errorf("error getting bot name: %w", err)
 	}
-	return login, git.GitTokenGenerator(o.tokenGenerator), nil
+	return login, gitv2.TokenGetter(o.tokenGenerator), nil
 }
 
 func (o *GitHubOptions) appPrivateKeyGenerator() (func() *rsa.PrivateKey, error) {