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 <[email protected]>
  • Loading branch information
@diogoteles08
diogoteles08 committed Sep 20, 2023
1 parent b181f6e commit 5f53749
Show file tree
Hide file tree
Showing 2 changed files with 37 additions and 94 deletions.
129 changes: 36 additions & 93 deletions checks/evaluation/branch_protection.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,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 @@ -112,76 +112,25 @@ 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 computeAdminBasicScore(scores []levelScore) int {
score := 0
for i := range scores {
s := scores[i]
score += s.scores.adminBasic
}
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, scores []levelScore, dl checker.DetailLogger) int {
sum := 0
for _, score := range scores {

Check failure on line 117 in checks/evaluation/branch_protection.go

View workflow job for this annotation

GitHub Actions / check-linter 

rangeValCopy: each iteration copies 128 bytes (consider pointers or indexing) (gocritic)
switch tier {
case 1:
sum += score.scores.basic + score.scores.adminBasic
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 failure on line 130 in checks/evaluation/branch_protection.go

View workflow job for this annotation

GitHub Actions / check-linter 

line is 139 characters (lll)
}
}
return score
return sum
}

func normalizeScore(score, max, level int) float64 {
Expand All @@ -191,7 +140,7 @@ func normalizeScore(score, max, level int) float64 {
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 @@ -202,50 +151,44 @@ func computeScore(scores []levelScore) (int, error) {
// First, check if they all pass the basic (admin and non-admin) checks.
maxBasicScore := maxScore.basic * len(scores)
maxAdminBasicScore := maxScore.adminBasic * len(scores)
basicScore := computeNonAdminBasicScore(scores)
adminBasicScore := computeAdminBasicScore(scores)
score += normalizeScore(basicScore+adminBasicScore, maxBasicScore+maxAdminBasicScore, adminNonAdminBasicLevel)
if basicScore != maxBasicScore ||
adminBasicScore != maxAdminBasicScore {
adminNonAdminBasicScore := sumUpScoreForTier(1, scores, dl)
score += normalizeScore(adminNonAdminBasicScore, maxBasicScore+maxAdminBasicScore, adminNonAdminBasicLevel)
if adminNonAdminBasicScore < maxBasicScore+maxAdminBasicScore {
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 += normalizeScore(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)
contextScore := sumUpScoreForTier(3, scores, dl)
score += normalizeScore(contextScore, maxContextScore, nonAdminContextLevel)
if contextScore != maxContextScore {
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 += normalizeScore(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)
adminThoroughReviewScore := sumUpScoreForTier(5, scores, dl)
score += normalizeScore(adminThoroughReviewScore, maxAdminThoroughReviewScore, adminThoroughReviewLevel)
if adminThoroughReviewScore != maxAdminThoroughReviewScore {
return int(score), nil
Expand Down Expand Up @@ -459,14 +402,14 @@ func nonAdminThoroughReviewProtection(branch *clients.BranchRef, dl checker.Deta
*branch.BranchProtectionRule.RequiredPullRequestReviews.RequiredApprovingReviewCount == 0 {
warn(dl, log, "number of required reviewers is 0 on branch '%s'", *branch.Name)
} else if *branch.BranchProtectionRule.RequiredPullRequestReviews.RequiredApprovingReviewCount >= minReviews {
info(dl, log, "number of required reviewers is %d on branch '%s'",
*branch.BranchProtectionRule.RequiredPullRequestReviews.RequiredApprovingReviewCount, *branch.Name)
score++
info(dl, log, "number of required reviewers is %d on branch '%s'",
*branch.BranchProtectionRule.RequiredPullRequestReviews.RequiredApprovingReviewCount, *branch.Name)
score++
} else {
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
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,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 5f53749

Please sign in to comment.