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

🌱 refactor pinned dependencies #3667

Merged
merged 2 commits into from
Nov 27, 2023

Conversation

AdamKorcz
Copy link
Contributor

What kind of change does this PR introduce?

The goal of this PR is to refactor the Pinned Dependencies evaluation to process findings instead of dependencies. The reason for doing this is to structure the code to resemble how it would look once it gets split into probes without modifying the score. As such, this is an intermediary code change towards migrating the Pinned Dependencies check into probes. Some of the code changes in this PR will be moved into probes in the near future.

My motivation for this PR is to reduce the review overhead. This PR is already substantial, and adding probes would add to that. As such, with this PR I want to refactor the code so it works as-is and make incremental steps towards migrating the check to probes.

The core change of this PR is an added step of converting the dependencies to findings and calculate the score + log from the findings instead of directly from the dependencies. Other code changes are utilities and tests around that core change.

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

  • PR title follows the guidelines defined in our pull request documentation

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

Which issue(s) this PR fixes

NONE

Does this PR introduce a user-facing change?

NONE


@AdamKorcz AdamKorcz requested a review from a team as a code owner November 12, 2023 18:53
@AdamKorcz AdamKorcz requested review from raghavkaul and laurentsimon and removed request for a team November 12, 2023 18:53
Copy link

codecov bot commented Nov 12, 2023

Codecov Report

Merging #3667 (4d7886c) into main (1a17bb8) will decrease coverage by 5.47%.
The diff coverage is 86.66%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3667      +/-   ##
==========================================
- Coverage   76.23%   70.76%   -5.47%     
==========================================
  Files         209      209              
  Lines       14212    14310      +98     
==========================================
- Hits        10834    10127     -707     
- Misses       2737     3596     +859     
+ Partials      641      587      -54     

@spencerschrock
Copy link
Member

/scdiff generate Pinned-Dependencies

Copy link

Copy link
Member

@spencerschrock spencerschrock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the scdiff run showed no diffs, and the tests are working, so good enough for me for a temporary refactor

checks/evaluation/pinned_dependencies_test.go Outdated Show resolved Hide resolved
Signed-off-by: AdamKorcz <adam@adalogics.com>
Signed-off-by: AdamKorcz <adam@adalogics.com>
@spencerschrock spencerschrock merged commit f8198b0 into ossf:main Nov 27, 2023
38 checks passed
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.

3 participants