From c03085ad9baec2a6ea52560f27175a1499df0598 Mon Sep 17 00:00:00 2001 From: Azeem Shaikh Date: Tue, 22 Feb 2022 07:38:56 -0800 Subject: [PATCH] Remove duplicated function definitions (#1666) Co-authored-by: Azeem Shaikh --- checks/fileparser/listing.go | 36 ---------- checks/fileparser/listing_test.go | 113 ------------------------------ checks/license.go | 24 +++---- checks/raw/security_policy.go | 20 ++---- 4 files changed, 19 insertions(+), 174 deletions(-) diff --git a/checks/fileparser/listing.go b/checks/fileparser/listing.go index dcd9d606166..0cf7bbf4cb6 100644 --- a/checks/fileparser/listing.go +++ b/checks/fileparser/listing.go @@ -196,42 +196,6 @@ func CheckIfFileExistsV6(repoClient clients.RepoClient, return nil } -// FileCb represents a callback fn. -type FileCb func(path string, - dl checker.DetailLogger, data FileCbData) (bool, error) - -// CheckIfFileExists downloads the tar of the repository and calls the onFile() to check -// for the occurrence. -func CheckIfFileExists(c *checker.CheckRequest, onFile FileCb, data FileCbData) error { - matchedFiles, err := c.RepoClient.ListFiles(func(string) (bool, error) { return true, nil }) - if err != nil { - // nolint: wrapcheck - return err - } - for _, filename := range matchedFiles { - continueIter, err := onFile(filename, c.Dlogger, data) - if err != nil { - return err - } - - if !continueIter { - break - } - } - - return nil -} - -// FileGetCbDataAsBoolPointer returns callback data as bool. -func FileGetCbDataAsBoolPointer(data FileCbData) *bool { - pdata, ok := data.(*bool) - if !ok { - // This never happens. - panic("invalid type") - } - return pdata -} - // CheckFileContainsCommands checks if the file content contains commands or not. // `comment` is the string or character that indicates a comment: // for example for Dockerfiles, it would be `#`. diff --git a/checks/fileparser/listing_test.go b/checks/fileparser/listing_test.go index 66227186fe3..5496763da93 100644 --- a/checks/fileparser/listing_test.go +++ b/checks/fileparser/listing_test.go @@ -379,54 +379,6 @@ func Test_isTestdataFile(t *testing.T) { } } -// TestFileGetCbDataAsBoolPointer tests the FileGetCbDataAsBoolPointer function. -func TestFileGetCbDataAsBoolPointer(t *testing.T) { - t.Parallel() - type args struct { - data FileCbData - } - b := true - //nolint - tests := []struct { - name string - args args - want *bool - wantPanic bool - }{ - { - name: "true", - args: args{ - data: &b, - }, - want: &b, - }, - { - name: "nil", - args: args{}, - want: &b, - wantPanic: true, - }, - } - for _, tt := range tests { - tt := tt // Re-initializing variable so it is not changed while executing the closure below - t.Run(tt.name, func(t *testing.T) { - t.Parallel() - if tt.wantPanic { - defer func() { - if r := recover(); r == nil { - t.Errorf("FileGetCbDataAsBoolPointer() did not panic") - } - }() - FileGetCbDataAsBoolPointer(tt.args.data) - return - } - if got := FileGetCbDataAsBoolPointer(tt.args.data); got != tt.want { - t.Errorf("FileGetCbDataAsBoolPointer() = %v, want %v", got, tt.want) - } - }) - } -} - // TestCheckFilesContentV6 tests the CheckFilesContentV6 function. func TestCheckFilesContentV6(t *testing.T) { t.Parallel() @@ -699,68 +651,3 @@ func TestCheckIfFileExistsV6(t *testing.T) { }) } } - -// TestCheckIfFileExists tests the CheckIfFileExists function. -func TestCheckIfFileExists(t *testing.T) { - t.Parallel() - //nolint - type args struct { - cbReturn bool - cbErr error - } - //nolint - tests := []struct { - name string - args args - wantErr bool - }{ - { - name: "cb true and no error", - args: args{ - cbReturn: true, - cbErr: nil, - }, - wantErr: false, - }, - { - name: "cb false and no error", - args: args{ - cbReturn: false, - cbErr: nil, - }, - wantErr: false, - }, - { - name: "cb error", - args: args{ - cbReturn: true, - cbErr: errors.New("test error"), - }, - wantErr: true, - }, - } - for _, tt := range tests { - tt := tt // Re-initializing variable so it is not changed while executing the closure below - t.Run(tt.name, func(t *testing.T) { - t.Parallel() - - ctrl := gomock.NewController(t) - mockRepo := mockrepo.NewMockRepoClient(ctrl) - mockRepo.EXPECT().ListFiles(gomock.Any()).Return([]string{"foo"}, nil).AnyTimes() - c := checker.CheckRequest{ - RepoClient: mockRepo, - } - x := func(path string, - dl checker.DetailLogger, data FileCbData) (bool, error) { - return tt.args.cbReturn, tt.args.cbErr - } - - err := CheckIfFileExists(&c, x, x) - - if (err != nil) != tt.wantErr { - t.Errorf("CheckIfFileExists() error = %v, wantErr %v for %v", err, tt.wantErr, tt.name) - return - } - }) - } -} diff --git a/checks/license.go b/checks/license.go index d477be05ec4..ff5ee715f84 100644 --- a/checks/license.go +++ b/checks/license.go @@ -103,28 +103,28 @@ func testLicenseCheck(name string) bool { // LicenseCheck runs LicenseCheck check. func LicenseCheck(c *checker.CheckRequest) checker.CheckResult { - var r bool - - onFile := func(name string, dl checker.DetailLogger, data fileparser.FileCbData) (bool, error) { - pdata := fileparser.FileGetCbDataAsBoolPointer(data) + var s string + onFile := func(name string, data fileparser.FileCbData) (bool, error) { if checkLicense(name) { - c.Dlogger.Info(&checker.LogMessage{ - Path: name, - Type: checker.FileTypeSource, - Offset: 1, - }) - *pdata = true + if strData, ok := data.(*string); ok && strData != nil { + *strData = name + } return false, nil } return true, nil } - err := fileparser.CheckIfFileExists(c, onFile, &r) + err := fileparser.CheckIfFileExistsV6(c.RepoClient, onFile, &s) if err != nil { return checker.CreateRuntimeErrorResult(CheckLicense, err) } - if r { + if s != "" { + c.Dlogger.Info(&checker.LogMessage{ + Path: s, + Type: checker.FileTypeSource, + Offset: 1, + }) return checker.CreateMaxScoreResult(CheckLicense, "license file detected") } return checker.CreateMinScoreResult(CheckLicense, "license file not detected") diff --git a/checks/raw/security_policy.go b/checks/raw/security_policy.go index c5f66d5d1c8..c956aef853e 100644 --- a/checks/raw/security_policy.go +++ b/checks/raw/security_policy.go @@ -32,7 +32,7 @@ func SecurityPolicy(c *checker.CheckRequest) (checker.SecurityPolicyData, error) // Check repository for repository-specific policy. // https://docs.github.com/en/github/building-a-strong-community/creating-a-default-community-health-file. - onFile := func(name string, dl checker.DetailLogger, data fileparser.FileCbData) (bool, error) { + onFile := func(name string, data fileparser.FileCbData) (bool, error) { pfiles, ok := data.(*[]checker.File) if !ok { // This never happens. @@ -62,7 +62,7 @@ func SecurityPolicy(c *checker.CheckRequest) (checker.SecurityPolicyData, error) } files := make([]checker.File, 0) - err := fileparser.CheckIfFileExists(c, onFile, &files) + err := fileparser.CheckIfFileExistsV6(c.RepoClient, onFile, &files) if err != nil { return checker.SecurityPolicyData{}, err } @@ -74,18 +74,12 @@ func SecurityPolicy(c *checker.CheckRequest) (checker.SecurityPolicyData, error) // https://docs.github.com/en/github/building-a-strong-community/creating-a-default-community-health-file. logger := log.NewLogger(log.InfoLevel) - dotGitHub := &checker.CheckRequest{ - Ctx: c.Ctx, - Dlogger: c.Dlogger, - RepoClient: githubrepo.CreateGithubRepoClient(c.Ctx, logger), - Repo: c.Repo.Org(), - } - - err = dotGitHub.RepoClient.InitRepo(dotGitHub.Repo, clients.HeadSHA) + dotGitHubClient := githubrepo.CreateGithubRepoClient(c.Ctx, logger) + err = dotGitHubClient.InitRepo(c.Repo.Org(), clients.HeadSHA) switch { case err == nil: - defer dotGitHub.RepoClient.Close() - onFile = func(name string, dl checker.DetailLogger, data fileparser.FileCbData) (bool, error) { + defer dotGitHubClient.Close() + onFile = func(name string, data fileparser.FileCbData) (bool, error) { pfiles, ok := data.(*[]checker.File) if !ok { // This never happens. @@ -106,7 +100,7 @@ func SecurityPolicy(c *checker.CheckRequest) (checker.SecurityPolicyData, error) } return true, nil } - err = fileparser.CheckIfFileExists(dotGitHub, onFile, &files) + err = fileparser.CheckIfFileExistsV6(dotGitHubClient, onFile, &files) if err != nil { return checker.SecurityPolicyData{}, err }