From d15bd8e09adc4714b5cd98bb3cf42dabc6674ca9 Mon Sep 17 00:00:00 2001 From: Zoltan Szabo <63643463+zoltanszabo-bitrise@users.noreply.github.com> Date: Thu, 12 Nov 2020 16:21:41 +0100 Subject: [PATCH] Add branch recommendations to checkout errors [ACT-912] (#129) * refactor: Rename mapRecommendation to better state its purpose * test: Add new test cases for branch recommendation * feat: Implement new step error constructor * test: Add tests for branch related error handling * refactor: branch related error handling --- gitclone/git.go | 70 ++++++++++------- gitclone/git_test.go | 152 +++++++++++++++++++++++++++++++++++++ gitclone/steperror.go | 40 +++++++--- gitclone/steperror_test.go | 79 ++++++++++++++----- 4 files changed, 282 insertions(+), 59 deletions(-) diff --git a/gitclone/git.go b/gitclone/git.go index 422f7baa..81baa98c 100644 --- a/gitclone/git.go +++ b/gitclone/git.go @@ -13,7 +13,6 @@ import ( "strconv" "strings" - "github.com/bitrise-io/bitrise-init/errormapper" "github.com/bitrise-io/bitrise-init/step" "github.com/bitrise-io/go-utils/command" "github.com/bitrise-io/go-utils/command/git" @@ -27,7 +26,6 @@ import ( const ( checkoutFailedTag = "checkout_failed" fetchFailedTag = "fetch_failed" - branchRecKey = "BranchRecommendation" ) func isOriginPresent(gitCmd git.Git, dir, repoURL string) (bool, error) { @@ -296,6 +294,22 @@ func manualMerge(gitCmd git.Git, repoURL, prRepoURL, branch, commit, branchDest return nil } +type getAvailableBranches func() (map[string][]string, error) + +func listBranches(gitCmd git.Git) getAvailableBranches { + return func() (map[string][]string, error) { + if err := run(gitCmd.Fetch()); err != nil { + return nil, err + } + out, err := output(gitCmd.Branch("-r")) + if err != nil { + return nil, err + } + + return parseListBranchesOutput(out), nil + } +} + func parseListBranchesOutput(output string) map[string][]string { lines := strings.Split(output, "\n") branchesByRemote := map[string][]string{} @@ -315,16 +329,29 @@ func parseListBranchesOutput(output string) map[string][]string { return branchesByRemote } -func listBranches(gitCmd git.Git) (map[string][]string, error) { - if err := run(gitCmd.Fetch()); err != nil { - return nil, err - } - out, err := output(gitCmd.Branch("-r")) - if err != nil { - return nil, err +func handleCheckoutError(callback getAvailableBranches, tag string, err error, shortMsg string, branch string) *step.Error { + // We were checking out a branch (not tag or commit) + if branch != "" { + branchesByRemote, branchesErr := callback() + branches := branchesByRemote[defaultRemoteName] + // There was no error grabbing the available branches + // And the current branch is not present in the list + if branchesErr == nil && !sliceutil.IsStringInSlice(branch, branches) { + return newStepErrorWithBranchRecommendations( + tag, + err, + shortMsg, + branch, + branches, + ) + } } - return parseListBranchesOutput(out), nil + return newStepError( + tag, + err, + shortMsg, + ) } func checkout(gitCmd git.Git, arg, branch string, depth int, isTag bool) *step.Error { @@ -341,34 +368,23 @@ func checkout(gitCmd git.Git, arg, branch string, depth int, isTag bool) *step.E } return gitCmd.Fetch(opts...) }); err != nil { - if branch != "" { - branchesByRemote, branchesErr := listBranches(gitCmd) - branches := branchesByRemote[defaultRemoteName] - if branchesErr == nil && !sliceutil.IsStringInSlice(branch, branches) { - return newStepErrorWithRecommendations( - fetchFailedTag, - fmt.Errorf("fetch failed: invalid branch selected: %s, available branches: %s: %v", branch, strings.Join(branches, ", "), err), - "Fetching repository has failed", - step.Recommendation{ - branchRecKey: branches, - errormapper.DetailedErrorRecKey: newFetchFailedInvalidBranchDetailedError(branch), - }, - ) - } - } - return newStepError( + return handleCheckoutError( + listBranches(gitCmd), fetchFailedTag, fmt.Errorf("fetch failed, error: %v", err), "Fetching repository has failed", + branch, ) } if err := run(gitCmd.Checkout(arg)); err != nil { if depth == 0 { - return newStepError( + return handleCheckoutError( + listBranches(gitCmd), checkoutFailedTag, fmt.Errorf("checkout failed (%s), error: %v", arg, err), "Checkout has failed", + branch, ) } log.Warnf("Checkout failed, error: %v\nUnshallow...", err) diff --git a/gitclone/git_test.go b/gitclone/git_test.go index 63cd7234..476fdb5a 100644 --- a/gitclone/git_test.go +++ b/gitclone/git_test.go @@ -1,8 +1,12 @@ package gitclone import ( + "errors" "reflect" "testing" + + "github.com/bitrise-io/bitrise-init/errormapper" + "github.com/bitrise-io/bitrise-init/step" ) func TestFetchArg(t *testing.T) { @@ -96,3 +100,151 @@ func Test_parseListBranchesOutput(t *testing.T) { }) } } + +func Test_handleCheckoutError(t *testing.T) { + type args struct { + callback func() (map[string][]string, error) + tag string + err error + shortMsg string + branch string + } + tests := []struct { + name string + args args + want *step.Error + }{ + { + name: "handleCheckoutError: generic error without branch recommendation", + args: args{ + callback: func() (map[string][]string, error) { return nil, nil }, + tag: "fetch_failed", + err: errors.New("Something bad happened"), + shortMsg: "Fetching repository has failed", + branch: "", + }, + want: &step.Error{ + StepID: "git-clone", + Tag: "fetch_failed", + Err: errors.New("Something bad happened"), + ShortMsg: "Fetching repository has failed", + Recommendations: step.Recommendation{ + errormapper.DetailedErrorRecKey: errormapper.DetailedError{ + Title: "We couldn’t fetch your repository.", + Description: "Our auto-configurator returned the following error:\nSomething bad happened", + }, + }, + }, + }, + { + name: "handleCheckoutError: specific error with branch recommendations for default remote", + args: args{ + callback: func() (map[string][]string, error) { + return map[string][]string{ + defaultRemoteName: {"master", "develop"}, + }, nil + }, + tag: "checkout_failed", + err: errors.New("pathspec 'test' did not match any file(s) known to git"), + shortMsg: "Checkout has failed", + branch: "test", + }, + want: &step.Error{ + StepID: "git-clone", + Tag: "checkout_failed", + Err: errors.New("pathspec 'test' did not match any file(s) known to git"), + ShortMsg: "Checkout has failed", + Recommendations: step.Recommendation{ + branchRecKey: []string{"master", "develop"}, + errormapper.DetailedErrorRecKey: errormapper.DetailedError{ + Title: "We couldn't find the branch 'test'.", + Description: "Please choose another branch and try again.", + }, + }, + }, + }, + { + name: "handleCheckoutError: specific error without branch recommendations due to error", + args: args{ + callback: func() (map[string][]string, error) { + return nil, errors.New("No available branches") + }, + tag: "checkout_failed", + err: errors.New("pathspec 'test' did not match any file(s) known to git"), + shortMsg: "Checkout has failed", + branch: "test", + }, + want: &step.Error{ + StepID: "git-clone", + Tag: "checkout_failed", + Err: errors.New("pathspec 'test' did not match any file(s) known to git"), + ShortMsg: "Checkout has failed", + Recommendations: step.Recommendation{ + errormapper.DetailedErrorRecKey: errormapper.DetailedError{ + Title: "We couldn't find the branch 'test'.", + Description: "Please choose another branch and try again.", + }, + }, + }, + }, + { + name: "handleCheckoutError: specific error without branch recommendations due correct branch", + args: args{ + callback: func() (map[string][]string, error) { + return map[string][]string{ + defaultRemoteName: {"master", "develop", "test"}, + }, nil + }, + tag: "checkout_failed", + err: errors.New("pathspec 'test' did not match any file(s) known to git"), + shortMsg: "Checkout has failed", + branch: "test", + }, + want: &step.Error{ + StepID: "git-clone", + Tag: "checkout_failed", + Err: errors.New("pathspec 'test' did not match any file(s) known to git"), + ShortMsg: "Checkout has failed", + Recommendations: step.Recommendation{ + errormapper.DetailedErrorRecKey: errormapper.DetailedError{ + Title: "We couldn't find the branch 'test'.", + Description: "Please choose another branch and try again.", + }, + }, + }, + }, + { + name: "handleCheckoutError: specific error without branch recommendations for default remote", + args: args{ + callback: func() (map[string][]string, error) { + return map[string][]string{ + "something": {"master", "develop"}, + }, nil + }, + tag: "checkout_failed", + err: errors.New("pathspec 'test' did not match any file(s) known to git"), + shortMsg: "Checkout has failed", + branch: "test", + }, + want: &step.Error{ + StepID: "git-clone", + Tag: "checkout_failed", + Err: errors.New("pathspec 'test' did not match any file(s) known to git"), + ShortMsg: "Checkout has failed", + Recommendations: step.Recommendation{ + errormapper.DetailedErrorRecKey: errormapper.DetailedError{ + Title: "We couldn't find the branch 'test'.", + Description: "Please choose another branch and try again.", + }, + }, + }, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := handleCheckoutError(tt.args.callback, tt.args.tag, tt.args.err, tt.args.shortMsg, tt.args.branch); !reflect.DeepEqual(got, tt.want) { + t.Errorf("handleCheckoutError() = %v, want %v", got, tt.want) + } + }) + } +} diff --git a/gitclone/steperror.go b/gitclone/steperror.go index a01a4fb5..b69c88ed 100644 --- a/gitclone/steperror.go +++ b/gitclone/steperror.go @@ -7,7 +7,11 @@ import ( "github.com/bitrise-io/bitrise-init/step" ) -func mapRecommendation(tag, errMsg string) step.Recommendation { +const ( + branchRecKey = "BranchRecommendation" +) + +func mapDetailedErrorRecommendation(tag, errMsg string) step.Recommendation { switch tag { case checkoutFailedTag: matcher := newCheckoutFailedPatternErrorMatcher() @@ -15,29 +19,43 @@ func mapRecommendation(tag, errMsg string) step.Recommendation { case updateSubmodelFailedTag: // update_submodule_failed could have the same errors as fetch fallthrough case fetchFailedTag: - fetchFailedMatcher := newFetchFailedPatternErrorMatcher() - return fetchFailedMatcher.Run(errMsg) + matcher := newFetchFailedPatternErrorMatcher() + return matcher.Run(errMsg) } return nil } func newStepError(tag string, err error, shortMsg string) *step.Error { - recommendation := mapRecommendation(tag, err.Error()) - if recommendation != nil { - return step.NewErrorWithRecommendations("git-clone", tag, err, shortMsg, recommendation) + recommendations := mapDetailedErrorRecommendation(tag, err.Error()) + if recommendations != nil { + return step.NewErrorWithRecommendations("git-clone", tag, err, shortMsg, recommendations) } return step.NewError("git-clone", tag, err, shortMsg) } -func newStepErrorWithRecommendations(tag string, err error, shortMsg string, recommendations step.Recommendation) *step.Error { - return step.NewErrorWithRecommendations("git-clone", tag, err, shortMsg, recommendations) +func newStepErrorWithBranchRecommendations(tag string, err error, shortMsg, currentBranch string, availableBranches []string) *step.Error { + // First: Map the error messages + mappedError := newStepError(tag, err, shortMsg) + + // Second: Extend recommendation with available branches, if has any + if len(availableBranches) > 0 { + rec := mappedError.Recommendations + if rec == nil { + rec = step.Recommendation{} + } + rec[branchRecKey] = availableBranches + } + + return mappedError } func newCheckoutFailedPatternErrorMatcher() *errormapper.PatternErrorMatcher { return &errormapper.PatternErrorMatcher{ - DefaultBuilder: newCheckoutFailedGenericDetailedError, - PatternToBuilder: nil, + DefaultBuilder: newCheckoutFailedGenericDetailedError, + PatternToBuilder: errormapper.PatternToDetailedErrorBuilder{ + `pathspec '(.+)' did not match any file\(s\) known to git`: newInvalidBranchDetailedError, + }, } } @@ -122,7 +140,7 @@ func newFetchFailedSamlSSOEnforcedDetailedError(...string) errormapper.DetailedE } } -func newFetchFailedInvalidBranchDetailedError(params ...string) errormapper.DetailedError { +func newInvalidBranchDetailedError(params ...string) errormapper.DetailedError { branch := errormapper.GetParamAt(0, params) return errormapper.DetailedError{ Title: fmt.Sprintf("We couldn't find the branch '%s'.", branch), diff --git a/gitclone/steperror_test.go b/gitclone/steperror_test.go index 929da86d..315342e1 100644 --- a/gitclone/steperror_test.go +++ b/gitclone/steperror_test.go @@ -9,7 +9,7 @@ import ( "github.com/bitrise-io/bitrise-init/step" ) -func Test_mapRecommendation(t *testing.T) { +func Test_mapDetailedErrorRecommendation(t *testing.T) { type args struct { tag string errMsg string @@ -23,11 +23,22 @@ func Test_mapRecommendation(t *testing.T) { name: "checkout_failed generic error mapping", args: args{ tag: checkoutFailedTag, - errMsg: "error: pathspec 'master' did not match any file(s) known to git.", + errMsg: "error: fatal: /master: '/master' is outside repository", }, want: errormapper.NewDetailedErrorRecommendation(errormapper.DetailedError{ Title: "We couldn’t checkout your branch.", - Description: "Our auto-configurator returned the following error:\nerror: pathspec 'master' did not match any file(s) known to git.", + Description: "Our auto-configurator returned the following error:\nerror: fatal: /master: '/master' is outside repository", + }), + }, + { + name: "checkout_failed invalid banch error mapping", + args: args{ + tag: checkoutFailedTag, + errMsg: "error: pathspec 'master' did not match any file(s) known to git.", + }, + want: errormapper.NewDetailedErrorRecommendation(errormapper.DetailedError{ + Title: "We couldn't find the branch 'master'.", + Description: "Please choose another branch and try again.", }), }, { @@ -252,7 +263,7 @@ func Test_mapRecommendation(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - if got := mapRecommendation(tt.args.tag, tt.args.errMsg); !reflect.DeepEqual(got, tt.want) { + if got := mapDetailedErrorRecommendation(tt.args.tag, tt.args.errMsg); !reflect.DeepEqual(got, tt.want) { t.Errorf("mapRecommendation() = %v, want %v", got, tt.want) } }) @@ -312,12 +323,13 @@ func Test_newStepError(t *testing.T) { } } -func Test_newStepErrorWithRecommendations(t *testing.T) { +func Test_newStepErrorWithBranchRecommendations(t *testing.T) { type args struct { - tag string - err error - shortMsg string - recommendations step.Recommendation + tag string + err error + shortMsg string + currentBranch string + availableBranches []string } tests := []struct { name string @@ -325,30 +337,55 @@ func Test_newStepErrorWithRecommendations(t *testing.T) { want *step.Error }{ { - name: "newStepErrorWithRecommendations", + name: "newStepErrorWithBranchRecommendations without available branches", args: args{ - tag: "test_tag", - err: errors.New("fatal error"), - shortMsg: "unknown error", - recommendations: step.Recommendation{ - "Test": "Passed", + tag: "checkout_failed", + err: errors.New("Generic error"), + shortMsg: "Checkout has failed", + currentBranch: "feature1", + availableBranches: nil, + }, + want: &step.Error{ + StepID: "git-clone", + Tag: "checkout_failed", + Err: errors.New("Generic error"), + ShortMsg: "Checkout has failed", + Recommendations: step.Recommendation{ + errormapper.DetailedErrorRecKey: errormapper.DetailedError{ + Title: "We couldn’t checkout your branch.", + Description: "Our auto-configurator returned the following error:\nGeneric error", + }, }, }, + }, + { + name: "newStepErrorWithBranchRecommendations with available branches", + args: args{ + tag: "checkout_failed", + err: errors.New("pathspec 'feature1' did not match any file(s) known to git"), + shortMsg: "Checkout has failed", + currentBranch: "feature1", + availableBranches: []string{"master", "develop", "hotfix"}, + }, want: &step.Error{ StepID: "git-clone", - Tag: "test_tag", - Err: errors.New("fatal error"), - ShortMsg: "unknown error", + Tag: "checkout_failed", + Err: errors.New("pathspec 'feature1' did not match any file(s) known to git"), + ShortMsg: "Checkout has failed", Recommendations: step.Recommendation{ - "Test": "Passed", + branchRecKey: []string{"master", "develop", "hotfix"}, + errormapper.DetailedErrorRecKey: errormapper.DetailedError{ + Title: "We couldn't find the branch 'feature1'.", + Description: "Please choose another branch and try again.", + }, }, }, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - if got := newStepErrorWithRecommendations(tt.args.tag, tt.args.err, tt.args.shortMsg, tt.args.recommendations); !reflect.DeepEqual(got, tt.want) { - t.Errorf("newStepErrorWithRecommendations() = %v, want %v", got, tt.want) + if got := newStepErrorWithBranchRecommendations(tt.args.tag, tt.args.err, tt.args.shortMsg, tt.args.currentBranch, tt.args.availableBranches); !reflect.DeepEqual(got, tt.want) { + t.Errorf("newStepErrorWithBranchRecommendations() = %v,want %v", got, tt.want) } }) }