fix: ensure pull request exists during conflict resolution (lindell#369)

Signed-off-by: Raffael Sahli <[email protected]>
Co-authored-by: Johan Lindell <[email protected]>

internal/multigitter/print.go

@@ -3,11 +3,12 @@ package multigitter
 import (
 	"context"
 	"fmt"
+	"io"
+	"os"
+
 	"github.com/lindell/multi-gitter/internal/multigitter/repocounter"
 	"github.com/lindell/multi-gitter/internal/scm"
 	log "github.com/sirupsen/logrus"
-	"io"
-	"os"
 )
 
 // Printer contains fields to be able to do the print command
@@ -48,7 +49,7 @@ func (r Printer) Print(ctx context.Context) error {
 			if err != errAborted {
 				logger.Info(err)
 			}
-			rc.AddError(err, repos[i])
+			rc.AddError(err, repos[i], nil)
 			return
 		}
 

internal/multigitter/repocounter/counter.go

@@ -13,24 +13,32 @@ import (
 type Counter struct {
 	successPullRequests []scm.PullRequest
 	successRepositories []scm.Repository
-	errorRepositories   map[string][]scm.Repository
+	errors              map[string][]errorInfo
 	lock                sync.RWMutex
 }
 
+type errorInfo struct {
+	repository  scm.Repository
+	pullRequest scm.PullRequest
+}
+
 // NewCounter create a new repo counter
 func NewCounter() *Counter {
 	return &Counter{
-		errorRepositories: map[string][]scm.Repository{},
+		errors: map[string][]errorInfo{},
 	}
 }
 
 // AddError add a failing repository together with the error that caused it
-func (r *Counter) AddError(err error, repo scm.Repository) {
+func (r *Counter) AddError(err error, repo scm.Repository, pr scm.PullRequest) {
 	defer r.lock.Unlock()
 	r.lock.Lock()
 
 	msg := err.Error()
-	r.errorRepositories[msg] = append(r.errorRepositories[msg], repo)
+	r.errors[msg] = append(r.errors[msg], errorInfo{
+		repository:  repo,
+		pullRequest: pr,
+	})
 }
 
 // AddSuccessRepositories adds a repository that succeeded
@@ -42,11 +50,11 @@ func (r *Counter) AddSuccessRepositories(repo scm.Repository) {
 }
 
 // AddSuccessPullRequest adds a pullrequest that succeeded
-func (r *Counter) AddSuccessPullRequest(repo scm.PullRequest) {
+func (r *Counter) AddSuccessPullRequest(pr scm.PullRequest) {
 	defer r.lock.Unlock()
 	r.lock.Lock()
 
-	r.successPullRequests = append(r.successPullRequests, repo)
+	r.successPullRequests = append(r.successPullRequests, pr)
 }
 
 // Info returns a formatted string about all repositories
@@ -56,10 +64,18 @@ func (r *Counter) Info() string {
 
 	var exitInfo string
 
-	for errMsg := range r.errorRepositories {
+	for errMsg := range r.errors {
 		exitInfo += fmt.Sprintf("%s:\n", strings.ToUpper(errMsg[0:1])+errMsg[1:])
-		for _, repo := range r.errorRepositories[errMsg] {
-			exitInfo += fmt.Sprintf("  %s\n", repo.FullName())
+		for _, errInfo := range r.errors[errMsg] {
+			if errInfo.pullRequest == nil {
+				exitInfo += fmt.Sprintf("  %s\n", errInfo.repository.FullName())
+			} else {
+				if urler, ok := errInfo.pullRequest.(urler); ok {
+					exitInfo += fmt.Sprintf("  %s\n", terminal.Link(errInfo.pullRequest.String(), urler.URL()))
+				} else {
+					exitInfo += fmt.Sprintf("  %s\n", errInfo.pullRequest.String())
+				}
+			}
 		}
 	}
 

internal/multigitter/run.go

@@ -121,16 +121,17 @@ func (r *Runner) Run(ctx context.Context) error {
 		defer func() {
 			if r := recover(); r != nil {
 				log.Error(r)
-				rc.AddError(errors.New("run paniced"), repos[i])
+				rc.AddError(errors.New("run paniced"), repos[i], nil)
 			}
 		}()
 
 		pr, err := r.runSingleRepo(ctx, repos[i])
+
 		if err != nil {
 			if err != errAborted {
 				logger.Info(err)
 			}
-			rc.AddError(err, repos[i])
+			rc.AddError(err, repos[i], pr)
 
 			if log.IsLevelEnabled(log.TraceLevel) {
 				if stackTrace := getStackTrace(err); stackTrace != "" {
@@ -296,7 +297,12 @@ func (r *Runner) runSingleRepo(ctx context.Context, repo scm.Repository) (scm.Pu
 		if err != nil {
 			return nil, errors.Wrap(err, "could not verify if branch already exists")
 		} else if featureBranchExist && r.ConflictStrategy == ConflictStrategySkip {
-			return nil, errBranchExist
+			pr, err := r.ensurePullRequestExists(ctx, log, repo, prRepo, baseBranch, featureBranchExist)
+			if err != nil {
+				return nil, err
+			}
+
+			return pr, errBranchExist
 		}
 	}
 
@@ -307,6 +313,10 @@ func (r *Runner) runSingleRepo(ctx context.Context, repo scm.Repository) (scm.Pu
 		return nil, errors.Wrap(err, "could not push changes")
 	}
 
+	return r.ensurePullRequestExists(ctx, log, repo, prRepo, baseBranch, featureBranchExist)
+}
+
+func (r *Runner) ensurePullRequestExists(ctx context.Context, log log.FieldLogger, repo scm.Repository, prRepo scm.Repository, baseBranch string, featureBranchExist bool) (scm.PullRequest, error) {
 	if r.SkipPullRequest {
 		return nil, nil
 	}
@@ -321,29 +331,23 @@ func (r *Runner) runSingleRepo(ctx context.Context, repo scm.Repository) (scm.Pu
 		existingPullRequest = pr
 	}
 
-	var pr scm.PullRequest
 	if existingPullRequest != nil {
 		log.Info("Skip creating pull requests since one is already open")
-		pr = existingPullRequest
-	} else {
-		log.Info("Creating pull request")
-		pr, err = r.VersionController.CreatePullRequest(ctx, repo, prRepo, scm.NewPullRequest{
-			Title:         r.PullRequestTitle,
-			Body:          r.PullRequestBody,
-			Head:          r.FeatureBranch,
-			Base:          baseBranch,
-			Reviewers:     getReviewers(r.Reviewers, r.MaxReviewers),
-			TeamReviewers: getReviewers(r.TeamReviewers, r.MaxTeamReviewers),
-			Assignees:     r.Assignees,
-			Draft:         r.Draft,
-			Labels:        r.Labels,
-		})
-		if err != nil {
-			return nil, err
-		}
+		return existingPullRequest, nil
 	}
 
-	return pr, nil
+	log.Info("Creating pull request")
+	return r.VersionController.CreatePullRequest(ctx, repo, prRepo, scm.NewPullRequest{
+		Title:         r.PullRequestTitle,
+		Body:          r.PullRequestBody,
+		Head:          r.FeatureBranch,
+		Base:          baseBranch,
+		Reviewers:     getReviewers(r.Reviewers, r.MaxReviewers),
+		TeamReviewers: getReviewers(r.TeamReviewers, r.MaxTeamReviewers),
+		Assignees:     r.Assignees,
+		Draft:         r.Draft,
+		Labels:        r.Labels,
+	})
 }
 
 var interactiveInfo = `(V)iew changes. (A)ccept or (R)eject`

tests/table_test.go

@@ -378,11 +378,28 @@ func TestTable(t *testing.T) {
 				changeBranch(t, repo.Path, "custom-branch-name", true)
 				changeTestFile(t, repo.Path, "i like apple", "test change")
 				changeBranch(t, repo.Path, "master", false)
+
+				repoExistingPR := createRepo(t, "owner", "already-existing-branch-and-pr", "i like apples")
+				changeBranch(t, repoExistingPR.Path, "custom-branch-name", true)
+				changeTestFile(t, repoExistingPR.Path, "i like apple", "test change")
+				changeBranch(t, repoExistingPR.Path, "master", false)
+
 				return &vcmock.VersionController{
 					Repositories: []vcmock.Repository{
 						repo,
+						repoExistingPR,
 						createRepo(t, "owner", "should-change", "i like apples"),
 					},
+					PullRequests: []vcmock.PullRequest{
+						{
+							PRStatus:   scm.PullRequestStatusPending,
+							PRNumber:   10,
+							Repository: repoExistingPR,
+							NewPullRequest: scm.NewPullRequest{
+								Head: "custom-branch-name",
+							},
+						},
+					},
 				}
 			},
 			args: []string{
@@ -394,14 +411,15 @@ func TestTable(t *testing.T) {
 				changerBinaryPath,
 			},
 			verify: func(t *testing.T, vcMock *vcmock.VersionController, runData runData) {
-				require.Len(t, vcMock.PullRequests, 1)
-				assert.Contains(t, runData.logOut, "Running on 2 repositories")
+				require.Len(t, vcMock.PullRequests, 3)
+				assert.Contains(t, runData.logOut, "Running on 3 repositories")
 				assert.Contains(t, runData.logOut, "Cloning and running script")
 
 				assert.Equal(t, `The new branch already exists:
-  owner/already-existing-branch
+  owner/already-existing-branch #1
+  owner/already-existing-branch-and-pr #10
 Repositories with a successful run:
-  owner/should-change #1
+  owner/should-change #2
 `, runData.out)
 			},
 		},