-
Notifications
You must be signed in to change notification settings - Fork 361
evaluator: Introduce the option to evaluate a licenseRule for all license sources at once #11154
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
734e7b1 to
7c3325f
Compare
1e4d928 to
260566b
Compare
00d5186 to
08ced1a
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #11154 +/- ##
=========================================
Coverage 57.43% 57.43%
- Complexity 1701 1703 +2
=========================================
Files 346 346
Lines 12845 12845
Branches 1222 1222
=========================================
Hits 7378 7378
Misses 4994 4994
Partials 473 473
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
b3e3164 to
819ea09
Compare
sschuberth
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the general idea of this change.
|
|
||
| /** | ||
| * The source of the license. | ||
| * The license sources to evaluate the license for. Must not be empty and contained in the [resolvedLicense]. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: I'm unsure about the "to evaluate the license for" part. No one forces the rule to actually look at / evaluate the license sources. I think it should rather say "The sources where this particular license was found".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe "The sources where this particular license was found" is mis-leading:
- The above
resolvedLicense.sourceis actually the set of sources
where the license was found. - And
licenseSourcesin fact are the
sources which shall be considered by the license rule.
Does this make more sense now?
| fun licenseRule( | ||
| name: String, | ||
| licenseView: LicenseView, | ||
| separateEvaluationPerSource: Boolean = true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we even need this configurability? Why not make the new the default, and require rule users to manually create multiple violations from this rule (one violation per source) if needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is needed for:
- Backwards compatibility
- I believe separate violations per source to be very valuable and should be still supported in
the same simple way as before.
| } | ||
| } | ||
|
|
||
| /** Backwards compatibility */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally, I would be fine with omitting this and having a breaking change, and clearly documenting the migration path as part of the release notes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean only this property, or the entire commit?
(Would you also be ok, with marking it as deprected now, and removing it later? This would allow a smooth transition outside of the critical path of upgrading ORT.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean only this property, or the entire commit?
I was referring to the entire commit.
Would you also be ok, with marking it as deprected now, and removing it later?
Yes. I was just trying to say that this additional work would not be required from my side.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I ended up not deprecating this property, because the separate per source evaluation / single source per license rule or violation is still a valid (and in my opinion even the preferred) use case. In that case using that property is a bit simpler.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree with @fviernau we don't want to get rid of per license source evaluator per rule we just want to offer ORT user more power/flexibility to define rules that match their organization's open source policies.
819ea09 to
8ee235d
Compare
be57cb6 to
0bafa29
Compare
0bafa29 to
6e5a7b5
Compare
Prepare for combining rule violations that stem from the same license and package, but from different licenses sources. Signed-off-by: Frank Viernau <frank.viernau@gmail.com>
Prepare for an upcoming change. Signed-off-by: Frank Viernau <frank.viernau@gmail.com>
While it is valuable to be able to evaluate the license rule seperately per license source, some users do desire to evaluate only once per license for all source. Introduce a toggle, which allows for that while keeping the old behavior as default. Signed-off-by: Frank Viernau <frank.viernau@gmail.com>
Ensure the previous change is backwards compatible. Signed-off-by: Frank Viernau <frank.viernau@gmail.com>
6e5a7b5 to
72fa425
Compare
See individual commits.
This is howit looks if new API is used. But migration is not necessary as its backwards compatible, it
also shows how to combine sources for one rule: oss-review-toolkit/ort-config#370.
Here you can see how the combined license soure violation is rendered in static-html reports: