feat: risk score filtering [IDE-1579] #1081
Conversation
The threshold value is stored in the folder config. When the risk score FF is set, only issues matching or exceeding this value are displayed.
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
Summary of ChangesHello @rrama, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a significant enhancement by enabling risk score-based filtering for issues, allowing users to focus on the most critical findings. It centralizes and streamlines the issue visibility determination process, making the codebase more robust and easier to extend. Additionally, it addresses a performance concern by reducing verbose logging, contributing to a more efficient system. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces filtering for OSS issues based on a risk score threshold, which is a valuable addition. The implementation is clean, with the new logic well-encapsulated. I appreciate the refactoring of existing filter functions into methods, which improves code organization, and the removal of spammy logs. The tests are thorough, covering the new configuration mechanism and the risk score filtering feature. Overall, this is a solid contribution. I have one minor suggestion to improve code conciseness.
Team-Hammerhead
temporarily deployed
to
Standard
GitHub Actions
Inactive
rrama
force-pushed
the
feat/IDE-1579_risk-score-filtering
branch
from
10a44a8 to
01fb6df
Compare
|
Why is it global, and not per folder config / org? |
| enableSnykLearnCodeActions bool | ||
| enableSnykOSSQuickFixCodeActions bool | ||
| enableDeltaFindings bool | ||
| writer *zerolog.ConsoleWriter |
There was a problem hiding this comment.
why do we introduce this?
| // AddLoggerOutputForTesting allows tests to capture log output by adding an additional writer. | ||
| // Note: This updates c.logger but not c.writer, as the new logger still outputs through c.writer's formatting. | ||
| func (c *Config) AddLoggerOutputForTesting(writer io.Writer) { | ||
| c.m.Lock() | ||
| defer c.m.Unlock() | ||
| newLogger := c.logger.Output(io.MultiWriter(c.writer, writer)) | ||
| c.logger = &newLogger | ||
| } | ||
|
|
There was a problem hiding this comment.
Hm. Why do we need to do this in production code instead of our test setup? This code, should probably live in a _test.go file or in our test helpers.
There was a problem hiding this comment.
Also, I'm not sure we need (or should) verify log output to test business logic.
There was a problem hiding this comment.
I come from a background where if a customer has an issue, you can't ask them to upgrade to a version where we add more logging, you need to get all the information you need from the first set of logs. So we would always add tests to ensure the log lines we cared about were displayed correctly, mostly so that if another developer removed it in the future, they were made aware that it was important because a test would fail (just adding comments did not work, as people deleted or skipped calling functions that did the logging inside of them sometimes).
I am happy to not have this sort of testing if we don't think it necessary.
I do agree that I probably should have made a testhelper in the config package for it.
| logger := c.Logger().With().Str("method", "isVisibleForIssueViewOptions").Logger() | ||
| func (f *Folder) isVisibleRiskScore(issue types.Issue) bool { | ||
| riskScoreThreshold := f.c.RiskScoreThreshold() | ||
| if riskScoreThreshold == 0 { |
There was a problem hiding this comment.
could be a switch, I guess, instead of an if - elseif - else
| }) | ||
| } | ||
|
|
||
| func Test_FilterIssues_LogsCorrectFilterReasons(t *testing.T) { |
There was a problem hiding this comment.
Lots of (test) code to check if a log message is correct. Do we really need this?
There was a problem hiding this comment.
It was also more of a check all the filters work together test, as we have tests for them individually, but never together. Not that I would ever expect it to cause issues when you have multiple issues that are filtered for different reasons.
Description
The threshold value is stored in the global config. When the risk score FF is set, only issues matching or exceeding this value are displayed.
Also removed some logging, which was causing excessive spam in our logs as it was per issue.
The logging has been replaced with a summary of issues filtered and why.
Added a test which checks for this logging, and this required some changes to
config.goso an additional log output can be attached and checked in the test.Checklist
make generate)make lint-fix)