-
Notifications
You must be signed in to change notification settings - Fork 571
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
Allow for custom non SPDX-expression licenses (including content) #3366
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: HeyeOpenSource <[email protected]>
Signed-off-by: HeyeOpenSource <[email protected]>
…available. Signed-off-by: HeyeOpenSource <[email protected]>
Signed-off-by: HeyeOpenSource <[email protected]>
…tion in order to avoid incompatibilities (and multiple instances of the same license). Signed-off-by: HeyeOpenSource <[email protected]>
Signed-off-by: HeyeOpenSource <[email protected]>
Signed-off-by: HeyeOpenSource <[email protected]> # Conflicts: # internal/licenses/parser.go
Signed-off-by: HeyeOpenSource <[email protected]>
Signed-off-by: HeyeOpenSource <[email protected]>
Signed-off-by: HeyeOpenSource <[email protected]>
…tic analysis. Signed-off-by: HeyeOpenSource <[email protected]>
Signed-off-by: HeyeOpenSource <[email protected]>
Signed-off-by: HeyeOpenSource <[email protected]>
This reverts commit beda1b6. Signed-off-by: HeyeOpenSource <[email protected]>
Signed-off-by: HeyeOpenSource <[email protected]>
This reverts commit 9b90378. Signed-off-by: HeyeOpenSource <[email protected]>
Signed-off-by: HeyeOpenSource <[email protected]>
7a98f3a
to
3e546de
Compare
@@ -29,6 +29,7 @@ type License struct { | |||
Type license.Type | |||
URLs []string `hash:"ignore"` | |||
Locations file.LocationSet `hash:"ignore"` | |||
Contents string `hash:"ignore"` // The optional binary contents of the license file |
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.
If we add support for this we should make this configurable. It could be a simple on/off (collect or do not collect contents) but there is some argument for more options here. Specifically, when we can get the ID from contents and we have a high degree of confidence that it matches then including the contents is redundant / not necessary (one of multiple possible middle of the road options).
I think the default for this option should either be "off" or the middle of the road option.
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.
My implementation only ever fills the Contents
field if a non-empty file identified as potential license file (by name) exists and no SPDX-expression can be determined for it with the required confidence.
Otherwise the status quo implementation is retained.
Hence I would assume that the option as you describe it is currently "middle of the road" by default. 😉
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 would advocate for a later implementation of the configurability, as my current implementation mitigates the fact, that packages with an identified (but non-SPDX-expression-compatible) license file will be erroneously handled as unlicensed. (cf. #3412)
The Validations workflow finally throws no more errors at me. 👍 |
The implementation of this pull request mitigates the fact, that packages with an identified (but non-SPDX-compatible) license file will be erroneously handled as unlicensed. (cf. #3412) |
Description
Implements support for custom licenses.
Should probably be reviewed by @wagoodman and / or @spiffcs.
Type of change
Checklist:
make test
locally without receiving any failures