Prepare advisor module for supporting multiple advisors in one run #3761
Conversation
KorSin
requested review from
fviernau,
MarcelBochtler,
mnonnenmacher,
oheger-bosch and
sschuberth
as code owners
| ).convert { advisorName -> | ||
| allAdvisorsByName[advisorName.toUpperCase()] | ||
| ?: throw BadParameterValue("Advisor '$advisorName' is not one of ${allAdvisorsByName.keys}") | ||
| allVulnerabilityProvidersByName[advisorName.toUpperCase()] |
There was a problem hiding this comment.
There is again some confusion regarding the terms Advisor and Vulnerability provider. Not sure, which ones should be used for the user interface. But I guess, as long as there is only a single provider, we can stick to the term Advisor. Only if the user can enable multiple providers, this term should be used.
There was a problem hiding this comment.
To clarify: "Advisor" is supposed to be the umbrella term; a "vulnerability provider" is a source that advises about security issues. But there are plans to also advise about completely different aspects, like the quality of an Open Source project / its community health. Such feedback should still be implemented as an advisor (in my current thinking; if that does not make the data model too heterogeneous).
There was a problem hiding this comment.
@oheger-bosch Hmm technically the names here should also be changed such as the --advisor option, but I did not want to make this a breaking change. Should we just stick to it for now?
There was a problem hiding this comment.
@sschuberth But is this additional "Advisor" task supposed to be done by, e.g, NexusIq as well or other services. If it is the latter should they later also be called in the CLI as advisors ?
There was a problem hiding this comment.
@oheger-bosch Hmm technically the names here should also be changed such as the
--advisoroption, but I did not want to make this a breaking change. Should we just stick to it for now?
Yes, I would say we keep the names for now.
KorSin
force-pushed
the
korsin/multiple-advisors
branch
from
939e51d to
14286dd
Compare
| ).convert { advisorName -> | ||
| allAdvisorsByName[advisorName.toUpperCase()] | ||
| ?: throw BadParameterValue("Advisor '$advisorName' is not one of ${allAdvisorsByName.keys}") | ||
| allVulnerabilityProvidersByName[advisorName.toUpperCase()] |
There was a problem hiding this comment.
@oheger-bosch Hmm technically the names here should also be changed such as the
--advisoroption, but I did not want to make this a breaking change. Should we just stick to it for now?
Yes, I would say we keep the names for now.
KorSin
force-pushed
the
korsin/multiple-advisors
branch
from
14286dd to
4ced35f
Compare
Use a map to access an advisor directly by its name. Signed-off-by: Korbinian Singhammer <[email protected]>
KorSin
force-pushed
the
korsin/multiple-advisors
branch
from
4ced35f to
9d76059
Compare
KorSin
force-pushed
the
korsin/multiple-advisors
branch
from
9d76059 to
978df32
Compare
sschuberth
changed the title
Signed-off-by: Korbinian Singhammer <[email protected]>
In the following commit, vulnerability providers, such as NexusIq, are going to implement the VulnerabilitProvider class instead of the Advisor class. Thus, rename the factory to match the name. Signed-off-by: Korbinian Singhammer <[email protected]>
Let vulnerabilty providers, e.g. NexusIq, implement the VulnerabilityProvider class. Move responsibility to create providers to the Advisor class, which can then call the provider. This is a preparation to let the Advisor handle multiple providers. Signed-off-by: Korbinian Singhammer <[email protected]>
KorSin
force-pushed
the
korsin/multiple-advisors
branch
from
978df32 to
b93553b
Compare
I think it is a good idea to split #3638 into multiple PRs.
This PR is a preparation of allowing multiple advisors during a single run.
I am following the idea of the analyzer: The
Analyzerclass has a single instance and isto call all the demanded
PackageManagers.For the advisor, the
Advisorclass should also be a single instance that calls several providers of vulnerabilities.Thus, a new class is created called
VulnerabilityProvider, from whichNexusIqandVulnerablerCodeinherit.The PR implements the basics of such a structure, but does not change the logic of calling only one
VulnerabilityProviderso this should not be a breaking change. In a following PR, this should then be extended to multiple providers.