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.

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

🌱 Add probe support for contributors metrics #3460

Merged
merged 31 commits into from
Oct 24, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
31 commits
Select commit Hold shift + click to select a range
d746704
:seedling: Add probe support for contributors metrics
AdamKorcz Sep 5, 2023
2c1abb2
fix lint issues
AdamKorcz Sep 7, 2023
cc7d944
change 'contributorsWith' to 'contributorsFrom'
AdamKorcz Sep 7, 2023
e383b3b
change remediation difficulty
AdamKorcz Sep 7, 2023
69688ca
fix nits
AdamKorcz Sep 7, 2023
d840261
Updates to checks and checks/evaluation
AdamKorcz Sep 7, 2023
3d316e4
fix tests like in #3409
AdamKorcz Sep 8, 2023
163ee2a
fix raw test
AdamKorcz Sep 12, 2023
8440af4
Update description in def.yml
AdamKorcz Sep 12, 2023
1f72071
move logic out of utils
AdamKorcz Sep 12, 2023
231ca85
add comment to consolidate unit test validation
AdamKorcz Sep 12, 2023
d701890
change a couple of t.Fatal to t.Error
AdamKorcz Sep 14, 2023
44c710f
un-remove comment
AdamKorcz Sep 14, 2023
b53256d
remove map
AdamKorcz Sep 14, 2023
b936154
fix typo
AdamKorcz Sep 14, 2023
9fa6e7b
remove lint comment
AdamKorcz Sep 14, 2023
f574e7d
fix incorrect -1/0 scoring
AdamKorcz Sep 14, 2023
1c9d53c
Do not specify 'Github' in def.yml
AdamKorcz Sep 14, 2023
f9485c2
do not mention 'which companies' in def.yml
AdamKorcz Sep 14, 2023
b18c40c
Rename tests
AdamKorcz Sep 14, 2023
d627e32
Use getRawResults and uncomment logging statement
AdamKorcz Sep 14, 2023
fd41e07
Define return values of probe better
AdamKorcz Sep 14, 2023
251f93b
Use proportional score instead of min score
AdamKorcz Sep 14, 2023
54dbe9f
revert changed scoring
AdamKorcz Sep 28, 2023
4d1d4f2
fix incorrect function name
AdamKorcz Oct 5, 2023
8966f96
remove utility function that finds non-positive outcomes
AdamKorcz Oct 9, 2023
bec8e29
rebase with latest upstream main and fix linter issues
AdamKorcz Oct 11, 2023
55fb01e
Log findings in one statements except a logging statements per finding
AdamKorcz Oct 20, 2023
e24b6df
redefine conditional logic
AdamKorcz Oct 24, 2023
74d28f7
rebase
AdamKorcz Oct 24, 2023
4087852
remove unused function
AdamKorcz Oct 24, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 13 additions & 5 deletions checks/contributors.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@
"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 @@

// 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)

Check warning on line 53 in checks/contributors.go

View check run for this annotation

Codecov / codecov/patch

checks/contributors.go#L52-L53

Added lines #L52 - L53 were not covered by tests
}

// 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
Original file line number Diff line number Diff line change
Expand Up @@ -16,60 +16,67 @@

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.
AdamKorcz marked this conversation as resolved.
Show resolved Hide resolved
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)
}

Check warning on line 43 in checks/evaluation/contributors.go

View check run for this annotation

Codecov / codecov/patch

checks/evaluation/contributors.go#L41-L43

Added lines #L41 - L43 were not covered by tests

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)
AdamKorcz marked this conversation as resolved.
Show resolved Hide resolved
}

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{
AdamKorcz marked this conversation as resolved.
Show resolved Hide resolved
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
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
Original file line number Diff line number Diff line change
Expand Up @@ -34,9 +34,10 @@
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 {
spencerschrock marked this conversation as resolved.
Show resolved Hide resolved
continue

Check warning on line 38 in checks/evaluation/finding.go

View check run for this annotation

Codecov / codecov/patch

checks/evaluation/finding.go#L38

Added line #L38 was not covered by tests
}
ff = append(ff, *f)
}
return ff
}
3 changes: 2 additions & 1 deletion checks/raw/contributors.go
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
laurentsimon marked this conversation as resolved.
Show resolved Hide resolved
var users []clients.User

contribs, err := c.ListContributors()
Expand Down
7 changes: 5 additions & 2 deletions checks/raw/contributors_test.go
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)
AdamKorcz marked this conversation as resolved.
Show resolved Hide resolved
}
Expand Down
Loading
Loading