Various FossID refactorings #9728

mnonnenmacher · 2025-01-12T19:31:13Z

Various FossID refactorings to prepare for simplifying the configuration. See the commit messages for details.

codecov · 2025-01-12T19:44:36Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 68.09%. Comparing base (b85af8b) to head (59f559e).

Additional details and impacted files

@@            Coverage Diff            @@
##               main    #9728   +/-   ##
=========================================
  Coverage     68.09%   68.09%           
  Complexity     1294     1294           
=========================================
  Files           249      249           
  Lines          8846     8846           
  Branches        923      923           
=========================================
  Hits           6024     6024           
  Misses         2433     2433           
  Partials        389      389

Flag	Coverage Δ
test-ubuntu-24.04	`35.89% <ø> (ø)`

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.

sschuberth · 2025-01-12T19:53:20Z

plugins/scanners/fossid/src/main/kotlin/FossIdNamingProvider.kt

@@ -116,8 +116,8 @@ class FossIdNamingProvider(
    }

    /**
-     * Replaces non-standard characters in branch name and trims its length to one that will not exceed
-     * maximum length of FossID scan ID, when combined with the rest of variables
+     * Replace characters in [branch] not matching `[a-zA-Z0-9-_]` with underscores and trim its length so that the


Instead of repeating the regex here, how about extracting it to a constant (which anyway would be good to avoid recompiling it on each call), and referring to that constant here instead?

While at it, the regex could be simplified to [^\w-].

I have added a commit on top for that.

sschuberth · 2025-01-12T19:56:51Z

plugins/scanners/fossid/src/main/kotlin/FossIdNamingProvider.kt

@@ -37,7 +37,8 @@ import org.apache.logging.log4j.kotlin.logger
 * * **currentTimestamp**: The current time.
 * * **deltaTag** (scan code only): If delta scans are enabled, this qualifies the scan as an *origin* scan or a *delta*
 * scan.
- * * **branch**: The branch name (revision) to scan.
+ * * **branch**: The branch name (revision) to scan. Characters not matching `[a-zA-Z0-9-_]` are replaced with


Also here, I wonder whether hard-coding the regex really makes sense. Maybe just say a bit lax "non-word characters", even if it does not exactly match \w only, but also excludes -?

I have reworded the sentence to also explain the reason.

nnobelis

LGTM

nnobelis · 2025-01-13T06:08:26Z

plugins/scanners/fossid/src/main/kotlin/FossIdNamingProvider.kt

        namingProjectPattern?.let {
            val builtins = mapOf(
-                "#projectName" to projectName
+                "#repositoryName" to repositoryName


Should we put the "breaking change" label on the PR ?

I don't remember ORT's policy.

This should only be a breaking change if users are required to change their configuration. But my understanding is they don't, or?

Yes they do (and I guess that's why @mnonnenmacher marked is as breaking in the commit message).

One of the builtin naming variable has been renamed. If ORT users are using it, they need to adapt their naming pattern with the new name.
For instance at Bosch we do.

Should we put the "breaking change" label on the PR ?

Didn't we stop doing this when we started publishing release notes?

There will be more breaking changes in a follow-up, I just didn't include them here because I could not yet get the tests to work, but my hope is that I can get them in before the next release.

Didn't we stop doing this when we started publishing release notes?

That's why I was asking, I was unsure.

Ah, sorry, I was somehow missing the "label on the PR" part. No, we don't do that anymore.

sschuberth · 2025-01-13T10:53:26Z

There are real test failures now.

Signed-off-by: Martin Nonnenmacher <martin.nonnenmacher@bosch.com>

Rename the `projectName` variable used by the `FossIdNamingProvider` to `repositoryName` to make clear where it is coming from and to better distinguish it from the `namingProjectPattern`. Signed-off-by: Martin Nonnenmacher <martin.nonnenmacher@bosch.com>

Signed-off-by: Martin Nonnenmacher <martin.nonnenmacher@bosch.com>

This simplifies calling the function. Signed-off-by: Martin Nonnenmacher <martin.nonnenmacher@bosch.com>

This simplifies an upcoming change. Signed-off-by: Martin Nonnenmacher <martin.nonnenmacher@bosch.com>

Calculating the values is cheap, so add them always to simplify the code. Signed-off-by: Martin Nonnenmacher <martin.nonnenmacher@bosch.com>

With the recent refactorings, the `createScanCodeForDefaultPattern` and `createScanCodeForCustomPattern` are almost identical, so inline them into `createScanCode` to reduce duplication. Signed-off-by: Martin Nonnenmacher <martin.nonnenmacher@bosch.com>

Signed-off-by: Martin Nonnenmacher <martin.nonnenmacher@bosch.com>

The function is only used internally and from tests. Signed-off-by: Martin Nonnenmacher <martin.nonnenmacher@bosch.com>

Rename `convertGitUrlToProjectName` to `extractRepositoryName` to better describe what the function does. While at it, also improve the function docs and optimize the code a bit. Signed-off-by: Martin Nonnenmacher <martin.nonnenmacher@bosch.com>

While at it, also simplify the expression by using `\w`. Signed-off-by: Martin Nonnenmacher <martin.nonnenmacher@bosch.com>

mnonnenmacher requested a review from a team as a code owner January 12, 2025 19:31

sschuberth reviewed Jan 12, 2025

View reviewed changes

sschuberth requested a review from nnobelis January 12, 2025 19:59

nnobelis reviewed Jan 13, 2025

View reviewed changes

mnonnenmacher force-pushed the simplify-fossid-config branch from a7bff56 to 2196bf4 Compare January 13, 2025 10:24

mnonnenmacher requested review from sschuberth and nnobelis January 13, 2025 10:25

mnonnenmacher added 12 commits January 13, 2025 18:05

docs(fossid): Fix issues in FossIdNamingProvider docs

cd3f9b1

Signed-off-by: Martin Nonnenmacher <martin.nonnenmacher@bosch.com>

docs(fossid): Improve docs of normalizeBranchName

f1dd2d0

Signed-off-by: Martin Nonnenmacher <martin.nonnenmacher@bosch.com>

docs(fossid): Improve docs of the built-in branch naming variable

d17034f

Signed-off-by: Martin Nonnenmacher <martin.nonnenmacher@bosch.com>

refactor(fossid): Remove #branch inside of normalizeBranchName

59d9dcd

This simplifies calling the function. Signed-off-by: Martin Nonnenmacher <martin.nonnenmacher@bosch.com>

refactor(fossid): Extract logic to build default naming pattern

cf0e878

This simplifies an upcoming change. Signed-off-by: Martin Nonnenmacher <martin.nonnenmacher@bosch.com>

refactor(scancode): Always add all built-in variables

aac87d5

Calculating the values is cheap, so add them always to simplify the code. Signed-off-by: Martin Nonnenmacher <martin.nonnenmacher@bosch.com>

chore(fossid): Improve error message for too long scan code

b2be7ee

Signed-off-by: Martin Nonnenmacher <martin.nonnenmacher@bosch.com>

refactor(fossid): Make convertGitUrlToProjectName internal

6918bda

The function is only used internally and from tests. Signed-off-by: Martin Nonnenmacher <martin.nonnenmacher@bosch.com>

refactor(fossid): Extract a regular expression to a constant

59f559e

While at it, also simplify the expression by using `\w`. Signed-off-by: Martin Nonnenmacher <martin.nonnenmacher@bosch.com>

mnonnenmacher force-pushed the simplify-fossid-config branch from 2196bf4 to 59f559e Compare January 13, 2025 17:05

mnonnenmacher enabled auto-merge (rebase) January 13, 2025 17:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Various FossID refactorings #9728

Various FossID refactorings #9728

mnonnenmacher commented Jan 12, 2025

codecov bot commented Jan 12, 2025 •

edited

Loading

sschuberth Jan 12, 2025

sschuberth Jan 12, 2025

mnonnenmacher Jan 13, 2025

sschuberth Jan 12, 2025 •

edited

Loading

mnonnenmacher Jan 13, 2025

nnobelis left a comment

nnobelis Jan 13, 2025

sschuberth Jan 13, 2025

nnobelis Jan 13, 2025

mnonnenmacher Jan 13, 2025

nnobelis Jan 13, 2025

sschuberth Jan 13, 2025

sschuberth commented Jan 13, 2025

Various FossID refactorings #9728

Are you sure you want to change the base?

Various FossID refactorings #9728

Conversation

mnonnenmacher commented Jan 12, 2025

codecov bot commented Jan 12, 2025 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sschuberth Jan 12, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nnobelis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sschuberth commented Jan 13, 2025

codecov bot commented Jan 12, 2025 •

edited

Loading

sschuberth Jan 12, 2025 •

edited

Loading