Make issues for unmapped licenses consider concluded licenses #4082
Conversation
| @@ -49,15 +49,17 @@ data class CuratedPackage( | |||
| /** | |||
| * Check if this package contains any erroneous data. | |||
| */ | |||
| fun collectIssues(): List<OrtIssue> = | |||
| pkg.declaredLicensesProcessed.unmapped.map { unmappedLicense -> | |||
| fun collectIssues(): List<OrtIssue> { | |||
There was a problem hiding this comment.
The issue is just a warning. I believe it's better to not do this change to keep the default behavior simple and thus easier to understand for the user. I suspect that the behavior 'as-is' before and after this change anyway does not match the needs of all users.
Instead of adding more complexity here I propose to (1) add a policy rule to our defaults which implements your idea with a rule violation and to (2) optionally eliminate creating this ORT issue completely.
(In general I think we should move towards reporting issues via rule violations where applicable. It's already valuable for just customizing the severity, let alone the added flexibility)
What do you think?
There was a problem hiding this comment.
Leaving this as-is is not an option for us. We have an "everything must be green" mandate, so also warnings need to be addressed.
While we could have gotten rid of the warning by creating a mapping as part of a curation, there is little reason to create such a mapping if its result is anyway going to be overridden by a concluded license.
But I do see some more options:
- lower the severity to hint (which for us results in a "green" run) if a concluded license is set
- only log a warning for this, but not create an issue if a concluded license is set
How about these?
There was a problem hiding this comment.
In general, I like the approach to put more of such checking logic into rules, where it can be customized according to the users' needs.
In this special case, I find the implementation of this PR more intuitive compared to the current state. The user might have explicitly added a curation for a concluded license to get rid off these warnings.
There was a problem hiding this comment.
The fact that Bosch fixes all warning and HERE doesn't makes it hard to make a good proposal.
I believe it indicates that severity should be under user control, so that everybody can decide what exactly is a must fix.
Anyhow, the only additional idea I'd throw into the basket of options is the following (which tries to avoid adding complexity into ORT)
- Turn this issue into a hint
- Implement a rule violation with your desired (more complex) logic
_edit: On the other hand warning seems just right, so I can't see any short term options which is ideal imo. I'd actually prefer @sschuberth 's proposal to "lower the severity to hint (which for us results in a "green" run) if a concluded license is set" _
There was a problem hiding this comment.
On the other hand warning seems just right, so I can't see any short term options which is ideal imo. I'd actually prefer @sschuberth 's proposal to "lower the severity to hint (which for us results in a "green" run) if a concluded license is set"
Ok, I'll make the change, as I also like to have a warning for this in the general case.
This is a preparation to take properties of a CuratedPackage into account in this function. Signed-off-by: Sebastian Schuberth <[email protected]>
If a concluded license is set, declared licenses are likely not taken into account at all (depending on policy rules), so lower the severity of unmapped declared licneses in this case. Fixes #4081. Signed-off-by: Sebastian Schuberth <[email protected]>
sschuberth
force-pushed
the
unmapped-consider-concluded
branch
from
fe5fa32 to
7c6a80e
Compare
|
I'll work on the |
Please have a look at the individual commit messages for the details.