Skip to content

Conversation

@isasmendiagus
Copy link
Contributor

This PR enhances SCANOSS scanner by implementing:

  • Exclusion pattern support: Respects path patterns in .ort.yml to exclude files during scans
  • Snippet choice processing: Converts ORT's SnippetChoices to SCANOSS rule types

This builds on top of the SCANOSS SDK migration completed in the previous PR #10265

@isasmendiagus isasmendiagus requested a review from a team as a code owner May 6, 2025 11:23
@codecov
Copy link

codecov bot commented May 6, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 56.43%. Comparing base (e855ea5) to head (792f5de).
Report is 3 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff            @@
##               main   #10270   +/-   ##
=========================================
  Coverage     56.43%   56.43%           
  Complexity     1604     1604           
=========================================
  Files           331      331           
  Lines         12243    12243           
  Branches       1135     1135           
=========================================
  Hits           6909     6909           
  Misses         4887     4887           
  Partials        447      447           
Flag Coverage Δ
funTest-docker 70.59% <ø> (ø)
funTest-non-docker 33.42% <ø> (ø)
test-ubuntu-24.04 40.45% <ø> (ø)
test-windows-2022 40.43% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@isasmendiagus isasmendiagus force-pushed the feat/scanoss/exclusion-patterns-and-snippet-choices branch from 53042f4 to 2c695dc Compare May 6, 2025 11:43
@isasmendiagus isasmendiagus force-pushed the feat/scanoss/exclusion-patterns-and-snippet-choices branch 2 times, most recently from 93ad889 to 9761e69 Compare May 6, 2025 14:21
@isasmendiagus isasmendiagus requested a review from sschuberth May 6, 2025 14:23
@isasmendiagus isasmendiagus force-pushed the feat/scanoss/exclusion-patterns-and-snippet-choices branch 2 times, most recently from d51e6aa to ee1c81e Compare May 6, 2025 16:05
@isasmendiagus isasmendiagus requested a review from sschuberth May 6, 2025 16:18
Copy link
Member

@sschuberth sschuberth left a comment

Choose a reason for hiding this comment

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

Some final nits / questions.


// Set line range only if both `sourceLocation.startLine` and `sourceLocation.endLine` are positive numbers.
// If either line is zero or negative, the rule will apply to the entire file.
if (choice.given.sourceLocation.startLine > 0 && choice.given.sourceLocation.endLine > 0) {
Copy link
Member

Choose a reason for hiding this comment

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

As the TextLocation constructor limits valid line numbers, see

require(startLine in 1..endLine || (startLine == UNKNOWN_LINE && endLine == UNKNOWN_LINE)) {
"Invalid start or end line values."
}

I believe this check can be simplified to just choice.given.sourceLocation.startLine != TextLocation.UNKNOWN_LINE.

Copy link
Member

Choose a reason for hiding this comment

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

Note that if changing the code, the comment should be slightly adjusted as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Replaced hardcoded choice.given.sourceLocation.startLine > 0 checks with UNKNOWN_LINE constants for better semantics.

I kept both checks for readability, but I'm happy to simplify to a single check if you prefer that approach

Copy link
Member

@sschuberth sschuberth May 7, 2025

Choose a reason for hiding this comment

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

I kept both checks for readability, but I'm happy to simplify to a single check if you prefer that approach

I wasn't super happy with the single check either, as it reads a bit odd if don't know the internals. I was also thinking about introducing this. How about that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was thinking also to introduce a method like you did on #10280. Is that PR going to be merge today?

Copy link
Member

Choose a reason for hiding this comment

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

Is that PR going to be merge today?

I'm confident at it will.

@isasmendiagus isasmendiagus force-pushed the feat/scanoss/exclusion-patterns-and-snippet-choices branch from ee1c81e to bfd7a6d Compare May 7, 2025 10:07
// A sample purl in the results.
private const val PURL_1 = "pkg:github/fakeuser/[email protected]"

class ScanOssTest : WordSpec({

Check warning

Code scanning / QDJVMC

Unused symbol

Class "ScanOssTest" is never used
This commit replaces test files containing OSS source code with randomly
generated data to prevent false positives during code scanning.

Signed-off-by: Agustin Isasmendi <[email protected]>
Implement exclusion filtering to respect path patterns specified in the
configuration. The scanner now properly excludes files matching the
patterns during the scan process.

Signed-off-by: Agustin Isasmendi <[email protected]>
@sschuberth
Copy link
Member

Please remember to rebase instead of merge to catch-up with latest main!

@isasmendiagus
Copy link
Contributor Author

Please remember to rebase instead of merge to catch-up with latest main!

Ignore please the latest merge. I'll remove it from the history

Implement snippet choice processing functionality. It handles findings
according to two different scenarios:
- Original findings that should be included
- Non-relevant findings that should be removed

The implementation converts ORT's SnippetChoices into SCANOSS-specific rule
types.

Signed-off-by: Agustin Isasmendi <[email protected]>
@isasmendiagus isasmendiagus force-pushed the feat/scanoss/exclusion-patterns-and-snippet-choices branch from ec7a4d7 to 792f5de Compare May 7, 2025 14:55
@sschuberth sschuberth enabled auto-merge (rebase) May 7, 2025 14:57
@sschuberth sschuberth merged commit 77293c6 into oss-review-toolkit:main May 7, 2025
23 of 24 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.

2 participants