From 8c04788f385a15ec21a104bdfe5f07a1ff6167a7 Mon Sep 17 00:00:00 2001 From: Aiden Wang <54022336+aidenwang9867@users.noreply.github.com> Date: Wed, 3 Aug 2022 15:54:26 -0700 Subject: [PATCH] =?UTF-8?q?=E2=9C=A8=20Enhancement:=20Dependency-diff=20AP?= =?UTF-8?q?I=20optimization=20-=20changing=20the=20input=20param=20changeT?= =?UTF-8?q?ype=20from=20a=20map=20to=20an=20array=20(#2111)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * save * save * save * save * save * save * save --- dependencydiff/dependencydiff.go | 34 ++++++++++++++---- dependencydiff/dependencydiff_test.go | 51 +++++++++++++++++++++++++++ e2e/dependencydiff_test.go | 13 ++++--- 3 files changed, 84 insertions(+), 14 deletions(-) diff --git a/dependencydiff/dependencydiff.go b/dependencydiff/dependencydiff.go index 37aa419b..76a8cb11 100644 --- a/dependencydiff/dependencydiff.go +++ b/dependencydiff/dependencydiff.go @@ -41,7 +41,7 @@ type dependencydiffContext struct { ossFuzzClient clients.RepoClient vulnsClient clients.VulnerabilitiesClient ciiClient clients.CIIBestPracticesClient - changeTypesToCheck map[pkg.ChangeType]bool + changeTypesToCheck []string checkNamesToRun []string dependencydiffs []dependency results []pkg.DependencyCheckResult @@ -55,7 +55,7 @@ func GetDependencyDiffResults( repoURI string, /* Use the format "ownerName/repoName" as the repo URI, such as "ossf/scorecard". */ base, head string, /* Two code commits base and head, can use either SHAs or branch names. */ checksToRun []string, /* A list of enabled check names to run. */ - changeTypesToCheck map[pkg.ChangeType]bool, /* A list of change types for which to surface scorecard results. */ + changeTypes []string, /* A list of dependency change types for which we surface scorecard results. */ ) ([]pkg.DependencyCheckResult, error) { logger := sclog.NewLogger(sclog.DefaultLevel) @@ -71,7 +71,7 @@ func GetDependencyDiffResults( base: base, head: head, ctx: ctx, - changeTypesToCheck: changeTypesToCheck, + changeTypesToCheck: changeTypes, checkNamesToRun: checksToRun, } // Fetch the raw dependency diffs. This API will also handle error cases such as invalid base or head. @@ -133,13 +133,21 @@ func getScorecardCheckResults(dCtx *dependencydiffContext) error { Ecosystem: d.Ecosystem, Version: d.Version, Name: d.Name, + /* The scorecard check result is nil now. */ } - // Run the checks on all types if (1) the type is found in changeTypesToCheck or (2) no types are specified. - TypeFoundOrNoneGiven := dCtx.changeTypesToCheck[*d.ChangeType] || - (dCtx.changeTypesToCheck == nil || len(dCtx.changeTypesToCheck) == 0) + if d.ChangeType == nil { + // Since we allow a dependency having a nil change type, so we also + // give such a dependency a nil scorecard result. + dCtx.results = append(dCtx.results, depCheckResult) + continue + } + // (1) If no change types are specified, run the checks on all types of dependencies. + // (2) If there are change types specified by the user, run the checks on the specified types. + noneGivenOrIsSpecified := len(dCtx.changeTypesToCheck) == 0 || /* None specified.*/ + isSpecifiedByUser(*d.ChangeType, dCtx.changeTypesToCheck) /* Specified by the user.*/ // For now we skip those without source repo urls. // TODO (#2063): use the BigQuery dataset to supplement null source repo URLs to fetch the Scorecard results for them. - if d.SourceRepository != nil && TypeFoundOrNoneGiven { + if d.SourceRepository != nil && noneGivenOrIsSpecified { // Initialize the repo and client(s) corresponding to the checks to run. err = initRepoAndClientByChecks(dCtx, *d.SourceRepository) if err != nil { @@ -176,3 +184,15 @@ func getScorecardCheckResults(dCtx *dependencydiffContext) error { } return nil } + +func isSpecifiedByUser(ct pkg.ChangeType, changeTypes []string) bool { + if len(changeTypes) == 0 { + return false + } + for _, ctByUser := range changeTypes { + if string(ct) == ctByUser { + return true + } + } + return false +} diff --git a/dependencydiff/dependencydiff_test.go b/dependencydiff/dependencydiff_test.go index 75814ec7..b1088480 100644 --- a/dependencydiff/dependencydiff_test.go +++ b/dependencydiff/dependencydiff_test.go @@ -22,6 +22,7 @@ import ( "github.com/ossf/scorecard/v4/clients" sclog "github.com/ossf/scorecard/v4/log" + "github.com/ossf/scorecard/v4/pkg" ) // Test_fetchRawDependencyDiffData is a test function for fetchRawDependencyDiffData. @@ -226,3 +227,53 @@ func Test_mapDependencyEcosystemNaming(t *testing.T) { }) } } + +func Test_isSpecifiedByUser(t *testing.T) { + t.Parallel() + //nolint + tests := []struct { + name string + ct pkg.ChangeType + changeTypesToCheck []string + resultWanted bool + }{ + { + name: "error invalid github ecosystem", + }, + { + name: "added", + ct: pkg.ChangeType("added"), + changeTypesToCheck: nil, + resultWanted: false, + }, + { + name: "ct is added but not specified", + ct: pkg.ChangeType("added"), + changeTypesToCheck: []string{"removed"}, + resultWanted: false, + }, + { + name: "removed", + ct: pkg.ChangeType("added"), + changeTypesToCheck: []string{"added", "removed"}, + resultWanted: true, + }, + { + name: "not_supported", + ct: pkg.ChangeType("not_supported"), + changeTypesToCheck: []string{"added", "removed"}, + resultWanted: false, + }, + } + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + result := isSpecifiedByUser(tt.ct, tt.changeTypesToCheck) + if result != tt.resultWanted { + t.Errorf("result (%v) != result wanted (%v)", result, tt.resultWanted) + return + } + }) + } +} diff --git a/e2e/dependencydiff_test.go b/e2e/dependencydiff_test.go index fa388ba8..98fed969 100644 --- a/e2e/dependencydiff_test.go +++ b/e2e/dependencydiff_test.go @@ -22,7 +22,6 @@ import ( "github.com/ossf/scorecard/v4/checks" "github.com/ossf/scorecard/v4/dependencydiff" - "github.com/ossf/scorecard/v4/pkg" ) const ( @@ -38,8 +37,8 @@ var _ = Describe("E2E TEST:"+dependencydiff.Depdiff, func() { checksToRun := []string{ checks.CheckBranchProtection, } - changeTypesToCheck := map[pkg.ChangeType]bool{ - pkg.Removed: true, // Only checking those removed ones will make this test faster. + changeTypesToCheck := []string{ + "removed", // Only checking those removed ones will make this test faster. } results, err := dependencydiff.GetDependencyDiffResults( ctx, @@ -56,8 +55,8 @@ var _ = Describe("E2E TEST:"+dependencydiff.Depdiff, func() { checksToRun := []string{ checks.CheckBranchProtection, } - changeTypesToCheck := map[pkg.ChangeType]bool{ - pkg.Removed: true, + changeTypesToCheck := []string{ + "removed", } results, err := dependencydiff.GetDependencyDiffResults( ctx, @@ -74,8 +73,8 @@ var _ = Describe("E2E TEST:"+dependencydiff.Depdiff, func() { checksToRun := []string{ checks.CheckFuzzing, } - changeTypesToCheck := map[pkg.ChangeType]bool{ - pkg.Removed: true, + changeTypesToCheck := []string{ + "removed", } _, err := dependencydiff.GetDependencyDiffResults( ctx,