Skip to content

Commit

Permalink
🌱 Add probe support for contributors metrics (#3460)
Browse files Browse the repository at this point in the history 
* 🌱 Add probe support for contributors metrics

Signed-off-by: AdamKorcz <[email protected]>

* fix lint issues

Signed-off-by: AdamKorcz <[email protected]>

* change 'contributorsWith' to 'contributorsFrom'

Signed-off-by: AdamKorcz <[email protected]>

* change remediation difficulty

Signed-off-by: AdamKorcz <[email protected]>

* fix nits

Signed-off-by: AdamKorcz <[email protected]>

* Updates to checks and checks/evaluation

Signed-off-by: AdamKorcz <[email protected]>

* fix tests like in #3409

Signed-off-by: AdamKorcz <[email protected]>

* fix raw test

Signed-off-by: AdamKorcz <[email protected]>

* Update description in def.yml

Signed-off-by: AdamKorcz <[email protected]>

* move logic out of utils

Signed-off-by: AdamKorcz <[email protected]>

* add comment to consolidate unit test validation

Signed-off-by: AdamKorcz <[email protected]>

* change a couple of t.Fatal to t.Error

Signed-off-by: AdamKorcz <[email protected]>

* un-remove comment

Signed-off-by: AdamKorcz <[email protected]>

* remove map

Signed-off-by: AdamKorcz <[email protected]>

* fix typo

Signed-off-by: AdamKorcz <[email protected]>

* remove lint comment

Signed-off-by: AdamKorcz <[email protected]>

* fix incorrect -1/0 scoring

Signed-off-by: AdamKorcz <[email protected]>

* Do not specify 'Github' in def.yml

Signed-off-by: AdamKorcz <[email protected]>

* do not mention 'which companies' in def.yml

Signed-off-by: AdamKorcz <[email protected]>

* Rename tests

Signed-off-by: AdamKorcz <[email protected]>

* Use getRawResults and uncomment logging statement

Signed-off-by: AdamKorcz <[email protected]>

* Define return values of probe better

Signed-off-by: AdamKorcz <[email protected]>

* Use proportional score instead of min score

Signed-off-by: AdamKorcz <[email protected]>

* revert changed scoring

Signed-off-by: AdamKorcz <[email protected]>

* fix incorrect function name

Signed-off-by: AdamKorcz <[email protected]>

* remove utility function that finds non-positive outcomes

Signed-off-by: AdamKorcz <[email protected]>

* rebase with latest upstream main and fix linter issues

Signed-off-by: AdamKorcz <[email protected]>

* Log findings in one statements except a logging statements per finding

Signed-off-by: AdamKorcz <[email protected]>

* redefine conditional logic

Signed-off-by: AdamKorcz <[email protected]>

* rebase

Signed-off-by: AdamKorcz <[email protected]>

* remove unused function

Signed-off-by: AdamKorcz <[email protected]>

---------

Signed-off-by: AdamKorcz <[email protected]>
  • Loading branch information
@AdamKorcz
AdamKorcz authored Oct 24, 2023
1 parent 1aca1d9 commit ae75bbb
Show file tree
Hide file tree
Showing 10 changed files with 419 additions and 157 deletions.
18 changes: 13 additions & 5 deletions checks/contributors.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@ import (
"github.com/ossf/scorecard/v4/checks/evaluation"
"github.com/ossf/scorecard/v4/checks/raw"
sce "github.com/ossf/scorecard/v4/errors"
"github.com/ossf/scorecard/v4/probes"
"github.com/ossf/scorecard/v4/probes/zrunner"
)

// CheckContributors is the registered name for Contributors.
Expand All @@ -34,17 +36,23 @@ func init() {

// Contributors run Contributors check.
func Contributors(c *checker.CheckRequest) checker.CheckResult {
rawData, err := raw.Contributors(c.RepoClient)
rawData, err := raw.Contributors(c)
if err != nil {
e := sce.WithMessage(sce.ErrScorecardInternal, err.Error())
return checker.CreateRuntimeErrorResult(CheckContributors, e)
}

// Return raw results.
if c.RawResults != nil {
c.RawResults.ContributorsResults = rawData
// Set the raw results.
pRawResults := getRawResults(c)
pRawResults.ContributorsResults = rawData

// Evaluate the probes.
findings, err := zrunner.Run(pRawResults, probes.Contributors)
if err != nil {
e := sce.WithMessage(sce.ErrScorecardInternal, err.Error())
return checker.CreateRuntimeErrorResult(CheckContributors, e)
}

// Return the score evaluation.
return evaluation.Contributors(CheckContributors, c.Dlogger, &rawData)
return evaluation.Contributors(CheckContributors, findings, c.Dlogger)
}
75 changes: 41 additions & 34 deletions checks/evaluation/contributors.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,60 +16,67 @@ package evaluation

import (
"fmt"
"sort"
"strings"

"github.com/ossf/scorecard/v4/checker"
sce "github.com/ossf/scorecard/v4/errors"
"github.com/ossf/scorecard/v4/finding"
"github.com/ossf/scorecard/v4/probes/contributorsFromOrgOrCompany"
)

const (
minContributionsPerUser = 5
numberCompaniesForTopScore = 3
)

// Contributors applies the score policy for the Contributors check.
func Contributors(name string, dl checker.DetailLogger,
r *checker.ContributorsData,
func Contributors(name string,
findings []finding.Finding,
dl checker.DetailLogger,
) checker.CheckResult {
if r == nil {
e := sce.WithMessage(sce.ErrScorecardInternal, "empty raw data")
return checker.CreateRuntimeErrorResult(name, e)
expectedProbes := []string{
contributorsFromOrgOrCompany.Probe,
}

entities := make(map[string]bool)

for _, user := range r.Users {
if user.NumContributions < minContributionsPerUser {
continue
}
if !finding.UniqueProbesEqual(findings, expectedProbes) {
e := sce.WithMessage(sce.ErrScorecardInternal, "invalid probe results")
return checker.CreateRuntimeErrorResult(name, e)
}

for _, org := range user.Organizations {
entities[org.Login] = true
}
numberOfPositives := getNumberOfPositives(findings)
reason := fmt.Sprintf("project has %d contributing companies or organizations", numberOfPositives)

for _, comp := range user.Companies {
entities[comp] = true
}
if numberOfPositives > 0 {
logFindings(findings, dl)
}

names := []string{}
for c := range entities {
names = append(names, c)
if numberOfPositives > numberCompaniesForTopScore {
return checker.CreateMaxScoreResult(name, reason)
}

sort.Strings(names)
return checker.CreateProportionalScoreResult(name, reason, numberOfPositives, numberCompaniesForTopScore)
}

if len(names) > 0 {
dl.Info(&checker.LogMessage{
Text: fmt.Sprintf("contributors work for %v", strings.Join(names, ",")),
})
} else {
dl.Warn(&checker.LogMessage{
Text: "no contributors have an org or company",
})
func getNumberOfPositives(findings []finding.Finding) int {
var numberOfPositives int
for i := range findings {
f := &findings[i]
if f.Outcome == finding.OutcomePositive {
if f.Probe == contributorsFromOrgOrCompany.Probe {
numberOfPositives++
}
}
}
return numberOfPositives
}

reason := fmt.Sprintf("%d different organizations found", len(entities))
return checker.CreateProportionalScoreResult(name, reason, len(entities), numberCompaniesForTopScore)
func logFindings(findings []finding.Finding, dl checker.DetailLogger) {
var sb strings.Builder
for i := range findings {
f := &findings[i]
if f.Outcome == finding.OutcomePositive {
sb.WriteString(fmt.Sprintf("%s, ", f.Message))
}
}
dl.Info(&checker.LogMessage{
Text: sb.String(),
})
}
163 changes: 50 additions & 113 deletions checks/evaluation/contributors_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,138 +16,75 @@ package evaluation
import (
"testing"

"github.com/google/go-cmp/cmp"
"github.com/google/go-cmp/cmp/cmpopts"

"github.com/ossf/scorecard/v4/checker"
"github.com/ossf/scorecard/v4/clients"
"github.com/ossf/scorecard/v4/utests"
"github.com/ossf/scorecard/v4/finding"
scut "github.com/ossf/scorecard/v4/utests"
)

func TestContributors(t *testing.T) {
t.Parallel()
testCases := []struct {
tests := []struct {
name string
raw *checker.ContributorsData
expected checker.CheckResult
findings []finding.Finding
result scut.TestReturn
}{
{
name: "No data",
raw: nil,
expected: checker.CheckResult{
Version: 2,
Score: -1,
Reason: "internal error: empty raw data",
},
},
{
name: "No contributors",
raw: &checker.ContributorsData{
Users: []clients.User{},
name: "Only has two positive outcomes",
findings: []finding.Finding{
{
Probe: "contributorsFromOrgOrCompany",
Outcome: finding.OutcomePositive,
},
{
Probe: "contributorsFromOrgOrCompany",
Outcome: finding.OutcomePositive,
},
},
expected: checker.CheckResult{
Version: 2,
Score: 0,
Reason: "0 different organizations found -- score normalized to 0",
result: scut.TestReturn{
Score: 6,
NumberOfInfo: 1,
},
},
{
name: "Contributors with orgs and number of contributions is greater than 5 with companies",
raw: &checker.ContributorsData{
Users: []clients.User{
{
NumContributions: 10,
Organizations: []clients.User{
{
Login: "org1",
},
},
Companies: []string{"company1"},
},
{
NumContributions: 10,
Organizations: []clients.User{
{
Login: "org2",
},
},
},
{
NumContributions: 10,
Organizations: []clients.User{
{
Login: "org3",
},
},
},
{
NumContributions: 1,
Organizations: []clients.User{
{
Login: "org2",
},
},
},
}, {
name: "No contributors",
findings: []finding.Finding{
{
Probe: "contributorsFromOrgOrCompany",
Outcome: finding.OutcomeNegative,
},
},
expected: checker.CheckResult{
Version: 2,
Score: 10,
Reason: "4 different organizations found -- score normalized to 10",
result: scut.TestReturn{
Score: 0,
},
},
{
name: "Contributors with orgs and number of contributions is greater than 5 without companies",
raw: &checker.ContributorsData{
Users: []clients.User{
{
NumContributions: 10,
Organizations: []clients.User{
{
Login: "org1",
},
},
},
{
NumContributions: 10,
Organizations: []clients.User{
{
Login: "org2",
},
},
},
{
NumContributions: 10,
Organizations: []clients.User{
{
Login: "org3",
},
},
},
{
NumContributions: 1,
Organizations: []clients.User{
{
Login: "org10",
},
},
},
}, {
name: "Has three positive outcomes",
findings: []finding.Finding{
{
Probe: "contributorsFromOrgOrCompany",
Outcome: finding.OutcomePositive,
},
{
Probe: "contributorsFromOrgOrCompany",
Outcome: finding.OutcomePositive,
},
{
Probe: "contributorsFromOrgOrCompany",
Outcome: finding.OutcomePositive,
},
},
expected: checker.CheckResult{
Version: 2,
Score: 10,
Reason: "3 different organizations found -- score normalized to 10",
result: scut.TestReturn{
Score: checker.MaxResultScore,
NumberOfInfo: 1,
},
},
}
for _, tc := range testCases {
tc := tc
t.Run(tc.name, func(t *testing.T) {
for _, tt := range tests {
tt := tt
t.Run(tt.name, func(t *testing.T) {
t.Parallel()
result := Contributors("", &utests.TestDetailLogger{}, tc.raw)
if !cmp.Equal(result, tc.expected, cmpopts.IgnoreFields(checker.CheckResult{}, "Error")) { //nolint:govet
t.Errorf("expected %v, got %v", tc.expected, cmp.Diff(tc.expected, result, cmpopts.IgnoreFields(checker.CheckResult{}, "Error"))) //nolint:lll
dl := scut.TestDetailLogger{}
got := Contributors(tt.name, tt.findings, &dl)
if !scut.ValidateTestReturn(t, tt.name, &tt.result, &got, &dl) {
t.Error(tt.name)
}
})
}
Expand Down
5 changes: 3 additions & 2 deletions checks/evaluation/finding.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,9 +34,10 @@ func negativeFindings(findings []finding.Finding) []finding.Finding {
var ff []finding.Finding
for i := range findings {
f := &findings[i]
if f.Outcome == finding.OutcomeNegative {
ff = append(ff, *f)
if f.Outcome == finding.OutcomePositive {
continue
}
ff = append(ff, *f)
}
return ff
}
3 changes: 2 additions & 1 deletion checks/raw/contributors.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,8 @@ import (
)

// Contributors retrieves the raw data for the Contributors check.
func Contributors(c clients.RepoClient) (checker.ContributorsData, error) {
func Contributors(cr *checker.CheckRequest) (checker.ContributorsData, error) {
c := cr.RepoClient
var users []clients.User

contribs, err := c.ListContributors()
Expand Down
7 changes: 5 additions & 2 deletions checks/raw/contributors_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import (
"github.com/golang/mock/gomock"
"github.com/google/go-cmp/cmp"

"github.com/ossf/scorecard/v4/checker"
"github.com/ossf/scorecard/v4/clients"
mockrepo "github.com/ossf/scorecard/v4/clients/mockclients"
)
Expand Down Expand Up @@ -130,8 +131,10 @@ func TestContributors(t *testing.T) {
}

mockRepoClient.EXPECT().ListContributors().Return(contributors, nil)

data, err := Contributors(mockRepoClient)
req := &checker.CheckRequest{
RepoClient: mockRepoClient,
}
data, err := Contributors(req)
if err != nil {
t.Fatalf("unexpected error: %v", err)
}
Expand Down
Loading

0 comments on commit ae75bbb

Please sign in to comment.