From 4b17d60d63e34372a73d5ad5a9b3149a111387da Mon Sep 17 00:00:00 2001 From: Diogo Teles Sant'Anna Date: Wed, 20 Sep 2023 14:43:38 +0000 Subject: [PATCH] refactor(branch-protection-check): avoid duplicate funcions and enhance 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 --- checks/evaluation/branch_protection.go | 129 ++++++-------------- checks/evaluation/branch_protection_test.go | 2 +- 2 files changed, 37 insertions(+), 94 deletions(-) diff --git a/checks/evaluation/branch_protection.go b/checks/evaluation/branch_protection.go index 45bc3286296d..f9c8801f3921 100644 --- a/checks/evaluation/branch_protection.go +++ b/checks/evaluation/branch_protection.go @@ -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) } @@ -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 { + 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 normalizeScore(score, max, level int) float64 { @@ -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") } @@ -202,30 +151,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 += 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 } @@ -233,11 +178,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 += 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 } @@ -245,7 +188,7 @@ 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) + adminThoroughReviewScore := sumUpScoreForTier(5, scores, dl) score += normalizeScore(adminThoroughReviewScore, maxAdminThoroughReviewScore, adminThoroughReviewLevel) if adminThoroughReviewScore != maxAdminThoroughReviewScore { return int(score), nil @@ -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 } diff --git a/checks/evaluation/branch_protection_test.go b/checks/evaluation/branch_protection_test.go index 362e4596a880..0188a2decf1a 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}) } func TestIsBranchProtected(t *testing.T) {