Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

⚠️ Removing the error field from CheckResult struct #1853

Merged
merged 1 commit into from Apr 22, 2022
Merged

⚠️ Removing the error field from CheckResult struct #1853

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
12 changes: 4 additions & 8 deletions checker/check_result.go
View file
Open in desktop
Expand Up @@ -79,7 +79,6 @@ const (
// nolint:govet
type CheckResult struct {
// TODO(#1393): Remove old structure after deprecation.
Error error `json:"-"`
Name string
Details []string
Confidence int
Expand All @@ -88,7 +87,7 @@ type CheckResult struct {
// UPGRADEv2: New structure. Omitting unchanged Name field
// for simplicity.
Version int `json:"-"` // Default value of 0 indicates old structure.
Error2 error `json:"-"` // Runtime error indicate a filure to run the check.
Error error `json:"-"` // Runtime error indicate a filure to run the check.
Details2 []CheckDetail `json:"-"` // Details of tests and sub-checks
Score int `json:"-"` // {[-1,0...10], -1 = Inconclusive}
Reason string `json:"-"` // A sentence describing the check result (score, etc)
Expand Down Expand Up @@ -161,12 +160,11 @@ func CreateResultWithScore(name, reason string, score int) CheckResult {
return CheckResult{
Name: name,
// Old structure.
Error: nil,
Confidence: MaxResultScore,
Pass: pass,
// New structure.
Version: 2,
Error2: nil,
Error: nil,
Score: score,
Reason: reason,
}
Expand All @@ -186,12 +184,11 @@ func CreateProportionalScoreResult(name, reason string, b, t int) CheckResult {
return CheckResult{
Name: name,
// Old structure.
Error: nil,
Confidence: MaxResultConfidence,
Pass: pass,
// New structure.
Version: 2,
Error2: nil,
Error: nil,
Score: score,
Reason: NormalizeReason(reason, score),
}
Expand Down Expand Up @@ -232,12 +229,11 @@ func CreateRuntimeErrorResult(name string, e error) CheckResult {
return CheckResult{
Name: name,
// Old structure.
Error: e,
Confidence: 0,
Pass: false,
// New structure.
Version: 2,
Error2: e,
Error: e,
Score: InconclusiveResultScore,
Reason: e.Error(), // Note: message already accessible by caller thru `Error`.
}
Expand Down
6 changes: 3 additions & 3 deletions checker/check_runner.go
View file
Open in desktop
Expand Up @@ -77,7 +77,7 @@ func logStats(ctx context.Context, startTime time.Time, result *CheckResult) err
opencensusstats.Record(ctx, stats.CheckRuntimeInSec.M(runTimeInSecs))

if result.Error != nil {
ctx, err := tag.New(ctx, tag.Upsert(stats.ErrorName, sce.GetName(result.Error2)))
ctx, err := tag.New(ctx, tag.Upsert(stats.ErrorName, sce.GetName(result.Error)))
if err != nil {
return sce.WithMessage(sce.ErrScorecardInternal, fmt.Sprintf("tag.New: %v", err))
}
Expand Down Expand Up @@ -109,9 +109,9 @@ func (r *Runner) Run(ctx context.Context, c Check) CheckResult {
checkRequest.Ctx = ctx
checkRequest.Dlogger = l
res = c.Fn(&checkRequest)
if res.Error2 != nil && errors.Is(res.Error2, sce.ErrRepoUnreachable) {
if res.Error != nil && errors.Is(res.Error, sce.ErrRepoUnreachable) {
checkRequest.Dlogger.Warn(&LogMessage{
Text: fmt.Sprintf("%v", res.Error2),
Text: fmt.Sprintf("%v", res.Error),
})
continue
}
Expand Down
2 changes: 1 addition & 1 deletion checks/code_review_test.go
View file
Open in desktop
Expand Up @@ -207,7 +207,7 @@ func TestCodereview(t *testing.T) {
res := CodeReview(&req)

if tt.err != nil {
if res.Error2 == nil {
if res.Error == nil {
t.Errorf("Expected error %v, got nil", tt.err)
}
// return as we don't need to check the rest of the fields.
Expand Down
2 changes: 1 addition & 1 deletion checks/contributors_test.go
View file
Open in desktop
Expand Up @@ -175,7 +175,7 @@ func TestContributors(t *testing.T) {
res := Contributors(&req)

if tt.err != nil {
if res.Error2 == nil {
if res.Error == nil {
t.Errorf("Expected error %v, got nil", tt.err)
}
// return as we don't need to check the rest of the fields.
Expand Down
4 changes: 2 additions & 2 deletions checks/evaluation/binary_artifacts_test.go
View file
Open in desktop
Expand Up @@ -277,8 +277,8 @@ func TestBinaryArtifacts(t *testing.T) {
t.Parallel()
got := BinaryArtifacts(tt.args.name, tt.args.dl, tt.args.r)
if tt.wantErr {
if got.Error2 == nil {
t.Errorf("BinaryArtifacts() error = %v, wantErr %v", got.Error2, tt.wantErr)
if got.Error == nil {
t.Errorf("BinaryArtifacts() error = %v, wantErr %v", got.Error, tt.wantErr)
}
} else {
if got.Score != tt.want.Score {
Expand Down
20 changes: 10 additions & 10 deletions checks/evaluation/dependency_update_tool_test.go
View file
Open in desktop
Expand Up @@ -70,8 +70,8 @@ func TestDependencyUpdateTool(t *testing.T) {
},
},
want: checker.CheckResult{
Score: 0,
Error2: nil,
Score: 0,
Error: nil,
},
err: false,
expected: scut.TestReturn{
Expand Down Expand Up @@ -104,8 +104,8 @@ func TestDependencyUpdateTool(t *testing.T) {
},
},
want: checker.CheckResult{
Score: 10,
Error2: nil,
Score: 10,
Error: nil,
},
expected: scut.TestReturn{
Error: nil,
Expand All @@ -131,8 +131,8 @@ func TestDependencyUpdateTool(t *testing.T) {
},
},
want: checker.CheckResult{
Score: -1,
Error2: nil,
Score: -1,
Error: nil,
},
expected: scut.TestReturn{
Error: sce.ErrScorecardInternal,
Expand All @@ -147,8 +147,8 @@ func TestDependencyUpdateTool(t *testing.T) {
dl: &scut.TestDetailLogger{},
},
want: checker.CheckResult{
Score: -1,
Error2: nil,
Score: -1,
Error: nil,
},
expected: scut.TestReturn{
Error: sce.ErrScorecardInternal,
Expand All @@ -167,8 +167,8 @@ func TestDependencyUpdateTool(t *testing.T) {
if tt.want.Score != got.Score {
t.Errorf("DependencyUpdateTool() got Score = %v, want %v for %v", got.Score, tt.want.Score, tt.name)
}
if tt.err && got.Error2 == nil {
t.Errorf("DependencyUpdateTool() error = %v, want %v for %v", got.Error2, tt.want.Error2, tt.name)
if tt.err && got.Error == nil {
t.Errorf("DependencyUpdateTool() error = %v, want %v for %v", got.Error, tt.want.Error, tt.name)
return
}

Expand Down
4 changes: 2 additions & 2 deletions checks/evaluation/webhooks_test.go
View file
Open in desktop
Expand Up @@ -139,8 +139,8 @@ func TestWebhooks(t *testing.T) {
t.Parallel()
got := Webhooks(tt.args.name, tt.args.dl, tt.args.r)
if tt.wantErr {
if got.Error2 == nil {
t.Errorf("Webhooks() error = %v, wantErr %v", got.Error2, tt.wantErr)
if got.Error == nil {
t.Errorf("Webhooks() error = %v, wantErr %v", got.Error, tt.wantErr)
}
} else {
if got.Score != tt.want.Score {
Expand Down
4 changes: 2 additions & 2 deletions checks/fuzzing_test.go
View file
Open in desktop
Expand Up @@ -295,8 +295,8 @@ func TestFuzzing(t *testing.T) {
}

result := Fuzzing(&req)
if (result.Error2 != nil) != tt.wantErr {
t.Errorf("Fuzzing() error = %v, wantErr %v", result.Error2, tt.wantErr)
if (result.Error != nil) != tt.wantErr {
t.Errorf("Fuzzing() error = %v, wantErr %v", result.Error, tt.wantErr)
return
}

Expand Down
2 changes: 1 addition & 1 deletion checks/maintained_test.go
View file
Open in desktop
Expand Up @@ -345,7 +345,7 @@ func Test_Maintained(t *testing.T) {
res := Maintained(&req)

if tt.err != nil {
if res.Error2 == nil {
if res.Error == nil {
t.Errorf("Expected error %v, got nil", tt.err)
}
// return as we don't need to check the rest of the fields.
Expand Down
40 changes: 20 additions & 20 deletions checks/pinned_dependencies_test.go
View file
Open in desktop
Expand Up @@ -128,8 +128,8 @@ func TestGithubWorkflowPinning(t *testing.T) {

s, e := testIsGitHubActionsWorkflowPinned(p, content, &dl)
actual := checker.CheckResult{
Score: s,
Error2: e,
Score: s,
Error: e,
}
if !scut.ValidateTestReturn(t, tt.name, &tt.expected, &actual, &dl) {
t.Fail()
Expand Down Expand Up @@ -221,8 +221,8 @@ func TestNonGithubWorkflowPinning(t *testing.T) {

s, e := testIsGitHubActionsWorkflowPinned(p, content, &dl)
actual := checker.CheckResult{
Score: s,
Error2: e,
Score: s,
Error: e,
}
if !scut.ValidateTestReturn(t, tt.name, &tt.expected, &actual, &dl) {
t.Fail()
Expand Down Expand Up @@ -268,8 +268,8 @@ func TestGithubWorkflowPkgManagerPinning(t *testing.T) {

s, e := testValidateGitHubWorkflowScriptFreeOfInsecureDownloads(p, content, &dl)
actual := checker.CheckResult{
Score: s,
Error2: e,
Score: s,
Error: e,
}

if !scut.ValidateTestReturn(t, tt.name, &tt.expected, &actual, &dl) {
Expand Down Expand Up @@ -392,8 +392,8 @@ func TestDockerfilePinning(t *testing.T) {
dl := scut.TestDetailLogger{}
s, e := testValidateDockerfileIsPinned(tt.filename, content, &dl)
actual := checker.CheckResult{
Score: s,
Error2: e,
Score: s,
Error: e,
}
if !scut.ValidateTestReturn(t, tt.name, &tt.expected, &actual, &dl) {
t.Fail()
Expand Down Expand Up @@ -781,8 +781,8 @@ func TestDockerfilePinningWihoutHash(t *testing.T) {
dl := scut.TestDetailLogger{}
s, e := testValidateDockerfileIsPinned(tt.filename, content, &dl)
actual := checker.CheckResult{
Score: s,
Error2: e,
Score: s,
Error: e,
}
if !scut.ValidateTestReturn(t, tt.name, &tt.expected, &actual, &dl) {
t.Fail()
Expand Down Expand Up @@ -970,8 +970,8 @@ func TestDockerfileScriptDownload(t *testing.T) {
dl := scut.TestDetailLogger{}
s, e := testValidateDockerfileIsFreeOfInsecureDownloads(tt.filename, content, &dl)
actual := checker.CheckResult{
Score: s,
Error2: e,
Score: s,
Error: e,
}
if !scut.ValidateTestReturn(t, tt.name, &tt.expected, &actual, &dl) {
t.Fail()
Expand Down Expand Up @@ -1013,8 +1013,8 @@ func TestDockerfileScriptDownloadInfo(t *testing.T) {
dl := scut.TestDetailLogger{}
s, e := testValidateDockerfileIsFreeOfInsecureDownloads(tt.filename, content, &dl)
actual := checker.CheckResult{
Score: s,
Error2: e,
Score: s,
Error: e,
}
if !scut.ValidateTestReturn(t, tt.name, &tt.expected, &actual, &dl) {
t.Fail()
Expand Down Expand Up @@ -1123,8 +1123,8 @@ func TestShellScriptDownload(t *testing.T) {
dl := scut.TestDetailLogger{}
s, e := testValidateShellScriptIsFreeOfInsecureDownloads(tt.filename, content, &dl)
actual := checker.CheckResult{
Score: s,
Error2: e,
Score: s,
Error: e,
}
if !scut.ValidateTestReturn(t, tt.name, &tt.expected, &actual, &dl) {
t.Fail()
Expand Down Expand Up @@ -1178,8 +1178,8 @@ func TestShellScriptDownloadPinned(t *testing.T) {
dl := scut.TestDetailLogger{}
s, e := testValidateShellScriptIsFreeOfInsecureDownloads(tt.filename, content, &dl)
actual := checker.CheckResult{
Score: s,
Error2: e,
Score: s,
Error: e,
}
if !scut.ValidateTestReturn(t, tt.name, &tt.expected, &actual, &dl) {
t.Fail()
Expand Down Expand Up @@ -1257,8 +1257,8 @@ func TestGitHubWorflowRunDownload(t *testing.T) {

s, e := testValidateGitHubWorkflowScriptFreeOfInsecureDownloads(p, content, &dl)
actual := checker.CheckResult{
Score: s,
Error2: e,
Score: s,
Error: e,
}
if !scut.ValidateTestReturn(t, tt.name, &tt.expected, &actual, &dl) {
t.Fail()
Expand Down
8 changes: 4 additions & 4 deletions checks/signed_releases_test.go
View file
Open in desktop
Expand Up @@ -359,9 +359,9 @@ func TestSignedRelease(t *testing.T) {
name: "Error getting releases",
err: errors.New("Error getting releases"),
expected: checker.CheckResult{
Pass: false,
Score: -1,
Error2: errors.New("Error getting releases"),
Pass: false,
Score: -1,
Error: errors.New("Error getting releases"),
},
},
}
Expand Down Expand Up @@ -390,7 +390,7 @@ func TestSignedRelease(t *testing.T) {
res := SignedReleases(&req)

if tt.err != nil {
if res.Error2 == nil {
if res.Error == nil {
t.Errorf("Expected error %v, got nil", tt.err)
}
// return as we don't need to check the rest of the fields.
Expand Down
2 changes: 1 addition & 1 deletion checks/webhook_test.go
View file
Open in desktop
Expand Up @@ -123,7 +123,7 @@ func TestWebhooks(t *testing.T) {
}
res := WebHooks(&req)
if tt.err != nil {
if res.Error2 == nil {
if res.Error == nil {
t.Errorf("Expected error %v, got nil", tt.err)
}
// return as we don't need to check the rest of the fields.
Expand Down
4 changes: 2 additions & 2 deletions cron/worker/main.go
View file
Open in desktop
Expand Up @@ -118,10 +118,10 @@ func processRequest(ctx context.Context,
}
for checkIndex := range result.Checks {
check := &result.Checks[checkIndex]
if !errors.Is(check.Error2, sce.ErrScorecardInternal) {
if !errors.Is(check.Error, sce.ErrScorecardInternal) {
continue
}
errorMsg := fmt.Sprintf("check %s has a runtime error: %v", check.Name, check.Error2)
errorMsg := fmt.Sprintf("check %s has a runtime error: %v", check.Name, check.Error)
if !(*ignoreRuntimeErrors) {
// nolint: goerr113
return errors.New(errorMsg)
Expand Down
2 changes: 0 additions & 2 deletions e2e/binary_artifacts_test.go
View file
Open in desktop
Expand Up @@ -86,7 +86,6 @@ var _ = Describe("E2E TEST:"+checks.CheckBinaryArtifacts, func() {
result := checks.BinaryArtifacts(&req)
// UPGRADEv2: to remove.
// Old version.
Expect(result.Error).Should(BeNil())
Expect(result.Pass).Should(BeFalse())
// New version.
Expect(scut.ValidateTestReturn(nil, "binary artifacts", &expected, &result, &dl)).Should(BeTrue())
Expand Down Expand Up @@ -117,7 +116,6 @@ var _ = Describe("E2E TEST:"+checks.CheckBinaryArtifacts, func() {
result := checks.BinaryArtifacts(&req)
// UPGRADEv2: to remove.
// Old version.
Expect(result.Error).Should(BeNil())
Expect(result.Pass).Should(BeFalse())
// New version.
Expect(scut.ValidateTestReturn(nil, "binary artifacts", &expected, &result, &dl)).Should(BeTrue())
Expand Down
5 changes: 1 addition & 4 deletions e2e/branch_protection_test.go
View file
Open in desktop
Expand Up @@ -54,7 +54,6 @@ var _ = Describe("E2E TEST PAT:"+checks.CheckBranchProtection, func() {
result := checks.BranchProtection(&req)
// UPGRADEv2: to remove.
// Old version.
Expect(result.Error).Should(BeNil())
Expect(result.Pass).Should(BeFalse())

// New version.
Expand Down Expand Up @@ -86,7 +85,6 @@ var _ = Describe("E2E TEST PAT:"+checks.CheckBranchProtection, func() {
result := checks.BranchProtection(&req)
// UPGRADEv2: to remove.
// Old version.
Expect(result.Error).Should(BeNil())
Expect(result.Pass).Should(BeFalse())

// New version.
Expand Down Expand Up @@ -118,7 +116,6 @@ var _ = Describe("E2E TEST PAT:"+checks.CheckBranchProtection, func() {
result := checks.BranchProtection(&req)
// UPGRADEv2: to remove.
// Old version.
Expect(result.Error).Should(BeNil())
Expect(result.Pass).Should(BeFalse())

// New version.
Expand Down Expand Up @@ -146,9 +143,9 @@ var _ = Describe("E2E TEST GITHUB_TOKEN:"+checks.CheckBranchProtection, func() {
}

result := checks.BranchProtection(&req)
Expect(result.Error).ShouldNot(BeNil())
// There should be an error with the GITHUB_TOKEN, until it's supported
// byt GitHub.
Expect(result.Error).ShouldNot(BeNil())
Expect(repoClient.Close()).Should(BeNil())
})
})
Expand Down