🐛 fix and result of multicheck (#571)

* fix multicheckand Signed-off-by: Asra Ali <[email protected]> * address comments Signed-off-by: Asra Ali <[email protected]>
ossf · Jun 14, 2021 · b7ca0d9 · b7ca0d9
1 parent a5b53c6
commit b7ca0d9
Show file tree

Hide file tree

Showing 6 changed files with 201 additions and 24 deletions.
diff --git a/checker/check_result.go b/checker/check_result.go
@@ -92,3 +92,35 @@ func MakeProportionalResult(name string, numerator int, denominator int,
 		Confidence: MaxResultConfidence - int(actual*MaxResultConfidence),
 	}
 }
+
+// Given a min result, check if another result is worse.
+func isMinResult(result, min CheckResult) bool {
+	if Bool2int(result.Pass) < Bool2int(min.Pass) {
+		return true
+	}
+	if result.Pass && result.Confidence < min.Confidence {
+		return true
+	} else if !result.Pass && result.Confidence > min.Confidence {
+		return true
+	}
+	return false
+}
+
+// MakeAndResult means all checks must succeed. This returns a conservative result
+// where the worst result is returned.
+func MakeAndResult(checks ...CheckResult) CheckResult {
+	minResult := CheckResult{
+		Pass:       true,
+		Confidence: MaxResultConfidence,
+	}
+
+	for _, result := range checks {
+		if minResult.Name == "" {
+			minResult.Name = result.Name
+		}
+		if isMinResult(result, minResult) {
+			minResult = result
+		}
+	}
+	return minResult
+}
diff --git a/checker/check_runner.go b/checker/check_runner.go
@@ -108,21 +108,11 @@ func MultiCheckOr(fns ...CheckFn) CheckFn {
 // where the worst result is returned.
 func MultiCheckAnd(fns ...CheckFn) CheckFn {
 	return func(c *CheckRequest) CheckResult {
-		minResult := CheckResult{
-			Pass:       true,
-			Confidence: MaxResultConfidence,
-		}
-
+		var checks []CheckResult
 		for _, fn := range fns {
-			result := fn(c)
-			if minResult.Name == "" {
-				minResult.Name = result.Name
-			}
-			if Bool2int(result.Pass) < Bool2int(minResult.Pass) ||
-				result.Confidence < MaxResultConfidence {
-				minResult = result
-			}
+			res := fn(c)
+			checks = append(checks, res)
 		}
-		return minResult
+		return MakeAndResult(checks...)
 	}
 }
diff --git a/checker/check_test.go b/checker/check_test.go
@@ -0,0 +1,151 @@
+// Copyright 2021 Security Scorecard Authors
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//      https://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package checker
+
+import (
+	"errors"
+	"testing"
+)
+
+const checkTest = "Check-Test"
+
+var errorTest = errors.New("test error")
+
+func TestMakeCheckAnd(t *testing.T) {
+	t.Parallel()
+	tests := []struct {
+		name   string
+		checks []CheckResult
+		want   CheckResult
+	}{
+		{
+			name: "Multiple passing",
+			checks: []CheckResult{
+				{
+					Name:        checkTest,
+					Pass:        true,
+					Details:     nil,
+					Confidence:  5,
+					ShouldRetry: false,
+					Error:       errorTest,
+				},
+				{
+					Name:        checkTest,
+					Pass:        true,
+					Details:     nil,
+					Confidence:  10,
+					ShouldRetry: false,
+					Error:       errorTest,
+				},
+			},
+			want: CheckResult{
+				Name:        checkTest,
+				Pass:        true,
+				Details:     nil,
+				Confidence:  5,
+				ShouldRetry: false,
+				Error:       nil,
+			},
+		},
+		{
+			name: "Multiple failing",
+			checks: []CheckResult{
+				{
+					Name:        checkTest,
+					Pass:        false,
+					Details:     nil,
+					Confidence:  10,
+					ShouldRetry: false,
+					Error:       errorTest,
+				},
+				{
+					Name:        checkTest,
+					Pass:        false,
+					Details:     nil,
+					Confidence:  5,
+					ShouldRetry: false,
+					Error:       errorTest,
+				},
+			},
+			want: CheckResult{
+				Name:        checkTest,
+				Pass:        false,
+				Details:     nil,
+				Confidence:  10,
+				ShouldRetry: false,
+				Error:       errorTest,
+			},
+		},
+		{
+			name: "Passing and failing",
+			checks: []CheckResult{
+				{
+					Name:        checkTest,
+					Pass:        true,
+					Details:     nil,
+					Confidence:  10,
+					ShouldRetry: false,
+					Error:       nil,
+				},
+				{
+					Name:        checkTest,
+					Pass:        false,
+					Details:     nil,
+					Confidence:  5,
+					ShouldRetry: false,
+					Error:       errorTest,
+				},
+				{
+					Name:        checkTest,
+					Pass:        false,
+					Details:     nil,
+					Confidence:  10,
+					ShouldRetry: false,
+					Error:       errorTest,
+				},
+			},
+			want: CheckResult{
+				Name:        checkTest,
+				Pass:        false,
+				Details:     nil,
+				Confidence:  10,
+				ShouldRetry: false,
+				Error:       errorTest,
+			},
+		},
+	}
+	for _, tt := range tests {
+		tt := tt
+		t.Run(tt.name, func(t *testing.T) {
+			t.Parallel()
+			result := MakeAndResult(tt.checks...)
+			if result.Pass != tt.want.Pass || result.Confidence != tt.want.Confidence {
+				t.Errorf("MakeAndResult failed (%s): got %v, expected %v", tt.name, result, tt.want)
+			}
+
+			// Also test CheckFn variant
+			var fns []CheckFn
+			for _, c := range tt.checks {
+				check := c
+				fns = append(fns, func(*CheckRequest) CheckResult { return check })
+			}
+			c := CheckRequest{}
+			resultfn := MultiCheckAnd(fns...)(&c)
+			if resultfn.Pass != tt.want.Pass || resultfn.Confidence != tt.want.Confidence {
+				t.Errorf("MultiCheckAnd failed (%s): got %v, expected %v", tt.name, resultfn, tt.want)
+			}
+		})
+	}
+}
diff --git a/cron/data/request.pb.go b/cron/data/request.pb.go
diff --git a/gitcache/pkg/service.go b/gitcache/pkg/service.go
@@ -183,7 +183,7 @@ func archiveFolder(folderToArchive, archivePath string) ([]byte, error) {
 
 // fetchGitRepo fetches the git repo. Returns git repository, bool if it is already up to date and error.
 func fetchGitRepo(storagePath *StoragePath, data []byte, repo RepoURL, tempDir string) (*git.Repository, bool, error) {
-	const fileMode os.FileMode = 0600
+	const fileMode os.FileMode = 0o600
 	gitZipFile := path.Join(tempDir, "gitfolder.tar.gz")
 	if err := ioutil.WriteFile(gitZipFile, data, fileMode); err != nil {
 		return nil, false, errors.Wrapf(err, "unable write targz file %s", storagePath.BlobArchiveFile)

diff --git a/gitcache/pkg/storagepath.go b/gitcache/pkg/storagepath.go
@@ -39,14 +39,14 @@ func NewStoragePath(repo RepoURL, tempDir string) (StoragePath, error) {
 	bucketPath := fmt.Sprintf("gitcache/%s/%s/%s", repo.Host, repo.Owner, repo.Repo)
 	gitDir := path.Join(tempDir, repo.NonURLString())
 
-	err := os.Mkdir(gitDir, 0755)
+	err := os.Mkdir(gitDir, 0o755)
 	if err != nil {
 		return StoragePath{}, errors.Wrapf(err, "unable to temp directory %s", gitDir)
 	}
 	gitTarPath := path.Join(gitDir, "gitfolder.tar.gz")
 
 	blobArchiveDir := gitDir + "tar"
-	err = os.Mkdir(blobArchiveDir, 0755)
+	err = os.Mkdir(blobArchiveDir, 0o755)
 	if err != nil {
 		return StoragePath{}, errors.Wrapf(err, "unable to create temp directory for blob archive %s", blobArchiveDir)
 	}