Skip to content

Commit

Permalink
Refactor comments
Browse files Browse the repository at this point in the history
  • Loading branch information
EyalDelarea committed Jun 27, 2023
1 parent 5fe65be commit 07591cf
Show file tree
Hide file tree
Showing 2 changed files with 26 additions and 23 deletions.
39 changes: 21 additions & 18 deletions commands/createfixpullrequests.go
Original file line number Diff line number Diff line change
Expand Up @@ -164,11 +164,11 @@ func (cfp *CreateFixPullRequestsCmd) fixIssuesSeparatePRs(vulnerabilitiesMap map
return
}

// Fix all the vulnerabilities in one aggregated pull request.
// In case of existing aggregated fix, check for different scan results
// if scan results are the same, do nothing.
// Else, force push to the same branch and reopen PR in case closed.
// Only one aggregated pull request should be open at all times.
// fixIssuesSinglePR fixes all the vulnerabilities in a single aggregated pull request.
// If an existing aggregated fix is present, it checks for different scan results.
// If the scan results are the same, no action is taken.
// Otherwise, it performs a force push to the same branch and reopens the pull request if it was closed.
// Only one aggregated pull request should remain open at all times.
func (cfp *CreateFixPullRequestsCmd) fixIssuesSinglePR(vulnerabilityDetails map[string]*utils.VulnerabilityDetails) (err error) {
aggregatedFixBranchName, err := cfp.gitManager.GenerateAggregatedFixBranchName()
if err != nil {
Expand Down Expand Up @@ -258,23 +258,26 @@ func (cfp *CreateFixPullRequestsCmd) openFixingPullRequest(fixBranchName string,
return cfp.details.Client().CreatePullRequest(context.Background(), cfp.details.RepoOwner, cfp.details.RepoName, fixBranchName, cfp.details.Branch(), pullRequestTitle, prBody)
}

// 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 and the pull request body.
// NOTE:
// It is unnecessary to check for a clean git state
// as we will always want to force push when we have different scan results.
// There are some exceptional scenarios where the git will be clean, but the scan results changed.
// In such cases, we need to update the pull request body with the updated information.
// openAggregatedPullRequest handles the opening or updating of a pull request when the aggregate mode is active.
// If a pull request is already open, Frogbot will update the branch and the pull request body.
func (cfp *CreateFixPullRequestsCmd) openAggregatedPullRequest(fixBranchName string, existingPullRequestId int64, vulnerabilities []formats.VulnerabilityOrViolationRow) (err error) {
commitMessage := cfp.gitManager.GenerateAggregatedCommitMessage()
log.Debug("Running git add all and commit...")
if err = cfp.gitManager.AddAllAndCommit(commitMessage); err != nil {
log.Debug("Checking if there are changes to commit")
isClean, err := cfp.gitManager.IsClean()
if err != nil {
return
}
log.Debug("Pushing branch:", fixBranchName, "...")
if err = cfp.gitManager.Push(true, fixBranchName); err != nil {
return
if !isClean {
commitMessage := cfp.gitManager.GenerateAggregatedCommitMessage()
log.Debug("Running git add all and commit...")
if err = cfp.gitManager.AddAllAndCommit(commitMessage); err != nil {
return
}
log.Debug("Pushing branch:", fixBranchName, "...")
if err = cfp.gitManager.Push(true, fixBranchName); err != nil {
return
}
}
// Even if the git state is clean, there are cases where we still need to update the pull request body.
prBody := cfp.OutputWriter.VulnerabiltiesTitle(false) + "\n" + cfp.OutputWriter.VulnerabilitiesContent(vulnerabilities)
pullRequestTitle := utils.AggregatedPullRequestTitleTemplate
if existingPullRequestId == PullRequestNotFound {
Expand Down
10 changes: 5 additions & 5 deletions commands/utils/git.go
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,8 @@ const (
bitbucketServerHttpsFormat = "%s/scm/%s/%s.git"
azureDevopsHttpsFormat = "https://%s@%s%s/_git/%s"

// Aggregate branches should always be the same name.
// Use a const to replace in the branch template ${BRANCH_NAME_HASH}
// Aggregate branches name should always be the same name.
// We use a const to replace in the branch template ${BRANCH_NAME_HASH}
constAggregatedHash = "0"
)

Expand Down Expand Up @@ -324,8 +324,8 @@ func (gm *GitManager) GeneratePullRequestTitle(impactedPackage string, version s
return formatStringWithPlaceHolders(template, impactedPackage, version, "", true)
}

// Generates consistent branch name to allow branch updates
// and to make sure there is only one Frogbot branch on aggreagated mode.
// GenerateAggregatedFixBranchName Generating a consistent branch name to enable branch updates
// and to ensure that there is only one Frogbot branch in aggregated mode.
func (gm *GitManager) GenerateAggregatedFixBranchName() (fixBranchName string, err error) {
branchFormat := gm.customTemplates.branchNameTemplate
if branchFormat == "" {
Expand Down Expand Up @@ -377,7 +377,7 @@ func (gm *GitManager) generateHTTPSCloneUrl() (url string, err error) {
func (gm *GitManager) CheckoutRemoteBranch(branchName string) error {
var checkoutConfig *git.CheckoutOptions
if gm.dryRun {
// On dry runs we mimic remote locally
// On dry runs we mimic remote as local branches.
checkoutConfig = &git.CheckoutOptions{
Branch: plumbing.NewBranchReferenceName(branchName),
Force: true,
Expand Down

0 comments on commit 07591cf

Please sign in to comment.