Skip to content

Commit

Permalink
✨ Support Binary-Artifacts check again for local repos (#3415)
Browse files Browse the repository at this point in the history
* invert workflow check and explain early exit.

Signed-off-by: Spencer Schrock <[email protected]>

* make workflow run validation optional.

Signed-off-by: Spencer Schrock <[email protected]>

* mark binary artifacts as local file friendly.

Signed-off-by: Spencer Schrock <[email protected]>

* add test for gradle wrapper without workflow run support

Signed-off-by: Spencer Schrock <[email protected]>

* fix policy tests and make their names more clear.

Signed-off-by: Spencer Schrock <[email protected]>

---------

Signed-off-by: Spencer Schrock <[email protected]>
  • Loading branch information
spencerschrock authored Aug 23, 2023
1 parent 475d975 commit eba10df
Show file tree
Hide file tree
Showing 4 changed files with 90 additions and 37 deletions.
3 changes: 2 additions & 1 deletion checks/binary_artifact.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,10 +24,11 @@ import (
// CheckBinaryArtifacts is the exported name for Binary-Artifacts check.
const CheckBinaryArtifacts string = "Binary-Artifacts"

//nolint
//nolint:gochecknoinits
func init() {
supportedRequestTypes := []checker.RequestType{
checker.CommitBased,
checker.FileBased,
}
if err := registerCheck(CheckBinaryArtifacts, BinaryArtifacts, supportedRequestTypes); err != nil {
// this should never happen
Expand Down
42 changes: 26 additions & 16 deletions checks/raw/binary_artifact.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
package raw

import (
"errors"
"fmt"
"path/filepath"
"regexp"
Expand Down Expand Up @@ -201,26 +202,35 @@ func gradleWrapperValidated(c clients.RepoClient) (bool, error) {
if err != nil {
return false, fmt.Errorf("%w", err)
}
if gradleWrapperValidatingWorkflowFile != "" {
// If validated, check that latest commit has a relevant successful run
runs, err := c.ListSuccessfulWorkflowRuns(gradleWrapperValidatingWorkflowFile)
if err != nil {
return false, fmt.Errorf("failure listing workflow runs: %w", err)
}
commits, err := c.ListCommits()
if err != nil {
return false, fmt.Errorf("failure listing commits: %w", err)
}
if len(commits) < 1 || len(runs) < 1 {
// no matching files, validation failed
if gradleWrapperValidatingWorkflowFile == "" {
return false, nil
}

// If validated, check that latest commit has a relevant successful run
runs, err := c.ListSuccessfulWorkflowRuns(gradleWrapperValidatingWorkflowFile)
if err != nil {
// some clients, such as the local file client, don't support this feature
// claim unvalidated, so that other parts of the check can still be used.
if errors.Is(err, clients.ErrUnsupportedFeature) {
return false, nil
}
for _, r := range runs {
if *r.HeadSHA == commits[0].SHA {
// Commit has corresponding successful run!
return true, nil
}
return false, fmt.Errorf("failure listing workflow runs: %w", err)
}
commits, err := c.ListCommits()
if err != nil {
return false, fmt.Errorf("failure listing commits: %w", err)
}
if len(commits) < 1 || len(runs) < 1 {
return false, nil
}
for _, r := range runs {
if *r.HeadSHA == commits[0].SHA {
// Commit has corresponding successful run!
return true, nil
}
}

return false, nil
}

Expand Down
32 changes: 32 additions & 0 deletions checks/raw/binary_artifact_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -256,3 +256,35 @@ func TestBinaryArtifacts(t *testing.T) {
})
}
}

func TestBinaryArtifacts_workflow_runs_unsupported(t *testing.T) {
ctrl := gomock.NewController(t)
mockRepoClient := mockrepo.NewMockRepoClient(ctrl)
const jarFile = "gradle-wrapper.jar"
const verifyWorkflow = ".github/workflows/verify.yaml"
files := []string{jarFile, verifyWorkflow}
mockRepoClient.EXPECT().ListFiles(gomock.Any()).Return(files, nil).AnyTimes()
mockRepoClient.EXPECT().GetFileContent(jarFile).DoAndReturn(func(file string) ([]byte, error) {
content, err := os.ReadFile("../testdata/binaryartifacts/jars/gradle-wrapper.jar")
if err != nil {
return nil, fmt.Errorf("%w", err)
}
return content, nil
}).AnyTimes()
mockRepoClient.EXPECT().GetFileContent(verifyWorkflow).DoAndReturn(func(file string) ([]byte, error) {
content, err := os.ReadFile("../testdata/binaryartifacts/workflows/verify.yaml")
if err != nil {
return nil, fmt.Errorf("%w", err)
}
return content, nil
}).AnyTimes()

mockRepoClient.EXPECT().ListSuccessfulWorkflowRuns(gomock.Any()).Return(nil, clients.ErrUnsupportedFeature).AnyTimes()
got, err := BinaryArtifacts(mockRepoClient)
if err != nil {
t.Fatalf("unexpected error: %v", err)
}
if len(got.Files) != 1 {
t.Errorf("expected 1 file, got %d", len(got.Files))
}
}
50 changes: 30 additions & 20 deletions policy/policy_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -217,7 +217,7 @@ func TestIsSupportedCheck(t *testing.T) {
result := isSupportedCheck(checkName, requiredRequestTypes)

// Assert the result
expectedResult := false
expectedResult := true
if result != expectedResult {
t.Errorf("Unexpected result: got %v, want %v", result, expectedResult)
}
Expand Down Expand Up @@ -308,49 +308,59 @@ func TestGetEnabled(t *testing.T) {

tests := []struct {
name string
sp *ScorecardPolicy
policyFile string
argsChecks []string
requiredRequestTypes []checker.RequestType
expectedEnabledChecks int
expectedError bool
}{
{
name: "With ScorecardPolicy and argsChecks",
sp: &ScorecardPolicy{},
name: "checks limited to those specified by checks arg",
argsChecks: []string{"Binary-Artifacts"},
requiredRequestTypes: []checker.RequestType{checker.FileBased, checker.CommitBased},
expectedEnabledChecks: 0,
expectedError: true,
requiredRequestTypes: []checker.RequestType{checker.FileBased},
expectedEnabledChecks: 1,
expectedError: false,
},
{
name: "With ScorecardPolicy and unsupported check",
sp: &ScorecardPolicy{},
name: "mix of supported and unsupported checks",
argsChecks: []string{"Binary-Artifacts", "UnsupportedCheck"},
requiredRequestTypes: []checker.RequestType{checker.FileBased, checker.CommitBased},
expectedEnabledChecks: 0,
expectedEnabledChecks: 1,
expectedError: true,
},
{
name: "Without ScorecardPolicy and argsChecks",
sp: nil,
name: "request types limit enabled checks",
argsChecks: []string{},
requiredRequestTypes: []checker.RequestType{checker.FileBased, checker.CommitBased},
expectedEnabledChecks: 4, // All checks are enabled by default
expectedEnabledChecks: 5, // All checks which are FileBased and CommitBased
expectedError: false,
},
{
name: "With ScorecardPolicy and missing policy",
sp: &ScorecardPolicy{},
argsChecks: []string{"Binary-Artifacts"},
requiredRequestTypes: []checker.RequestType{checker.FileBased, checker.CommitBased},
expectedEnabledChecks: 0,
expectedError: true,
name: "all checks in policy file enabled",
policyFile: "testdata/policy-ok.yaml",
argsChecks: []string{},
requiredRequestTypes: []checker.RequestType{},
expectedEnabledChecks: 3,
expectedError: false,
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
enabledChecks, err := GetEnabled(tt.sp, tt.argsChecks, tt.requiredRequestTypes)
var sp *ScorecardPolicy
if tt.policyFile != "" {
policyBytes, err := os.ReadFile(tt.policyFile)
if err != nil {
t.Fatalf("reading policy file: %v", err)
}
pol, err := parseFromYAML(policyBytes)
if err != nil {
t.Fatalf("parsing policy file: %v", err)
}
sp = pol
}

enabledChecks, err := GetEnabled(sp, tt.argsChecks, tt.requiredRequestTypes)

if len(enabledChecks) != tt.expectedEnabledChecks {
t.Errorf("Unexpected number of enabled checks: got %v, want %v", len(enabledChecks), tt.expectedEnabledChecks)
Expand Down

0 comments on commit eba10df

Please sign in to comment.