fix(PurlUtils): Add missing Debian Purl type to getPurlType
#7620
Conversation
Signed-off-by: Nicolas Nobelis <nicolas.nobelis@bosch.io>
| @@ -65,6 +65,7 @@ enum class PurlType(private val value: String) { | |||
| fun Identifier.getPurlType() = | |||
| when (type.lowercase()) { | |||
| "bower" -> PurlType.BOWER | |||
| "deb" -> PurlType.DEBIAN | |||
There was a problem hiding this comment.
Can you please indicate why you think this value is missing? (I believe so far this mapping was limited keys which are actually used by package managers within ORT.)
There was a problem hiding this comment.
At Bosch, we have a package manager plugin for Debian.
It used previously createPurl and I plan to migrate to toPurl following the merge of #7601.
toPurl calls this getPurlType function.
You wrote that getPurlType is meant to contains only package managers supported by ORT. In this case, we have a proprietary package manager plugin. How do you see it coexist with the rule you described in your comment ?
There was a problem hiding this comment.
You wrote that getPurlType is meant to
Actually, I didn't mean "is meant to" but I meant: it currently only contains values from the package managers in ORT.
How do you see it coexist with the rule you described in your comment ?
ORT has a mechanism to plug-in package managers, but lacks a mechanism for plugging in logic related to the ecosystem that package manager uses, e.g. Identifier.type. In my view, that should be a plug-in as well for the exact same reason the package manager has been chosen to be a plugin. However, we do not have such mechanism yet.
There are further places in the code which would need to be considered.
For example, OSV has a mapping from Identifier.type to OSV ecosystem constants. So, currently OSV cannot be used for plugin package managers which would require extension of that mapping.
In my view, it would make sense to define start a dedicated PackageManagagerPlugin interface, which would contain (very roughly spkeaking):
- The existing factory method, currently called
PackageManagerFactory.create() - A function which gives the purlType to the corresponding type (optional to implement)
- A function which gives the OSV ecosystem to the corresponding type
What do other guys think? @oss-review-toolkit/core-devs
There was a problem hiding this comment.
However, we do not have such mechanism yet.
Exactly. We had discussed that in past, and I have several branches on roughly that topic lying around (identifier-types, multiple-identifier-classes) that also try to deal with the problem that a package manager's project type does not need to match with it package type(s).
So for package manager that support multiple package types, like Pub, we'd even need a set of package types / PURLs / OSV ecosystem coordinates.
There was a problem hiding this comment.
@sschuberth not sure if I get your point. Do you believe it's fine to add mappings for non-existing types now?
There was a problem hiding this comment.
not sure if I get your point.
I was just amending to your API proposal more cases that we'd need to cover with such a new API.
Do you believe it's fine to add mappings for non-existing types now?
Basically no, but the right way to do it would require a rather large refactoring, I believe. Not sure whether that's feasible to do (now). I guess we should discuss this in a Kotlin developer meeting.
There was a problem hiding this comment.
My 2 cents: for the time being I found a workaround so we can close this PR.
However this topic should definitively be discussed as it will surely arise again.
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #7620 +/- ##
============================================
- Coverage 68.03% 68.03% -0.01%
Complexity 2022 2022
============================================
Files 344 344
Lines 16725 16726 +1
Branches 2371 2371
============================================
Hits 11379 11379
- Misses 4363 4364 +1
Partials 983 983
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
|
Change not needed anymore. |
No description provided.