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

⚠️ Delegate logging decisions to the checks instead of a helper #4019

Merged
merged 11 commits into from
Apr 9, 2024

Conversation

spencerschrock
Copy link
Member

What kind of change does this PR introduce?

breaking change since we're deleting functions

What is the current behavior?

checks use checker.LogFindings because logging used to be more standardized when we used Positive and Negative for outcomes.

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

The individual checks make decisions based on how to log a particular finding. Since it now depends on probe name and outcome.

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

Which issue(s) this PR fixes

Related to #3654

Special notes for your reviewer

Does 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.)

NONE

This may not be the final form, we may support want to support
passing a map of probe+outcome to level mappings.

Signed-off-by: Spencer Schrock <[email protected]>
For now, we only use one probe so logging is simple.

Signed-off-by: Spencer Schrock <[email protected]>
This changes the logging of an error state, but it's not one we expect to see.

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

codecov bot commented Apr 9, 2024

Codecov Report

Merging #4019 (1bb69eb) into main (b118c19) will decrease coverage by 6.59%.
Report is 4 commits behind head on main.
The diff coverage is 72.81%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4019      +/-   ##
==========================================
- Coverage   66.70%   60.11%   -6.59%     
==========================================
  Files         223      207      -16     
  Lines       16070    15268     -802     
==========================================
- Hits        10719     9179    -1540     
- Misses       4688     5421     +733     
- Partials      663      668       +5     

@spencerschrock
Copy link
Member Author

/scdiff generate Dependency-Update-Tool,Fuzzing,License,Maintained,Packaging,Security-Policy,Signed-Releases,Vulnerabilities

Copy link

github-actions bot commented Apr 9, 2024

scdiff caught a lot of new details being generated.
So going to try removing them

Signed-off-by: Spencer Schrock <[email protected]>
@spencerschrock
Copy link
Member Author

/scdiff generate Dependency-Update-Tool,Fuzzing,License,Maintained,Packaging,Security-Policy,Signed-Releases,Vulnerabilities

Copy link

github-actions bot commented Apr 9, 2024

@spencerschrock spencerschrock marked this pull request as ready for review April 9, 2024 20:11
@spencerschrock spencerschrock requested a review from a team as a code owner April 9, 2024 20:11
@spencerschrock
Copy link
Member Author

I will have to do yet another follow up for flipping the probe outcomes around.
Deciding when to add remediation was becoming a larger change

@spencerschrock spencerschrock merged commit a220b48 into ossf:main Apr 9, 2024
36 checks passed
@spencerschrock spencerschrock deleted the outcome-fix-logging branch April 9, 2024 22:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants