Skip to content

fix(fossid-webapp): Deduplicate and normalize ignore rules #10657

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

wkl3nk
Copy link
Contributor

@wkl3nk wkl3nk commented Jul 25, 2025

Deduplicate and normalize ignore rules by removing the trailing "/*" from directory rules and also remove duplicate directory,file and extension rules.
This prevents a problem where FossID reports an error when ignore rules are created that already exist.

"Scanner job '177452' failed."
java.lang.IllegalArgumentException: Could not 'create ignore rules'. Additional information: Rule already exists.

@wkl3nk wkl3nk requested a review from a team as a code owner July 25, 2025 14:22
Copy link

codecov bot commented Jul 25, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 57.30%. Comparing base (7aca3ba) to head (e4c52bf).

Additional details and impacted files
@@             Coverage Diff              @@
##               main   #10657      +/-   ##
============================================
+ Coverage     57.25%   57.30%   +0.04%     
  Complexity     1646     1646              
============================================
  Files           341      341              
  Lines         12692    12705      +13     
  Branches       1199     1202       +3     
============================================
+ Hits           7267     7280      +13     
  Misses         4965     4965              
  Partials        460      460              
Flag Coverage Δ
funTest-docker 71.28% <ø> (+0.04%) ⬆️
test-ubuntu-24.04 41.88% <100.00%> (+0.07%) ⬆️
test-windows-2022 41.86% <100.00%> (+0.07%) ⬆️

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.

@wkl3nk wkl3nk force-pushed the wkl3nk/deduplicate-and-normalize-ignore-rules branch from d77c6fa to 46ec571 Compare July 28, 2025 06:56
@wkl3nk wkl3nk force-pushed the wkl3nk/deduplicate-and-normalize-ignore-rules branch 2 times, most recently from b3d1f6b to d4a3b61 Compare July 28, 2025 07:17
@wkl3nk wkl3nk force-pushed the wkl3nk/deduplicate-and-normalize-ignore-rules branch from d4a3b61 to 6448835 Compare July 28, 2025 07:25

/**
* Deduplicate and normalize ignore rules [allRules] by removing duplicate directory and file rules, and by stripping
* the trailing "/ *" from directory rule values.
Copy link
Member

Choose a reason for hiding this comment

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

Looks like there a space too much in between "/ *"...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is intentionally. If you don't add a space, then it is interpreted as the beginning of a new comment. I tried to find a solution, but adding a space is the easiest option that was returned in my searches.

Copy link
Member

Choose a reason for hiding this comment

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

Then I propose to spell it out instead, like "the trailing slash and asterisk".

Copy link
Member

@mnonnenmacher mnonnenmacher Jul 28, 2025

Choose a reason for hiding this comment

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

@wkl3nk wkl3nk force-pushed the wkl3nk/deduplicate-and-normalize-ignore-rules branch from 6448835 to d2ec1b2 Compare July 28, 2025 08:02
sschuberth
sschuberth previously approved these changes Jul 28, 2025
@sschuberth sschuberth enabled auto-merge (rebase) July 28, 2025 08:06
nnobelis
nnobelis previously approved these changes Jul 28, 2025
@sschuberth
Copy link
Member

Looks like some test needs to be adjusted.

auto-merge was automatically disabled July 28, 2025 09:46

Head branch was pushed to by a user without write access

@wkl3nk wkl3nk force-pushed the wkl3nk/deduplicate-and-normalize-ignore-rules branch from d2ec1b2 to 862591a Compare July 28, 2025 09:46
@wkl3nk
Copy link
Contributor Author

wkl3nk commented Jul 28, 2025

@sschuberth @mnonnenmacher Unfortunately the tests are still failing, as you already noticed. Please consider to merge it nevertheless, as we would like to have a new ORT release as soon as possible, as this issue currently blocks our FossID scans.

@sschuberth
Copy link
Member

sschuberth commented Jul 28, 2025

@sschuberth @mnonnenmacher Unfortunately the tests are still failing, as you already noticed. Please consider to merge it nevertheless, as we would like to have a new ORT release as soon as possible, as this issue currently blocks our FossID scans.

@wkl3nk, the test I was linking indicates there's a real (mocking) issue caused by your changes. So just rebasing does not help. You need to adjust the test.

Deduplicate and normalize ignore rules by removing the trailing
"/*" from directory rules and also remove duplicate directory,
file and extension rules.
This prevents a problem where FossID reports an error when
ignore rules are created that already exist.

Signed-off-by: klw1imb <[email protected]>
@wkl3nk wkl3nk dismissed stale reviews from sschuberth and nnobelis via e4c52bf July 28, 2025 13:19
@wkl3nk wkl3nk force-pushed the wkl3nk/deduplicate-and-normalize-ignore-rules branch from 862591a to e4c52bf Compare July 28, 2025 13:19
@sschuberth sschuberth enabled auto-merge (rebase) July 28, 2025 13:24
@wkl3nk
Copy link
Contributor Author

wkl3nk commented Jul 28, 2025

@sschuberth Oh sorry, yes, a test was failing and I did not recognized. I adapted the code now.

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.

4 participants