Bring back negative outcome in case of 0 codeowners files

Signed-off-by: Adam Korczynski <adam@adalogics.com>
ossf · Feb 24, 2024 · 75c18eb · 75c18eb
1 parent 5c98388
commit 75c18eb
Show file tree

Hide file tree

Showing 5 changed files with 17 additions and 19 deletions.
diff --git a/checks/branch_protection_test.go b/checks/branch_protection_test.go
@@ -174,7 +174,7 @@ func TestReleaseAndDevBranchProtected(t *testing.T) {
 				Error:         nil,
 				Score:         4,
 				NumberOfWarn:  9,
-				NumberOfInfo:  12,
+				NumberOfInfo:  11,
 				NumberOfDebug: 0,
 			},
 			defaultBranch: main,
@@ -232,7 +232,7 @@ func TestReleaseAndDevBranchProtected(t *testing.T) {
 				Error:         nil,
 				Score:         8,
 				NumberOfWarn:  4,
-				NumberOfInfo:  18,
+				NumberOfInfo:  16,
 				NumberOfDebug: 0,
 			},
 			defaultBranch: main,

diff --git a/checks/evaluation/branch_protection.go b/checks/evaluation/branch_protection.go
@@ -222,7 +222,7 @@ func isBranchProtected(findings []finding.Finding, branchName string) (bool, err
 
 func getBranchName(f *finding.Finding) (string, error) {
 	for k := range f.Values {
-		if k == "branchProtected" || k == "numberOfRequiredReviewers" || k == "codeownersFiles" {
+		if k == "branchProtected" || k == "numberOfRequiredReviewers" {
 			continue
 		}
 		return k, nil
@@ -449,11 +449,7 @@ func codeownerBranchProtection(f *finding.Finding, dl checker.DetailLogger) (int
 	var score, max int
 	if f.Outcome == finding.OutcomePositive {
 		info(dl, true, f.Message)
-		if f.Values["codeownersFiles"] == 0 {
-			warn(dl, true, f.Message)
-		} else {
-			score++
-		}
+		score++
 	} else {
 		warn(dl, true, f.Message)
 	}

diff --git a/checks/evaluation/branch_protection_test.go b/checks/evaluation/branch_protection_test.go
@@ -555,7 +555,7 @@ func TestBranchProtection(t *testing.T) {
 			},
 			result: scut.TestReturn{
 				Score:        4,
-				NumberOfWarn: 2,
+				NumberOfWarn: 1,
 				NumberOfInfo: 9,
 			},
 		},
@@ -1261,7 +1261,7 @@ func TestBranchProtection(t *testing.T) {
 			},
 			result: scut.TestReturn{
 				Score:        8,
-				NumberOfWarn: 2,
+				NumberOfWarn: 1,
 				NumberOfInfo: 9,
 			},
 		},
@@ -1315,8 +1315,7 @@ func TestBranchProtection(t *testing.T) {
 					Probe:   "requiresCodeOwnersReview",
 					Outcome: finding.OutcomePositive,
 					Values: map[string]int{
-						"main":            1,
-						"codeownersFiles": 1,
+						"main": 1,
 					},
 				},
 				{
@@ -1438,7 +1437,7 @@ func TestBranchProtection(t *testing.T) {
 			},
 			result: scut.TestReturn{
 				Score:        5,
-				NumberOfWarn: 3,
+				NumberOfWarn: 2,
 				NumberOfInfo: 8,
 			},
 		},

diff --git a/probes/requiresCodeOwnersReview/impl.go b/probes/requiresCodeOwnersReview/impl.go
@@ -63,6 +63,9 @@ func Run(raw *checker.RawResults) ([]finding.Finding, string, error) {
 		case !*reqOwnerReviews:
 			text = fmt.Sprintf("codeowners review is not required on branch '%s'", *branch.Name)
 			outcome = finding.OutcomeNegative
+		case len(r.CodeownersFiles) == 0:
+			text = "codeowners review is required - but no codeowners file found in repo"
+			outcome = finding.OutcomeNegative
 		default:
 			text = fmt.Sprintf("codeowner review is required on branch '%s'", *branch.Name)
 			outcome = finding.OutcomePositive

diff --git a/probes/requiresCodeOwnersReview/impl_test.go b/probes/requiresCodeOwnersReview/impl_test.go
@@ -79,7 +79,7 @@ func Test_Run(t *testing.T) {
 				},
 			},
 			outcomes: []finding.Outcome{
-				finding.OutcomePositive,
+				finding.OutcomeNegative,
 			},
 		},
 		{
@@ -104,7 +104,7 @@ func Test_Run(t *testing.T) {
 							},
 						},
 					},
-					CodeownersFiles: []string{"file"},
+					CodeownersFiles: []string{"file1"},
 				},
 			},
 			outcomes: []finding.Outcome{
@@ -133,7 +133,7 @@ func Test_Run(t *testing.T) {
 							},
 						},
 					},
-					CodeownersFiles: []string{},
+					CodeownersFiles: []string{"file1", "file2"},
 				},
 			},
 			outcomes: []finding.Outcome{
@@ -191,7 +191,7 @@ func Test_Run(t *testing.T) {
 							},
 						},
 					},
-					CodeownersFiles: []string{},
+					CodeownersFiles: []string{"file"},
 				},
 			},
 			outcomes: []finding.Outcome{
@@ -228,7 +228,7 @@ func Test_Run(t *testing.T) {
 			},
 		},
 		{
-			name: "Requires code owner reviews on 1/2 branches - without files = 1 negative and 1 positive",
+			name: "Requires code owner reviews on 1/2 branches - without files = 2 negative",
 			raw: &checker.RawResults{
 				BranchProtectionResults: checker.BranchProtectionsData{
 					Branches: []clients.BranchRef{
@@ -253,7 +253,7 @@ func Test_Run(t *testing.T) {
 				},
 			},
 			outcomes: []finding.Outcome{
-				finding.OutcomeNegative, finding.OutcomePositive,
+				finding.OutcomeNegative, finding.OutcomeNegative,
 			},
 		},
 		{