diff --git a/checks/evaluation/branch_protection.go b/checks/evaluation/branch_protection.go index 6f18f833cf20..a1883fd48d30 100644 --- a/checks/evaluation/branch_protection.go +++ b/checks/evaluation/branch_protection.go @@ -95,7 +95,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) } @@ -113,86 +113,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 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, scoresData []levelScore, dl checker.DetailLogger) int { + sum := 0 + for i := range scoresData { + score := scoresData[i] + 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) + } } - 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") } @@ -203,30 +154,26 @@ 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 += noarmalizeScore(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 += 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 } @@ -234,11 +181,9 @@ func computeScore(scores []levelScore) (int, error) { // 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 } @@ -246,8 +191,8 @@ func computeScore(scores []levelScore) (int, error) { // 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) if adminThoroughReviewScore != maxAdminThoroughReviewScore { return int(score), nil } @@ -472,7 +417,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 } diff --git a/checks/evaluation/branch_protection_test.go b/checks/evaluation/branch_protection_test.go index 362e4596a880..2c548e7942a8 100644 --- a/checks/evaluation/branch_protection_test.go +++ b/checks/evaluation/branch_protection_test.go @@ -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) {