Skip to content

Commit

Permalink
refactor(branch-protection-check): avoid duplicate funcions and enhan…
Browse files Browse the repository at this point in the history
…ce readability

Made some nice-to-have improvements on project readability,
making it easier easier to  understand how the branch-protection
score is computed. Also unified 8 different functions that were
doing basically the same thing.

Signed-off-by: Diogo Teles Sant'Anna <diogoteles@google.com>
  • Loading branch information
diogoteles08 committed Sep 27, 2023
1 parent 8774320 commit d170f83
Show file tree
Hide file tree
Showing 2 changed files with 39 additions and 83 deletions.
120 changes: 38 additions & 82 deletions checks/evaluation/branch_protection.go
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@ func BranchProtection(name string, dl checker.DetailLogger,
return checker.CreateInconclusiveResult(name, "unable to detect any development/release branches")
}

score, err := computeScore(scores)
score, err := computeFinalScore(scores, dl)
if err != nil {
return checker.CreateRuntimeErrorResult(name, err)
}
Expand All @@ -111,77 +111,37 @@ func BranchProtection(name string, dl checker.DetailLogger,
}
}

func computeNonAdminBasicScore(scores []levelScore) int {
score := 0
for i := range scores {
s := scores[i]
score += s.scores.basic
}
return score
}

func computeNonAdminReviewScore(scores []levelScore) int {
score := 0
for i := range scores {
s := scores[i]
score += s.scores.review
}
return score
}

func computeAdminReviewScore(scores []levelScore) int {
score := 0
for i := range scores {
s := scores[i]
score += s.scores.adminReview
}
return score
}

func computeNonAdminThoroughReviewScore(scores []levelScore) int {
score := 0
for i := range scores {
s := scores[i]
score += s.scores.thoroughReview
}
return score
}

func computeAdminThoroughReviewScore(scores []levelScore) int {
score := 0
for i := range scores {
s := scores[i]
score += s.scores.adminThoroughReview
}
return score
}

func computeNonAdminContextScore(scores []levelScore) int {
score := 0
for i := range scores {
s := scores[i]
score += s.scores.context
}
return score
}

func computeCodeownerThoroughReviewScore(scores []levelScore) int {
score := 0
for i := range scores {
s := scores[i]
score += s.scores.codeownerReview
func sumUpScoreForTier(tier int, scoresData []levelScore, dl checker.DetailLogger) int {
sum := 0
for i := range scoresData {
score := scoresData[i]
switch tier {
case 1:
sum += score.scores.basic
case 2:
sum += score.scores.review + score.scores.adminReview
case 3:
sum += score.scores.context
case 4:
sum += score.scores.thoroughReview + score.scores.codeownerReview
case 5:
sum += score.scores.adminThoroughReview
default:
debug(dl, true, "Function sumUpScoreForTier called with the invalid parameter: '%d';"+
"BranchProtection score won't be accurate.", tier)

Check warning on line 131 in checks/evaluation/branch_protection.go

View check run for this annotation

Codecov / codecov/patch

checks/evaluation/branch_protection.go#L127-L131

Added lines #L127 - L131 were not covered by tests
}
}
return score
return sum
}

func noarmalizeScore(score, max, level int) float64 {
func normalizeScore(score, max, level int) float64 {
if max == 0 {
return float64(level)
}
return float64(score*level) / float64(max)
}

func computeScore(scores []levelScore) (int, error) {
func computeFinalScore(scores []levelScore, dl checker.DetailLogger) (int, error) {
if len(scores) == 0 {
return 0, sce.WithMessage(sce.ErrScorecardInternal, "scores are empty")
}
Expand All @@ -191,49 +151,45 @@ func computeScore(scores []levelScore) (int, error) {

// First, check if they all pass the basic (admin and non-admin) checks.
maxBasicScore := maxScore.basic * len(scores)
basicScore := computeNonAdminBasicScore(scores)
score += noarmalizeScore(basicScore, maxBasicScore, basicLevel)
if basicScore != maxBasicScore {
basicScore := sumUpScoreForTier(1, scores, dl)
score += normalizeScore(basicScore, maxBasicScore, basicLevel)
if basicScore < maxBasicScore {
return int(score), nil
}

// Second, check the (admin and non-admin) reviews.
maxReviewScore := maxScore.review * len(scores)
maxAdminReviewScore := maxScore.adminReview * len(scores)
reviewScore := computeNonAdminReviewScore(scores)
adminReviewScore := computeAdminReviewScore(scores)
score += noarmalizeScore(reviewScore+adminReviewScore, maxReviewScore+maxAdminReviewScore, adminNonAdminReviewLevel)
if reviewScore != maxReviewScore ||
adminReviewScore != maxAdminReviewScore {
adminNonAdminReviewScore := sumUpScoreForTier(2, scores, dl)
score += normalizeScore(adminNonAdminReviewScore, maxReviewScore+maxAdminReviewScore, adminNonAdminReviewLevel)
if adminNonAdminReviewScore < maxReviewScore+maxAdminReviewScore {
return int(score), nil
}

// Third, check the use of non-admin context.
maxContextScore := maxScore.context * len(scores)
contextScore := computeNonAdminContextScore(scores)
score += noarmalizeScore(contextScore, maxContextScore, nonAdminContextLevel)
if contextScore != maxContextScore {
contextScore := sumUpScoreForTier(3, scores, dl)
score += normalizeScore(contextScore, maxContextScore, nonAdminContextLevel)
if contextScore < maxContextScore {
return int(score), nil
}

// Fourth, check the thorough non-admin reviews.
// Also check whether this repo requires codeowner review
maxThoroughReviewScore := maxScore.thoroughReview * len(scores)
maxCodeownerReviewScore := maxScore.codeownerReview * len(scores)
thoroughReviewScore := computeNonAdminThoroughReviewScore(scores)
codeownerReviewScore := computeCodeownerThoroughReviewScore(scores)
score += noarmalizeScore(thoroughReviewScore+codeownerReviewScore, maxThoroughReviewScore+maxCodeownerReviewScore,
nonAdminThoroughReviewLevel)
if thoroughReviewScore != maxThoroughReviewScore {
tier4Score := sumUpScoreForTier(4, scores, dl)
score += normalizeScore(tier4Score, maxThoroughReviewScore+maxCodeownerReviewScore, nonAdminThoroughReviewLevel)
if tier4Score < maxThoroughReviewScore+maxCodeownerReviewScore {
return int(score), nil
}

// Lastly, check the thorough admin review config.
// This one is controversial and has usability issues
// https://github.com/ossf/scorecard/issues/1027, so we may remove it.
maxAdminThoroughReviewScore := maxScore.adminThoroughReview * len(scores)
adminThoroughReviewScore := computeAdminThoroughReviewScore(scores)
score += noarmalizeScore(adminThoroughReviewScore, maxAdminThoroughReviewScore, adminThoroughReviewLevel)
adminThoroughReviewScore := sumUpScoreForTier(5, scores, dl)
score += normalizeScore(adminThoroughReviewScore, maxAdminThoroughReviewScore, adminThoroughReviewLevel)

Check warning on line 192 in checks/evaluation/branch_protection.go

View check run for this annotation

Codecov / codecov/patch

checks/evaluation/branch_protection.go#L191-L192

Added lines #L191 - L192 were not covered by tests
if adminThoroughReviewScore != maxAdminThoroughReviewScore {
return int(score), nil
}
Expand Down Expand Up @@ -450,7 +406,7 @@ func nonAdminThoroughReviewProtection(branch *clients.BranchRef, dl checker.Deta
warn(dl, log, "number of required reviewers is only %d on branch '%s'",
*branch.BranchProtectionRule.RequiredPullRequestReviews.RequiredApprovingReviewCount, *branch.Name)
}
}
}

return score, max
}
Expand Down
2 changes: 1 addition & 1 deletion checks/evaluation/branch_protection_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ func testScore(branch *clients.BranchRef, codeownersFiles []string, dl checker.D
score.scores.adminThoroughReview, score.maxes.adminThoroughReview = adminThoroughReviewProtection(branch, dl)
score.scores.codeownerReview, score.maxes.codeownerReview = codeownerBranchProtection(branch, codeownersFiles, dl)

return computeScore([]levelScore{score})
return computeFinalScore([]levelScore{score}, dl)
}

func TestIsBranchProtected(t *testing.T) {
Expand Down

0 comments on commit d170f83

Please sign in to comment.