ci: add license scan workflow#1341
Conversation
| "MPL-2.0", | ||
| "WTFPL" | ||
|
|
||
| # All SPDX-compliant identifiers https://spdx.org/licenses/ |
There was a problem hiding this comment.
for reference or did you intend to remove this list on merge? it will change nevertheless, but very useful to allow-list licenses :)
what about if a package is detected without a license or has several licenses? will we be notified about that?
There was a problem hiding this comment.
I plan to remove all the unused licenses yeah, keeping them around for convenience until we have a good allowlist :)
Not sure what happens with several licenses, will look into that. I would be fairly sure that no license would not be a part of our allowed licenses, but I will also double check that!
There was a problem hiding this comment.
On the point of Others, according to GitHub docs https://github.com/actions/dependency-review-action#other-in-license-strings:
OTHER in license strings
License data comes from ClearlyDefined and you may sometimes see licenses displayed with the string OTHER in them. ClearlyDefined defines OTHER as:
This indicates that a human confirmed that there is license information in the file but that the license is not an SPDX-identified license.
OTHER is not a valid SPDX license identifier, so we convert OTHER in a license string into LicenseRef-clearlydefined-OTHER, which is valid in SPDX. If you want to add that to the deny or allow list, be sure to add LicenseRef-clearlydefined-OTHER to this list, because that is what we'll actually be comparing.
Then for some reason they also have this as a note:
If we can't detect the license for a dependency we will inform you, but the action won't fail.
There was a problem hiding this comment.
When multiple licenses are present I suspect it should handle each individually, tho I'm not sure. I see that the actions supports SPDX compliant license naming, and those support some boolean algebra like:
SPDX-License-Identifier: Apache-2.0 AND (MIT OR GPL-2.0-only)
But GitHub make no mention of whether or not the action handles such cases. Guess I'll have to look into the source code or test it manually somehow.
There was a problem hiding this comment.
Added custom step in the action actually blocking if a dependency ha no or unknown license.
There was a problem hiding this comment.
It seems like the action handles AND's & OR's :)
allow = allow?.filter(license => {
return !license.includes(' AND ') && !license.includes(' OR ')
})| "BSD-1-Clause", | ||
| "BSD-2-Clause", | ||
| "BSD-3-Clause", | ||
| "LGPL-2.0-only", |
There was a problem hiding this comment.
is lgpl 2 compatible with lgpl 3? you are a bit more familiar with licenses than me, so is that relevant to consider?
There was a problem hiding this comment.
is it relevant to block list those that we KNOW are incompatible? and then we iterate as we go?
There was a problem hiding this comment.
You are 100% correct! LGPL-2.O-only is not compatible, will remove it!
There was a problem hiding this comment.
Revised the list! I added some rationale. But I would really like some form of review of the decisions here to the degree that it's possible. I am in no way an expert on licenses or copyright law.
| "BSD-1-Clause", | ||
| "BSD-2-Clause", | ||
| "BSD-3-Clause", | ||
| "LGPL-2.0-only", |
There was a problem hiding this comment.
is it relevant to block list those that we KNOW are incompatible? and then we iterate as we go?
Type of Work
See here (internal): https://github.com/equinor/ecalc-internal/discussions/1044
Have you remembered and considered?
docs/drafts/next.draft.md)docs/docs/migration_guides/)BREAKING:in footer or!in headerWhat is this PR all about?
What else did you consider?
I think this action will also check if added deps have security-issues. The default threshold for criticality is "low". I'm not sure the what extent that will impact development, so I have left it untouched for now. With the caveat that it can be adjusted if needed.
Another point is which licenses to allow (note, deny-list is deprecated, so we can only specify the allow-list). I've added some I think should be good, but I've not delved deep into the actual contents of each license so the list may be both wrong and incomplete.
I hope this PR can be the start to fint out what to allow!
Between the lines?
close: https://github.com/equinor/ecalc-internal/issues/1442
close: https://github.com/equinor/ecalc-internal/issues/1238