Migrate Advisor plugins to configurable plugin API #7690
Conversation
mnonnenmacher
force-pushed
the
advisor-secret-options
branch
from
c349cff to
611a78a
Compare
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #7690 +/- ##
============================================
- Coverage 68.06% 67.90% -0.17%
- Complexity 2025 2033 +8
============================================
Files 345 350 +5
Lines 16736 16747 +11
Branches 2366 2368 +2
============================================
- Hits 11392 11372 -20
- Misses 4366 4397 +31
Partials 978 978
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
mnonnenmacher
force-pushed
the
advisor-secret-options
branch
from
611a78a to
e09b2b4
Compare
mnonnenmacher
requested review from
MarcelBochtler,
oheger-bosch and
a team
as code owners
| token = secrets["token"], | ||
| endpointUrl = options["endpointUrl"], | ||
| labelFilter = options["labelFilter"]?.split(",")?.map { it.trim() } | ||
| ?: listOf("!duplicate", "!enhancement", "!invalid", "!question", "*"), |
There was a problem hiding this comment.
This now duplicates the default value of GitHubDefectsConfiguration.labelFilter, which IMO is not so nice. In my old draft I tried to solve this by extracting default values to constants (not fully implemented, also see the last commit in that PR).
This raises the general question where default values should be applied. I see basically tree options:
- in
parseConfig(), - in the constructor of the output config class,
- in the consumer of the config class.
My current preference would be 1., and I'd propose a clean-up commit that removes the default parameters from GitHubDefectsConfiguration's constructor.
There was a problem hiding this comment.
Would you be fine with doing that clean up commit outside of this PR?
There was a problem hiding this comment.
Yes, if it's resolved within reasonable time, before we forget about it.
Use the `TypedConfigurablePluginFactory` as base class for the `AdviceProviderFactory`. This decouples the configuration model from any advisor specific configuration classes and is a precondition for moving advisors to their own plugin projects. The advisor specific configuration properties in the `AdvisorConfiguration` are replaced with a map similar to the one used for configuring scanner wrapper plugins and the `reference.yml` is updated accordingly. While at it, also sort the advice providers in the `reference.yml` alphabetically. Tests that were testing the behavior of the previous implementation are removed. Signed-off-by: Martin Nonnenmacher <martin.nonnenmacher@bosch.io>
Since the previous commit the model does not require the advisor specific configuration classes anymore. Move them to the advisor module to prepare for moving the advice provider implementations to their own plugin modules. Signed-off-by: Martin Nonnenmacher <martin.nonnenmacher@bosch.io>
Remove the Jackson annotation from the advice provider configuration classes as the configurations are not used for serialization anymore. Signed-off-by: Martin Nonnenmacher <martin.nonnenmacher@bosch.io>
mnonnenmacher
force-pushed
the
advisor-secret-options
branch
from
e09b2b4 to
48ea526
Compare
| */ | ||
| @JsonProperty(access = JsonProperty.Access.WRITE_ONLY) | ||
| val apiKey: String? = null | ||
| val config: Map<String, PluginConfiguration>? = null |
There was a problem hiding this comment.
Note to myself: Let's discuss later if this class could be inlined to get rid of the double config nesting.
|
Merged as failing |
|
Once again this is a breaking change that breaks our plugins. Please add the corresponding tags to the PR. |
@nnobelis Conventional commits and release notes generated from them by now have superseded PR labels in this regard. Please take a look at the "Breaking Changes" section for a release instead (this time it was not forgotten to mark commits accordingly).
Anyway, this PR actually was labeled accordingly. |
|
Ah yes true my mistake. Could we, as a convention, put also something in the PR title so we can have a quick oversight when browsing the Github notifications ? |
I'm not a fan of this as it's prone to typos etc., and we already have release notes, PR labels, and a note in the PR's top post; we can't put it just everywhere. |
Firstly, VulnerableCode can be used without an API key now [1], and secondly the code to configure the API key was outdated anyway as it was not adjusted to [2]. Simply remove the code to address this. If the need to specify an API key should reoccur, the feature introduced in 738790c should be used to set it via an environment variable that is being used in a `config.yml` template. [1]: aboutcode-org/vulnerablecode#1352 [2]: #7690 Signed-off-by: Sebastian Schuberth <sebastian@doubleopen.org>
Firstly, VulnerableCode can be used without an API key now [1], and secondly the code to configure the API key was outdated anyway as it was not adjusted to [2]. Simply remove the code to address this. If the need to specify an API key should reoccur, the feature introduced in 738790c should be used to set it via an environment variable that is being used in a `config.yml` template. [1]: aboutcode-org/vulnerablecode#1352 [2]: #7690 Signed-off-by: Sebastian Schuberth <sebastian@doubleopen.org>
Migrate the advisor plugins to use the configurable plugin API. See the commit messages for details.
Breaking change:
The configuration of advice providers has changed, see the diff of
reference.ymlfor examples.Breaking API change:
The API of the
AdviceProviderFactoryhas changed.