Package curation provider plugins #6308

mnonnenmacher · 2023-01-06T14:26:20Z

Make package curation providers configurable plugins. Please see the commit messages for details.

Breaking change:
This PR adds a new section packageCurationProviders to the config.yml which is used to configure the package curation providers. The configuration of the Sw360PackageCurationProvider moved from AnalyzerConfiguration to this new section. The command line options of AnalzyerCommand to enable curation providers have been removed, instead this now needs to be configured in config.yml. See the changes to reference.yml in this PR for possible configuration options, this includes also an example for how to enable/disable a specific provider using an environment variable.

analyzer/src/main/kotlin/PackageCurationProvider.kt

sschuberth · 2023-01-06T17:49:58Z

analyzer/src/main/kotlin/PackageCurationProvider.kt

+ */
+interface PackageCurationProviderFactory<CONFIG> : ConfigurablePluginFactory<PackageCurationProvider> {
+    companion object {
+        val ALL = NamedPlugin.getAll<PackageCurationProviderFactory<*>>()


Note to myself: I was actually surprised that this (is supposed to) work(s) here as generic types get erased at compile time. But after reading through the excellent https://typealias.com/guides/star-projections-and-how-they-work/ I see how this could work.

analyzer/src/main/kotlin/curation/FilePackageCurationProvider.kt

analyzer/src/main/kotlin/curation/Sw360PackageCurationProvider.kt

analyzer/src/main/kotlin/PackageCurationProvider.kt

mnonnenmacher · 2023-01-10T10:48:37Z

@oss-review-toolkit/core-devs I decided to move the files to the new plugins folder which we discussed yesterday in a separate PR as I added some commits to clean up reference.yml and OrtConfigurationTest and didn't want to make this PR too big.

sschuberth · 2023-01-10T10:58:50Z

FYI @mnonnenmacher, OrtMainFunTest still seems to fail.

mnonnenmacher · 2023-01-10T11:26:54Z

FYI @mnonnenmacher, OrtMainFunTest still seems to fail.

Should be fixed now.

analyzer/src/main/kotlin/PackageCurationProvider.kt

sschuberth · 2023-01-10T13:15:11Z

analyzer/src/main/kotlin/PackageCurationProvider.kt

+        fun create(configurations: List<PackageCurationProviderConfiguration>) =
+            // Reverse the list so that curations from providers with higher priority are applied later and can
+            // overwrite curations from providers with lower priority.
+            configurations.filter { it.enabled }.map { ALL.getValue(it.name).create(it.config) }.reversed()


Use .asReversed() to avoid creating a copy.

I'm actually unsure about this. Personally, I would expect curation providers to be queried / curations be applied in the order I define them in the configuration, and not for the order to express a priority.

I found it unintuitive for the user that higher priority providers would come later in the list, because a user will likely not know technical details about how curations are applied. For example, if ClearlyDefined is put before SW360 it seemed clearer to me that in case of a conflict ClearlyDefined wins.

and not for the order to express a priority

It does either way.

Do we have other places in the config where order expresses priority that we could align with?

I don't think so.

Ok, I don't have a strong opinion here, it just confused me. Should we ask for other opinions from @oss-review-toolkit/kotlin-devs?

Use .asReversed() to avoid creating a copy.

I believe this is the only open remark, otherwise I'd simply approve now.

cli/src/funTest/kotlin/OrtMainFunTest.kt

model/src/test/kotlin/config/OrtConfigurationTest.kt

zhernovs · 2023-01-10T14:57:45Z

model/src/main/resources/reference.yml

+    - name: File
+      config:
+        path: '/some-path/curations.yml'
+    - name: File


Shouldn't this be "Dir" or "Directory"?

There is currently only the FilePackageCurationProvider which handles both files and dirs. I would like to split this later on, but not in this PR.

Verify that the curation directory is a directory before trying to find curation files in it. Signed-off-by: Martin Nonnenmacher <martin.nonnenmacher@bosch.io>

Add the new interface `ConfigurablePluginFactory` which can be implemented by plugins that need to be configured on creation. Signed-off-by: Martin Nonnenmacher <martin.nonnenmacher@bosch.io>

Add a `PackageCurationProviderFactory` plugin extension point. Fixes #6056. Signed-off-by: Martin Nonnenmacher <martin.nonnenmacher@bosch.io>

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

Add the model required to configure package curation providers in the ORT configuration. It will be taken into use in the following commits. Signed-off-by: Martin Nonnenmacher <martin.nonnenmacher@bosch.io>

Use the `PackageCurationProviderFactory` to create the package curation providers that are configured in the config file and remove the now unused command line options to enable package curation providers. The only exception is currently the provider that reads package curations from the repository configuration, this will be handled in a later commit. Remove the test from `OrtMainFunTest` that tests the removed `--package-curations-file` option and instead apply curations in the "Analyzer creates correct output" test. Fixes #6055. Signed-off-by: Martin Nonnenmacher <martin.nonnenmacher@bosch.io>

Align the order of top-level configuration options with the order of properties in `OrtConfiguration`. Signed-off-by: Martin Nonnenmacher <martin.nonnenmacher@bosch.io>

Verify properties in the same order as they are defined in reference.yml. Signed-off-by: Martin Nonnenmacher <martin.nonnenmacher@bosch.io>

Add verification for properties from `reference.yml` that were not verified before. Signed-off-by: Martin Nonnenmacher <martin.nonnenmacher@bosch.io>

The configuration was used to configure the `Sw360PackageCurationProvider` which is now configured in a separate section of the configuration file. Signed-off-by: Martin Nonnenmacher <martin.nonnenmacher@bosch.io>

zhernovs · 2023-01-11T11:02:28Z

Just curious, if --package-curations-dir and --package-curations-file options were deprecated for AnalyzerCommand, shouldn't it be also deprecated for EvaluatorCommand?

sschuberth · 2023-01-11T11:43:37Z

Just curious, if --package-curations-dir and --package-curations-file options were deprecated for AnalyzerCommand, shouldn't it be also deprecated for EvaluatorCommand?

In the AnalyzerCommand these options were not deprecated, but removed, because there's now a new way to configure them. In the EvaluatorCommand, however, these options are used to override the configured curations, so these options should be kept.

mnonnenmacher · 2023-01-11T16:25:30Z

On the long term the EvaluatorCommand options should also be removed because it's inconsistent that not all curation providers are supported. Instead, a separate command to replace existing curations would be a better option, also see #6188.

github-advanced-security bot found potential problems Jan 6, 2023

View reviewed changes

analyzer/src/main/kotlin/PackageCurationProvider.kt Fixed Show fixed Hide fixed

mnonnenmacher force-pushed the package-curation-provider-plugins branch 2 times, most recently from 0d7f05f to 5155072 Compare January 6, 2023 16:43

sschuberth requested changes Jan 6, 2023

View reviewed changes

sschuberth reviewed Jan 6, 2023

View reviewed changes

analyzer/src/main/kotlin/PackageCurationProvider.kt Show resolved Hide resolved

analyzer/src/main/kotlin/PackageCurationProvider.kt Show resolved Hide resolved

mnonnenmacher force-pushed the package-curation-provider-plugins branch 3 times, most recently from a099b06 to faccfa6 Compare January 10, 2023 10:41

mnonnenmacher added breaking change Pull requests that break compatibility with previous behavior release notes Changes that should be mentioned in release notes labels Jan 10, 2023

mnonnenmacher marked this pull request as ready for review January 10, 2023 10:47

mnonnenmacher requested review from a team and oheger-bosch as code owners January 10, 2023 10:47

mnonnenmacher requested a review from sschuberth January 10, 2023 10:50

mnonnenmacher force-pushed the package-curation-provider-plugins branch from faccfa6 to 55d7695 Compare January 10, 2023 11:26

mnonnenmacher force-pushed the package-curation-provider-plugins branch from 55d7695 to 9cb0765 Compare January 10, 2023 11:31

sschuberth requested changes Jan 10, 2023

View reviewed changes

mnonnenmacher force-pushed the package-curation-provider-plugins branch from 9cb0765 to cdbe68f Compare January 10, 2023 14:35

zhernovs reviewed Jan 10, 2023

View reviewed changes

mnonnenmacher added 8 commits January 11, 2023 07:58

FilePackageCurationProvider: Verify the curation directory

eddd815

Verify that the curation directory is a directory before trying to find curation files in it. Signed-off-by: Martin Nonnenmacher <martin.nonnenmacher@bosch.io>

PluginManager: Add a ConfigurablePluginFactory

83229cb

Add the new interface `ConfigurablePluginFactory` which can be implemented by plugins that need to be configured on creation. Signed-off-by: Martin Nonnenmacher <martin.nonnenmacher@bosch.io>

analyzer: Add a PackageCurationProviderFactory

0d10fb8

Add a `PackageCurationProviderFactory` plugin extension point. Fixes #6056. Signed-off-by: Martin Nonnenmacher <martin.nonnenmacher@bosch.io>

OrtConfiguration: Add a missing space

4937f49

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

model: Add a configuration model for package curation providers

d38b483

Add the model required to configure package curation providers in the ORT configuration. It will be taken into use in the following commits. Signed-off-by: Martin Nonnenmacher <martin.nonnenmacher@bosch.io>

reference.yml: Align order with OrtConfiguration class

aac6f5b

Align the order of top-level configuration options with the order of properties in `OrtConfiguration`. Signed-off-by: Martin Nonnenmacher <martin.nonnenmacher@bosch.io>

OrtConfigurationTest: Align order of verification with reference.yml

588c696

Verify properties in the same order as they are defined in reference.yml. Signed-off-by: Martin Nonnenmacher <martin.nonnenmacher@bosch.io>

mnonnenmacher added 2 commits January 11, 2023 07:58

OrtConfigurationTest: Add missing verifications for reference.yml

fd95d54

Add verification for properties from `reference.yml` that were not verified before. Signed-off-by: Martin Nonnenmacher <martin.nonnenmacher@bosch.io>

AnalyzerConfiguration: Remove unused SW360 configuration

6538a3b

The configuration was used to configure the `Sw360PackageCurationProvider` which is now configured in a separate section of the configuration file. Signed-off-by: Martin Nonnenmacher <martin.nonnenmacher@bosch.io>

mnonnenmacher force-pushed the package-curation-provider-plugins branch from cdbe68f to 6538a3b Compare January 11, 2023 06:59

mnonnenmacher requested a review from sschuberth January 11, 2023 06:59

sschuberth approved these changes Jan 11, 2023

View reviewed changes

mnonnenmacher merged commit 498a298 into main Jan 11, 2023

mnonnenmacher deleted the package-curation-provider-plugins branch January 11, 2023 08:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Package curation provider plugins #6308

Package curation provider plugins #6308

mnonnenmacher commented Jan 6, 2023 •

edited

sschuberth Jan 6, 2023

mnonnenmacher commented Jan 10, 2023 •

edited

sschuberth commented Jan 10, 2023

mnonnenmacher commented Jan 10, 2023

sschuberth Jan 10, 2023

sschuberth Jan 10, 2023

mnonnenmacher Jan 10, 2023

sschuberth Jan 10, 2023

mnonnenmacher Jan 10, 2023

sschuberth Jan 10, 2023

sschuberth Jan 10, 2023

mnonnenmacher Jan 11, 2023

zhernovs Jan 10, 2023

mnonnenmacher Jan 10, 2023

zhernovs commented Jan 11, 2023

sschuberth commented Jan 11, 2023

mnonnenmacher commented Jan 11, 2023

Package curation provider plugins #6308

Package curation provider plugins #6308

Conversation

mnonnenmacher commented Jan 6, 2023 • edited

Choose a reason for hiding this comment

mnonnenmacher commented Jan 10, 2023 • edited

sschuberth commented Jan 10, 2023

mnonnenmacher commented Jan 10, 2023

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhernovs commented Jan 11, 2023

sschuberth commented Jan 11, 2023

mnonnenmacher commented Jan 11, 2023

mnonnenmacher commented Jan 6, 2023 •

edited

mnonnenmacher commented Jan 10, 2023 •

edited