refactor(scanner)!: Use the configurable plugin API for scanner wrappers #7673

mnonnenmacher · 2023-10-10T21:17:58Z

Use the TypedConfigurablePluginFactory interface as base for the ScannerWrapperFactory to further align the plugin APIs. While at it, extract the logic to parse the scanner matcher properties to a central place to not have to repeat the properties in the configuration classes for all scanner wrappers. This also makes it possible to use Unit as configuration class for scanners that do not offer any configuration options.

As this change makes the configuration classes of the scanner wrappers part of the public API, they must not be internal anymore.

Scanners that provide secret configuration options will be migrated to read them from the secrets map instead of the options map in later commits.

Breaking API change:
The ScannerWrapperFactory API was changed so all scanner wrapper plugins need to be adapted. See the PR diff for examples.

codecov · 2023-10-11T07:04:08Z

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (5cdf8ce) 68.06% compared to head (9577b50) 68.06%.

Additional details and impacted files

@@            Coverage Diff            @@
##               main    #7673   +/-   ##
=========================================
  Coverage     68.06%   68.06%           
  Complexity     2023     2023           
=========================================
  Files           344      344           
  Lines         16741    16741           
  Branches       2371     2371           
=========================================
  Hits          11394    11394           
  Misses         4366     4366           
  Partials        981      981

Flag	Coverage Δ
funTest-non-docker	`36.43% <100.00%> (ø)`
test	`35.61% <100.00%> (ø)`

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

Files	Coverage Δ
.../src/main/kotlin/storages/ClearlyDefinedStorage.kt	`83.14% <100.00%> (ø)`

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

sschuberth

I believe reference.yml should also be adjusted for the scanner options.

sschuberth · 2023-10-11T06:52:21Z

plugins/scanners/scancode/src/main/kotlin/ScanCodeConfig.kt

+        private const val COMMAND_LINE_PROPERTY = "commandLine"
+        private const val COMMAND_LINE_NON_CONFIG_PROPERTY = "commandLineNonConfig"
+
+        fun create(options: Options) =
+            ScanCodeConfig(options[COMMAND_LINE_PROPERTY], options[COMMAND_LINE_NON_CONFIG_PROPERTY])


Nit: Strictly speaking IMO the knowledge about the names of the keys in the options map is not something the ScanCodeConfig should need to care about. All existing implementations inline this to parseConfig, which I believe is the better place, as it "glues" options to ScanCodeConfig.

Ah, now I see where this probably comes from: The ScanOss implementation already did it that way. However, I'd then prefer to inline this for ScanOss in a preceding commit.

I think the ScanOss implementation was actually inspired by the FossId config. Looking at the number of properties the FossIdConfig supports in this case there is a clear benefit in moving the parsing to a separate file. What would you think of keeping the implementation as is for now and to discuss in the next dev meeting if we want to align on one approach?

discuss in the next dev meeting

I'm fine with that, it was only a nit anyway. Please put this on the agenda then.

sschuberth · 2023-10-11T07:01:46Z

scanner/src/main/kotlin/ScannerWrapperFactory.kt

+    }
+
+    final override fun create(config: CONFIG): ScannerWrapper {
+        throw UnsupportedOperationException("Use create(CONFIG, ScannerMatcherConfig) instead.")


Nit: Maybe single-quote the function call for clarity?

Are these backticks now instead of single quotes? If I'm not mistaken so far we never use backticks in such output, so I'd prefer to stick to single quotes.

Right, I used backticks out of habit, switched to single quotes now.

sschuberth · 2023-10-11T07:07:23Z

scanner/src/test/kotlin/ScannerMatcherTest.kt

+                "other" to "value"
+            )
+
+            ScannerMatcherConfig.create(options).second shouldContainExactly mapOf("other" to "value")


Can't we uncritically combine the tests by also asserting .first here? Like

with(ScannerMatcherConfig.create(options)) { with(first) { regScannerName shouldBe "foo" minVersion shouldBe "1.2.3" maxVersion shouldBe "4.5.6" configuration shouldBe "config" } second shouldContainExactly mapOf("other" to "value") }

We could, but since the tests are super fast I like that the test name already shows which feature of the function is tested/broken.

I don't think that's worth the code duplication, but ok.

sschuberth · 2023-10-11T07:54:03Z

scanner/src/main/kotlin/ScannerMatcher.kt

+         * the properties that are used to configure the matcher.
+         */
+        fun create(options: Options): Pair<ScannerMatcherConfig, Options> {
+            val filteredOptions = options.filterKeys { it !in properties }


Another thought: Now that we have a clean separation between options and secrets, it seems a bit weird to include storage related configuration into scanner options. It probably would be cleaner to introduce a third "top-level" matcher configuration map for these. Or even move the matcher options to the storage configuration (if scan storages were plugins) as you might want to have more lenient matchers for storages that e.g. only offer older ScanCode results ("better take something than nothing"), but use more strict matchers for storages that also have newer ScanCode results.

I'm also not super happy about the tight coupling of the matcher and the wrapper config, but I would prefer to keep it for now to not further complicate the changes. In a similar direction I was thinking that it could be useful to allow different configurations for the same scanner class, like we allow it for curation providers, for example to use different settings to scan projects and packages. However, since this and the feature you suggested to have different matchers for different storages are currently theoretical use cases without a real demand I think we should discuss these ideas first in the dev or community meetings.

mnonnenmacher · 2023-10-11T09:00:45Z

I believe reference.yml should also be adjusted for the scanner options.

The scanner options are not yet changed in this PR, this will happen in the next PR which will take the secrets map into use for scanner wrappers.

sschuberth · 2023-10-11T09:32:34Z

I believe reference.yml should also be adjusted for the scanner options.

The scanner options are not yet changed in this PR, this will happen in the next PR which will take the secrets map into use for scanner wrappers.

But isn't this commit then temporarily breaking scanner matcher configuration as it is now expected to come from per-scanner options, but there are no such options yet?

mnonnenmacher · 2023-10-11T09:42:42Z

I believe reference.yml should also be adjusted for the scanner options.

The scanner options are not yet changed in this PR, this will happen in the next PR which will take the secrets map into use for scanner wrappers.

But isn't this commit then temporarily breaking scanner matcher configuration as it is now expected to come from per-scanner options, but there are no such options yet?

No, the were already part of the scanner specific options before, see:

ort/model/src/main/resources/reference.yml

Lines 207 to 218 in 5cdf8ce

    
           ScanCode: 
        
             # Command line options that affect the ScanCode output. If changed, stored scan results that were created with 
        
             # different options are not reused. 
        
             commandLine: '--copyright --license --info --strip-root --timeout 300' 
        
             # Command line options that do not affect the ScanCode output. 
        
             commandLineNonConfig: '--processes 4' 
        
             # Criteria for matching stored scan results. These can be configured for any scanner that uses semantic 
        
             # versioning. Note that the 'maxVersion' is exclusive and not part of the range of accepted versions. 
        
             minVersion: '3.2.1-rc2' 
        
             maxVersion: '32.0.0'

Use the `TypedConfigurablePluginFactory` interface as base for the `ScannerWrapperFactory` to further align the plugin APIs. While at it, extract the logic to parse the scanner matcher properties to a central place to not have to repeat the properties in the configuration classes for all scanner wrappers. This also makes it possible to use `Unit` as configuration class for scanners that do not offer any configuration options. As this change makes the configuration classes of the scanner wrappers part of the public API, they must not be `internal` anymore. Scanners that provide secret configuration options will be migrated to read them from the `secrets` map instead of the `options` map in later commits. Signed-off-by: Martin Nonnenmacher <martin.nonnenmacher@bosch.io>

mnonnenmacher added the breaking API change Pull requests that break compatibility with previous API label Oct 10, 2023

mnonnenmacher requested a review from a team as a code owner October 10, 2023 21:17

mnonnenmacher force-pushed the configurable-scanner-plugins branch from 679a7e5 to fecf57f Compare October 11, 2023 07:03

sschuberth reviewed Oct 11, 2023

View reviewed changes

mnonnenmacher force-pushed the configurable-scanner-plugins branch from fecf57f to b2e4a11 Compare October 11, 2023 08:52

mnonnenmacher force-pushed the configurable-scanner-plugins branch from b2e4a11 to 9577b50 Compare October 11, 2023 09:45

sschuberth approved these changes Oct 11, 2023

View reviewed changes

mnonnenmacher merged commit d04aeb2 into main Oct 11, 2023
22 checks passed

mnonnenmacher deleted the configurable-scanner-plugins branch October 11, 2023 10:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(scanner)!: Use the configurable plugin API for scanner wrappers #7673

refactor(scanner)!: Use the configurable plugin API for scanner wrappers #7673

mnonnenmacher commented Oct 10, 2023 •

edited

Loading

codecov bot commented Oct 11, 2023 •

edited

Loading

sschuberth left a comment

sschuberth Oct 11, 2023

sschuberth Oct 11, 2023

mnonnenmacher Oct 11, 2023

sschuberth Oct 11, 2023

sschuberth Oct 11, 2023

sschuberth Oct 11, 2023

mnonnenmacher Oct 11, 2023

sschuberth Oct 11, 2023

mnonnenmacher Oct 11, 2023

sschuberth Oct 11, 2023

sschuberth Oct 11, 2023 •

edited

Loading

mnonnenmacher Oct 11, 2023

mnonnenmacher commented Oct 11, 2023 •

edited

Loading

sschuberth commented Oct 11, 2023

mnonnenmacher commented Oct 11, 2023

refactor(scanner)!: Use the configurable plugin API for scanner wrappers #7673

refactor(scanner)!: Use the configurable plugin API for scanner wrappers #7673

Conversation

mnonnenmacher commented Oct 10, 2023 • edited Loading

codecov bot commented Oct 11, 2023 • edited Loading

Codecov Report

sschuberth 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

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 Oct 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mnonnenmacher commented Oct 11, 2023 • edited Loading

sschuberth commented Oct 11, 2023

mnonnenmacher commented Oct 11, 2023

mnonnenmacher commented Oct 10, 2023 •

edited

Loading

codecov bot commented Oct 11, 2023 •

edited

Loading

sschuberth Oct 11, 2023 •

edited

Loading

mnonnenmacher commented Oct 11, 2023 •

edited

Loading