✨ Allow incomplete local checks #4423
Conversation
Signed-off-by: Luke Harrison <[email protected]>
lharrison13
requested review from
spencerschrock and
raghavkaul
and removed request for
a team
lharrison13
changed the title
lharrison13
temporarily deployed
to
integration-test
GitHub Actions
Inactive
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4423 +/- ##
==========================================
+ Coverage 66.80% 68.79% +1.98%
==========================================
Files 230 236 +6
Lines 16602 17573 +971
==========================================
+ Hits 11091 12089 +998
+ Misses 4808 4691 -117
- Partials 703 793 +90 |
checks/packaging.go
Outdated
| var err, errG, errGL error | ||
|
|
There was a problem hiding this comment.
can we be more explicit about which error is which?
errGitHub and errGitLab?
checks/raw/fuzzing.go
Outdated
| // ListProgrammingLanguages is unsupported by local client so just return | ||
| if errors.Is(err, clients.ErrUnsupportedFeature) { | ||
| return checker.FuzzingData{Fuzzers: detectedFuzzers}, nil | ||
| } |
There was a problem hiding this comment.
we miss out on most of the file-based detection if we return early here. (only clusterfuzzlite is detected).
I wonder if it makes sense to scan for every language if the client doesn't have the info
checks/raw/github/packaging.go
Outdated
| if err != nil { | ||
| return data, fmt.Errorf("Client.Actions.ListWorkflowRunsByFileName: %w", err) | ||
| // appending empty run for localdir client |
There was a problem hiding this comment.
lets clarify the purpose of adding the empty run in the comment. maybe something like (but open to suggestions):
// assume the workflow will run for localdir client
checks/raw/security_policy.go
Outdated
| case errors.Is(err, sce.ErrRepoUnreachable), errors.Is(err, clients.ErrUnsupportedFeature): | ||
| break | ||
| default: | ||
| return checker.SecurityPolicyData{}, err | ||
| if !errors.Is(err, clients.ErrUnsupportedFeature) { | ||
| return checker.SecurityPolicyData{}, err | ||
| } |
There was a problem hiding this comment.
does the case above default not already handle clients.ErrUnsupportedFeature?
There was a problem hiding this comment.
I honestly have no clue why i put that there
clients/localdir/client.go
Outdated
| //nolint:govet | ||
| type localDirClient struct { | ||
| type LocalDirClient struct { |
There was a problem hiding this comment.
nit: now that we're exporting this type, lets make the name match the others:
type Client struct {so that users will write:
case *localdir.Client:instead of having the repetition
case *localdir.LocalDirClient:| requiredRequestTypes: []checker.RequestType{checker.FileBased, checker.CommitBased}, | ||
| expectedEnabledChecks: 5, // All checks which are FileBased and CommitBased | ||
| expectedEnabledChecks: 7, // All checks which are FileBased and CommitBased |
There was a problem hiding this comment.
hmm, i thought this PR added FileBased to 3 checks that weren't before? Any idea what's going on here?
There was a problem hiding this comment.
I think that test is for FileBased and Commit Based. I added 3 checks that were FileBased but two originally had CommitBased.
Signed-off-by: Luke Harrison <[email protected]>
Signed-off-by: Luke Harrison <[email protected]>
What kind of change does this PR introduce?
(Is it a bug fix, feature, docs update, something else?)
What is the current behavior?
Certain checks are currently not run when using the
localoptionWhat is the new behavior (if this is a feature change)?**
Now allows incomplete versions of the following checks to be run when using the
localoptionFuzzing
License
Packaging
SAST
Security-Policy
Which issue(s) this PR fixes
Fixes #3832
Special notes for your reviewer
go-git#1709Does this PR introduce a user-facing change?
For user-facing changes, please add a concise, human-readable release note to
the
release-note(In particular, describe what changes users might need to make in their
application as a result of this pull request.)