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 dependency remediation in raw results instead of at log time #3632

Merged
merged 13 commits into from
Nov 9, 2023

Conversation

AdamKorcz
Copy link
Contributor

What kind of change does this PR introduce?

This moves the creation of remediation for dependencies to the part where the raw results get created instead of in the evaluation.

This is an intermediary PR to prepare the pinned dependencies check for being migrated to probes; for that purpose, client calls should not be in the evaluation phase.

(Is it a bug fix, feature, docs update, something else?)

What is the current behavior?

What is the new behavior (if this is a feature change)?**

  • Tests for the changes have been added (for bug fixes/features)

Which issue(s) this PR fixes

NONE

Special notes for your reviewer

Does this PR introduce a user-facing change?

NONE


@AdamKorcz AdamKorcz requested a review from a team as a code owner October 31, 2023 10:39
@AdamKorcz AdamKorcz requested review from spencerschrock and raghavkaul and removed request for a team October 31, 2023 10:39
@AdamKorcz AdamKorcz temporarily deployed to gitlab October 31, 2023 10:39 — with GitHub Actions Inactive
@AdamKorcz AdamKorcz temporarily deployed to integration-test October 31, 2023 10:39 — with GitHub Actions Inactive
@AdamKorcz AdamKorcz force-pushed the pinned-dependencies-1 branch from 1ecd0b6 to 8a33ad5 Compare October 31, 2023 10:40
@AdamKorcz AdamKorcz temporarily deployed to gitlab October 31, 2023 10:40 — with GitHub Actions Inactive
@AdamKorcz AdamKorcz changed the title 🌱 Create dependency remediation in raw 🌱 Add dependency remediation in raw results instead of at log time Oct 31, 2023
@AdamKorcz AdamKorcz temporarily deployed to integration-test October 31, 2023 10:40 — with GitHub Actions Inactive
@codecov
Copy link

codecov bot commented Oct 31, 2023

Codecov Report

Merging #3632 (0547949) into main (0fc8296) will decrease coverage by 5.68%.
Report is 1 commits behind head on main.
The diff coverage is 76.00%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3632      +/-   ##
==========================================
- Coverage   76.13%   70.45%   -5.68%     
==========================================
  Files         206      206              
  Lines       14053    14065      +12     
==========================================
- Hits        10699     9910     -789     
- Misses       2726     3578     +852     
+ Partials      628      577      -51     

@AdamKorcz AdamKorcz temporarily deployed to gitlab October 31, 2023 16:09 — with GitHub Actions Inactive
@AdamKorcz AdamKorcz temporarily deployed to integration-test October 31, 2023 16:09 — with GitHub Actions Inactive
@AdamKorcz AdamKorcz temporarily deployed to gitlab October 31, 2023 17:47 — with GitHub Actions Inactive
@AdamKorcz AdamKorcz temporarily deployed to integration-test October 31, 2023 17:48 — with GitHub Actions Inactive
checks/raw/pinned_dependencies.go Outdated Show resolved Hide resolved
checks/raw/pinned_dependencies.go Outdated Show resolved Hide resolved
checks/raw/pinned_dependencies.go Outdated Show resolved Hide resolved
@AdamKorcz AdamKorcz temporarily deployed to gitlab October 31, 2023 22:38 — with GitHub Actions Inactive
@AdamKorcz AdamKorcz temporarily deployed to gitlab October 31, 2023 23:02 — with GitHub Actions Inactive
@spencerschrock
Copy link
Member

Ah surfacing the error is causing the e2e to fail.

• [FAILED] [0.359 seconds]
E2E TEST:Pinned-Dependencies E2E TEST:Validating dependencies check is working [It] Should return dependencies check for a local repoClient
/home/runner/work/scorecard/scorecard/e2e/pinned_dependencies_test.go:86

  Captured StdOut/StdErr Output >>
  2023/10/31 23:18:37 dependencies check:   utests.TestReturn{
  - 	Error:         nil,
  + 	Error:         e"internal error: failed to create remediation metadata: GetDefaultBranchName: GetDefaultBranchName: unsupported feature",

Maybe something like this? We used to ignore the unsupported features like this.

diff --git a/remediation/remediations.go b/remediation/remediations.go
index b5713f7f..ab1dcd4d 100644
--- a/remediation/remediations.go
+++ b/remediation/remediations.go
@@ -22,6 +22,7 @@ import (
        "github.com/google/go-containerregistry/pkg/crane"
 
        "github.com/ossf/scorecard/v4/checker"
+       "github.com/ossf/scorecard/v4/clients"
        "github.com/ossf/scorecard/v4/rule"
 )
 
@@ -49,6 +50,9 @@ func New(c *checker.CheckRequest) (*RemediationMetadata, error) {
        // Get the branch for remediation.
        branch, err := c.RepoClient.GetDefaultBranchName()
        if err != nil {
+               if errors.Is(err, clients.ErrUnsupportedFeature) {
+                       return nil, nil
+               }
                return &RemediationMetadata{}, fmt.Errorf("GetDefaultBranchName: %w", err)
        }
 
@@ -63,6 +67,9 @@ func New(c *checker.CheckRequest) (*RemediationMetadata, error) {
 
 // CreateWorkflowPinningRemediation create remediaiton for pinninn GH Actions.
 func (r *RemediationMetadata) CreateWorkflowPinningRemediation(filepath string) *rule.Remediation {
+       if r == nil {
+               return nil
+       }
        return r.createWorkflowRemediation(filepath, "pin")
 }

This causes other e2e test(s) to fail. Up to you if you want to deal with it or maybe we just say "oops" and revert the error checking and tackle it in another PR

checks/raw/pinned_dependencies.go Outdated Show resolved Hide resolved
checks/raw/pinned_dependencies.go Outdated Show resolved Hide resolved
checks/raw/pinned_dependencies_test.go Outdated Show resolved Hide resolved
checks/raw/pinned_dependencies_test.go Outdated Show resolved Hide resolved
@spencerschrock spencerschrock merged commit b3d1a5a into ossf:main Nov 9, 2023
38 checks passed
diogoteles08 pushed a commit to diogoteles08/scorecard that referenced this pull request Nov 13, 2023
…ssf#3632)

* 🌱 Add dependency remediation in raw results instead of at log time

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

* add unit test

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

* add unit test

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

* return error

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

* use pointer to dependency

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

* check for errors in test

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

* Return nil if repo client returns an error from unsupported feature

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

* revert error checking

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

* revert returning nil is unsupported feature

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

* Fix wrong test name

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

* only create remediation when required

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

* remove remediation helper function

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

---------

Signed-off-by: AdamKorcz <[email protected]>
Signed-off-by: Spencer Schrock <[email protected]>
Signed-off-by: Diogo Teles Sant'Anna <[email protected]>
ashearin pushed a commit to kgangerlm/scorecard-gitlab that referenced this pull request Nov 13, 2023
…ssf#3632)

* 🌱 Add dependency remediation in raw results instead of at log time

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

* add unit test

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

* add unit test

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

* return error

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

* use pointer to dependency

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

* check for errors in test

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

* Return nil if repo client returns an error from unsupported feature

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

* revert error checking

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

* revert returning nil is unsupported feature

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

* Fix wrong test name

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

* only create remediation when required

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

* remove remediation helper function

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

---------

Signed-off-by: AdamKorcz <[email protected]>
Signed-off-by: Spencer Schrock <[email protected]>
Signed-off-by: Allen Shearin <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants