Allow optionally aggregating all fixes into one pull request (#314)

Author: EyalDelarea

commands/createfixpullrequests.go

@@ -31,6 +31,8 @@ type CreateFixPullRequestsCmd struct {
 	projectWorkingDir string
 	// The git client the command performs git operations with
 	gitManager *utils.GitManager
+	// Determines whether to open a pull request for each vulnerability fix or to aggregate all fixes into one pull request.
+	aggregateFixes bool
 }
 
 func (cfp *CreateFixPullRequestsCmd) Run(configAggregator utils.FrogbotConfigAggregator, client vcsclient.VcsClient) error {
@@ -63,6 +65,7 @@ func (cfp *CreateFixPullRequestsCmd) scanAndFixRepository(repository *utils.Frog
 	}
 	for _, project := range repository.Projects {
 		cfp.details.Project = project
+		cfp.aggregateFixes = repository.Git.AggregateFixes
 		projectFullPathWorkingDirs := getFullPathWorkingDirs(project.WorkingDirs, baseWd)
 		for _, fullPathWd := range projectFullPathWorkingDirs {
 			scanResults, isMultipleRoots, err := cfp.scan(cfp.details, fullPathWd)
@@ -134,9 +137,17 @@ func (cfp *CreateFixPullRequestsCmd) fixVulnerablePackages(fixVersionsMap map[st
 		}
 	}()
 
-	// Fix all impacted packages
+	if cfp.aggregateFixes {
+		err = cfp.fixIssuesSinglePR(fixVersionsMap)
+	} else {
+		err = cfp.fixIssuesSeparatePRs(fixVersionsMap)
+	}
+	return
+}
+
+func (cfp *CreateFixPullRequestsCmd) fixIssuesSeparatePRs(fixVersionsMap map[string]*utils.FixVersionInfo) (err error) {
 	for impactedPackage, fixVersionInfo := range fixVersionsMap {
-		if err = cfp.fixSinglePackage(impactedPackage, fixVersionInfo); err != nil {
+		if err = cfp.fixSinglePackageAndCreatePR(impactedPackage, fixVersionInfo); err != nil {
 			log.Warn(err)
 		}
 		// After finishing to work on the current vulnerability, we go back to the base branch to start the next vulnerability fix
@@ -148,10 +159,41 @@ func (cfp *CreateFixPullRequestsCmd) fixVulnerablePackages(fixVersionsMap map[st
 	return
 }
 
-func (cfp *CreateFixPullRequestsCmd) fixSinglePackage(impactedPackage string, fixVersionInfo *utils.FixVersionInfo) (err error) {
+func (cfp *CreateFixPullRequestsCmd) fixIssuesSinglePR(fixVersionsMap map[string]*utils.FixVersionInfo) (err error) {
+	successfullyFixedPackages := make(map[string]*utils.FixVersionInfo)
+	log.Info("-----------------------------------------------------------------")
+	log.Info("Start aggregated packages fix")
+	aggregatedFixBranchName, err := cfp.gitManager.GenerateAggregatedFixBranchName(fixVersionsMap)
+	if err != nil {
+		return
+	}
+	if err = cfp.createFixBranch(aggregatedFixBranchName, false); err != nil {
+		return
+	}
+	// Fix all packages in the same branch
+	for impactedPackage, fixVersionInfo := range fixVersionsMap {
+		if err = cfp.updatePackageToFixedVersion(impactedPackage, fixVersionInfo); err != nil {
+			log.Error("Could not fix impacted package", impactedPackage, "as part of the PR. Skipping it. Cause:", err.Error())
+		} else {
+			log.Info("Successfully fixed", impactedPackage)
+			successfullyFixedPackages[impactedPackage] = fixVersionInfo
+		}
+	}
+
+	if err = cfp.openAggregatedPullRequest(aggregatedFixBranchName, successfullyFixedPackages); err != nil {
+		return fmt.Errorf("failed while creating aggreagted pull request. Error: \n%s", err.Error())
+	}
+	return
+}
+
+func (cfp *CreateFixPullRequestsCmd) fixSinglePackageAndCreatePR(impactedPackage string, fixVersionInfo *utils.FixVersionInfo) (err error) {
 	log.Info("-----------------------------------------------------------------")
 	log.Info("Start fixing", impactedPackage, "with", fixVersionInfo.FixVersion)
-	fixBranchName, err := cfp.createFixingBranch(impactedPackage, fixVersionInfo)
+	fixBranchName, err := cfp.gitManager.GenerateFixBranchName(cfp.details.Branch, impactedPackage, fixVersionInfo.FixVersion)
+	if err != nil {
+		return
+	}
+	err = cfp.createFixBranch(fixBranchName, true)
 	if err != nil {
 		return fmt.Errorf("failed while creating new branch: \n%s", err.Error())
 	}
@@ -179,14 +221,12 @@ func (cfp *CreateFixPullRequestsCmd) openFixingPullRequest(impactedPackage, fixB
 
 	commitMessage := cfp.gitManager.GenerateCommitMessage(impactedPackage, fixVersionInfo.FixVersion)
 	log.Info("Running git add all and commit...")
-	err = cfp.gitManager.AddAllAndCommit(commitMessage)
-	if err != nil {
+	if err = cfp.gitManager.AddAllAndCommit(commitMessage); err != nil {
 		return
 	}
 
-	log.Info("Pushing fix branch:", fixBranchName, "...")
-	err = cfp.gitManager.Push()
-	if err != nil {
+	log.Info("Pushing branch:", fixBranchName, "...")
+	if err = cfp.gitManager.Push(false); err != nil {
 		return
 	}
 
@@ -196,22 +236,49 @@ func (cfp *CreateFixPullRequestsCmd) openFixingPullRequest(impactedPackage, fixB
 	return cfp.details.Client.CreatePullRequest(context.Background(), cfp.details.RepoOwner, cfp.details.RepoName, fixBranchName, cfp.details.Branch, pullRequestTitle, prBody)
 }
 
-func (cfp *CreateFixPullRequestsCmd) createFixingBranch(impactedPackage string, fixVersionInfo *utils.FixVersionInfo) (fixBranchName string, err error) {
-	fixBranchName, err = cfp.gitManager.GenerateFixBranchName(cfp.details.Branch, impactedPackage, fixVersionInfo.FixVersion)
+// When aggregate mode is active, there can be only one updated pull request to contain all the available fixes.
+// In case of an already opened pull request, Frogbot will only update the branch.
+func (cfp *CreateFixPullRequestsCmd) openAggregatedPullRequest(fixBranchName string, versionsMap map[string]*utils.FixVersionInfo) (err error) {
+	log.Info("Checking if there are changes to commit")
+	isClean, err := cfp.gitManager.IsClean()
 	if err != nil {
 		return
 	}
+	if isClean {
+		return fmt.Errorf("there were no changes in the fix branch '%s'", fixBranchName)
+	}
+	commitMessage := cfp.gitManager.GenerateAggregatedCommitMessage()
+	log.Info("Running git add all and commit...")
+	if err = cfp.gitManager.AddAllAndCommit(commitMessage); err != nil {
+		return
+	}
+	exists, err := cfp.gitManager.BranchExistsInRemote(fixBranchName)
+	if err != nil {
+		return
+	}
+	log.Info("Pushing branch:", fixBranchName, "...")
+	if err = cfp.gitManager.Push(true); err != nil {
+		return
+	}
+	if !exists {
+		log.Info("Creating Pull Request form:", fixBranchName, " to:", cfp.details.Branch)
+		prBody := commitMessage + "\n\n" + utils.WhatIsFrogbotMd
+		return cfp.details.Client.CreatePullRequest(context.Background(), cfp.details.RepoOwner, cfp.details.RepoName, fixBranchName, cfp.details.Branch, utils.AggregatedPullRequestTitleTemplate, prBody)
+	}
+	log.Info("Pull Request branch:", fixBranchName, "has been updated")
+	return
+}
 
+func (cfp *CreateFixPullRequestsCmd) createFixBranch(fixBranchName string, failOnExists bool) (err error) {
 	exists, err := cfp.gitManager.BranchExistsInRemote(fixBranchName)
 	if err != nil {
 		return
 	}
 	log.Info("Creating branch", fixBranchName, "...")
-	if exists {
-		return "", fmt.Errorf("branch %s already exists in the remote git repository", fixBranchName)
+	if failOnExists && exists {
+		return fmt.Errorf("branch %s already exists in the remote git repository", fixBranchName)
 	}
-
-	return fixBranchName, cfp.gitManager.CreateBranchAndCheckout(fixBranchName)
+	return cfp.gitManager.CreateBranchAndCheckout(fixBranchName)
 }
 
 func (cfp *CreateFixPullRequestsCmd) cloneRepository() (tempWd string, restoreDir func() error, err error) {
@@ -275,21 +342,6 @@ func (cfp *CreateFixPullRequestsCmd) addVulnerabilityToFixVersionsMap(vulnerabil
 	return nil
 }
 
-// getMinimalFixVersion that fixes the current impactedPackage
-// fixVersions array is sorted, so we take the first index, unless it's version is older than what we have now.
-func getMinimalFixVersion(impactedPackageVersion string, fixVersions []string) string {
-	// Trim 'v' prefix in case of Go package
-	currVersionStr := strings.TrimPrefix(impactedPackageVersion, "v")
-	currVersion := version.NewVersion(currVersionStr)
-	for _, fixVersion := range fixVersions {
-		fixVersionCandidate := parseVersionChangeString(fixVersion)
-		if currVersion.Compare(fixVersionCandidate) > 0 {
-			return fixVersionCandidate
-		}
-	}
-	return ""
-}
-
 func (cfp *CreateFixPullRequestsCmd) updatePackageToFixedVersion(impactedPackage string, fixVersionInfo *utils.FixVersionInfo) (err error) {
 	// 'CD' into the relevant working directory
 	if cfp.projectWorkingDir != "" {
@@ -317,6 +369,21 @@ func (cfp *CreateFixPullRequestsCmd) updatePackageToFixedVersion(impactedPackage
 	return packageHandler.UpdateImpactedPackage(impactedPackage, fixVersionInfo)
 }
 
+// getMinimalFixVersion find the minimal version that fixes the current impactedPackage;
+// fixVersions is a sorted array. The function returns the first version in the array, that is larger than impactedPackageVersion.
+func getMinimalFixVersion(impactedPackageVersion string, fixVersions []string) string {
+	// Trim 'v' prefix in case of Go package
+	currVersionStr := strings.TrimPrefix(impactedPackageVersion, "v")
+	currVersion := version.NewVersion(currVersionStr)
+	for _, fixVersion := range fixVersions {
+		fixVersionCandidate := parseVersionChangeString(fixVersion)
+		if currVersion.Compare(fixVersionCandidate) > 0 {
+			return fixVersionCandidate
+		}
+	}
+	return ""
+}
+
 // 1.0         --> 1.0 ≤ x
 // (,1.0]      --> x ≤ 1.0
 // (,1.0)      --> x < 1.0

commands/testdata/config/frogbot-config-test-params.yml

@@ -6,6 +6,7 @@
       branchNameTemplate: "this is my branch ${BRANCH_NAME_HASH}"
       commitMessageTemplate: "custom commit title"
       pullRequestTitleTemplate: "myPullRequests"
+      aggregateFixes: true
     scan:
       includeAllVulnerabilities: true
       failOnSecurityIssues: true

commands/utils/consts.go

@@ -61,10 +61,11 @@ const (
 	WatchesDelimiter             = ","
 
 	//#nosec G101 -- False positive - no hardcoded credentials.
-	GitTokenEnv         = "JF_GIT_TOKEN"
-	GitBaseBranchEnv    = "JF_GIT_BASE_BRANCH"
-	GitPullRequestIDEnv = "JF_GIT_PULL_REQUEST_ID"
-	GitApiEndpointEnv   = "JF_GIT_API_ENDPOINT"
+	GitTokenEnv          = "JF_GIT_TOKEN"
+	GitBaseBranchEnv     = "JF_GIT_BASE_BRANCH"
+	GitPullRequestIDEnv  = "JF_GIT_PULL_REQUEST_ID"
+	GitApiEndpointEnv    = "JF_GIT_API_ENDPOINT"
+	GitAggregateFixesEnv = "JF_GIT_AGGREGATE_FIXES"
 
 	// Comment
 	tableHeader = "\n| SEVERITY | DIRECT DEPENDENCIES | DIRECT DEPENDENCIES VERSIONS | IMPACTED DEPENDENCY NAME | IMPACTED DEPENDENCY VERSION | FIXED VERSIONS | CVE\n" +
@@ -93,7 +94,12 @@ const (
 	BranchHashPlaceHolder = "${BRANCH_NAME_HASH}"
 
 	// Default naming templates
-	BranchNameTemplate       = "frogbot-" + PackagePlaceHolder + "-" + BranchHashPlaceHolder
-	CommitMessageTemplate    = "Upgrade " + PackagePlaceHolder + " to " + FixVersionPlaceHolder
-	PullRequestTitleTemplate = "[🐸 Frogbot] Upgrade " + PackagePlaceHolder + " to " + FixVersionPlaceHolder
+	BranchNameTemplate                 = "frogbot-" + PackagePlaceHolder + "-" + BranchHashPlaceHolder
+	AggregatedBranchNameTemplate       = "frogobt-" + BranchHashPlaceHolder
+	CommitMessageTemplate              = "Upgrade " + PackagePlaceHolder + " to " + FixVersionPlaceHolder
+	AggregatedPullRequestTitleTemplate = "[🐸 Frogbot] Update dependencies versions"
+	PullRequestTitleTemplate           = "[🐸 Frogbot] Update version of " + PackagePlaceHolder + " to " + FixVersionPlaceHolder
+	// Frogbot Git author details showed in commits
+	frogbotAuthorName  = "JFrog-Frogbot"
+	frogbotAuthorEmail = "[email protected]"
 )

commands/utils/git.go

@@ -171,8 +171,8 @@ func (gm *GitManager) commit(commitMessage string) error {
 	}
 	_, err = worktree.Commit(commitMessage, &git.CommitOptions{
 		Author: &object.Signature{
-			Name:  "JFrog-Frogbot",
-			Email: "[email protected]",
+			Name:  frogbotAuthorName,
+			Email: frogbotAuthorEmail,
 			When:  time.Now(),
 		},
 	})
@@ -200,7 +200,7 @@ func (gm *GitManager) BranchExistsInRemote(branchName string) (bool, error) {
 	return false, nil
 }
 
-func (gm *GitManager) Push() error {
+func (gm *GitManager) Push(force bool) error {
 	if gm.dryRun {
 		// On dry run do not push to any remote
 		return nil
@@ -209,6 +209,7 @@ func (gm *GitManager) Push() error {
 	err := gm.repository.Push(&git.PushOptions{
 		RemoteName: gm.remoteName,
 		Auth:       gm.auth,
+		Force:      force,
 	})
 	if err != nil {
 		err = fmt.Errorf("git push failed with error: %s", err.Error())
@@ -238,6 +239,14 @@ func (gm *GitManager) GenerateCommitMessage(impactedPackage string, fixVersion s
 	return formatStringWithPlaceHolders(template, impactedPackage, fixVersion, "", true)
 }
 
+func (gm *GitManager) GenerateAggregatedCommitMessage() string {
+	template := gm.customTemplates.commitMessageTemplate
+	if template == "" {
+		template = AggregatedPullRequestTitleTemplate
+	}
+	return formatStringWithPlaceHolders(template, "", "", "", true)
+}
+
 func formatStringWithPlaceHolders(str, impactedPackage, fixVersion, hash string, allowSpaces bool) string {
 	replacements := []struct {
 		placeholder string
@@ -280,6 +289,19 @@ func (gm *GitManager) GeneratePullRequestTitle(impactedPackage string, version s
 	return formatStringWithPlaceHolders(template, impactedPackage, version, "", true)
 }
 
+// Generates unique branch name constructed by all the vulnerable packages.
+func (gm *GitManager) GenerateAggregatedFixBranchName(versionsMap map[string]*FixVersionInfo) (fixBranchName string, err error) {
+	hash, err := fixVersionsMapToMd5Hash(versionsMap)
+	if err != nil {
+		return
+	}
+	branchFormat := gm.customTemplates.branchNameTemplate
+	if branchFormat == "" {
+		branchFormat = AggregatedBranchNameTemplate
+	}
+	return formatStringWithPlaceHolders(branchFormat, "", "", hash, false), nil
+}
+
 // dryRunClone clones an existing repository from our testdata folder into the destination folder for testing purposes.
 // We should call this function when the current working directory is the repository we want to clone.
 func (gm *GitManager) dryRunClone(destination string) error {

commands/utils/git_test.go

@@ -1,6 +1,7 @@
 package utils
 
 import (
+	"github.com/jfrog/jfrog-cli-core/v2/utils/coreutils"
 	"github.com/stretchr/testify/assert"
 	"testing"
 )
@@ -115,3 +116,74 @@ func TestGitManager_GeneratePullRequestTitle(t *testing.T) {
 		})
 	}
 }
+
+func TestGitManager_GenerateAggregatedFixBranchName(t *testing.T) {
+	tests := []struct {
+		fixVersionMapFirst  map[string]*FixVersionInfo
+		fixVersionMapSecond map[string]*FixVersionInfo
+		gitManager          GitManager
+		equal               bool
+		desc                string
+	}{
+		{
+			fixVersionMapFirst: map[string]*FixVersionInfo{
+				"pkg":  {FixVersion: "1.2.3", PackageType: coreutils.Npm, DirectDependency: false},
+				"pkg2": {FixVersion: "1.5.3", PackageType: coreutils.Npm, DirectDependency: false}},
+			fixVersionMapSecond: map[string]*FixVersionInfo{
+				"pkg":  {FixVersion: "1.2.3", PackageType: coreutils.Npm, DirectDependency: false},
+				"pkg2": {FixVersion: "1.5.3", PackageType: coreutils.Npm, DirectDependency: false}},
+			equal: true, desc: "should be equal",
+			gitManager: GitManager{},
+		},
+		{
+			fixVersionMapFirst: map[string]*FixVersionInfo{
+				"pkg":  {FixVersion: "1.2.3", PackageType: coreutils.Npm, DirectDependency: false},
+				"pkg2": {FixVersion: "1.5.3", PackageType: coreutils.Npm, DirectDependency: false},
+			},
+			fixVersionMapSecond: map[string]*FixVersionInfo{
+				"pkgOther": {FixVersion: "1.2.3", PackageType: coreutils.Npm, DirectDependency: false},
+				"pkg2":     {FixVersion: "1.5.3", PackageType: coreutils.Npm, DirectDependency: false}},
+			equal:      false,
+			desc:       "should not be equal",
+			gitManager: GitManager{},
+		},
+		{
+			fixVersionMapFirst: map[string]*FixVersionInfo{
+				"pkg":  {FixVersion: "1.2.3", PackageType: coreutils.Npm, DirectDependency: false},
+				"pkg2": {FixVersion: "1.5.3", PackageType: coreutils.Npm, DirectDependency: false},
+			},
+			fixVersionMapSecond: map[string]*FixVersionInfo{
+				"pkgOther": {FixVersion: "1.2.3", PackageType: coreutils.Npm, DirectDependency: false},
+				"pkg2":     {FixVersion: "1.5.3", PackageType: coreutils.Npm, DirectDependency: false}},
+			equal:      true,
+			desc:       "should be equal with template",
+			gitManager: GitManager{customTemplates: CustomTemplates{branchNameTemplate: "custom"}},
+		},
+	}
+	for _, test := range tests {
+		t.Run(test.desc, func(t *testing.T) {
+			titleOutput1, err := test.gitManager.GenerateAggregatedFixBranchName(test.fixVersionMapFirst)
+			assert.NoError(t, err)
+			titleOutput2, err := test.gitManager.GenerateAggregatedFixBranchName(test.fixVersionMapSecond)
+			assert.NoError(t, err)
+			equal := titleOutput1 == titleOutput2
+			assert.Equal(t, test.equal, equal)
+		})
+	}
+}
+
+func TestGitManager_GenerateAggregatedCommitMessage(t *testing.T) {
+	tests := []struct {
+		gitManager GitManager
+		expected   string
+	}{
+		{gitManager: GitManager{}, expected: AggregatedPullRequestTitleTemplate},
+		{gitManager: GitManager{customTemplates: CustomTemplates{commitMessageTemplate: "custom_template"}}, expected: "custom_template"},
+	}
+	for _, test := range tests {
+		t.Run(test.expected, func(t *testing.T) {
+			commit := test.gitManager.GenerateAggregatedCommitMessage()
+			assert.Equal(t, commit, test.expected)
+		})
+	}
+}