Configurable check by becoded · Pull Request #150 · google/go-licenses

becoded · 2022-09-21T14:15:21Z

Allow to configure the check action.
Added flags:

  --disallowed_types  list of disallowed types
  --allowed_licenses list of allowed license names

Keeping the default behaviour of just checking the forbidden licenses.

Bobgy · 2022-09-22T12:52:11Z

check.go

+	return false
+}
+
+func getSupportedLicenseNames() []string {


Curious, why do we need this list?

I didn't find a method in github.com/google/licenseclassifier to get all defined license names.
It is used here to validate the input of flag allowed_license_names.

Is there a strong need to do this validation? If there is a typo, the check command fails and reports the correct license name in error message.

An extraneous invalid license name seems not much harm either.

You have a valid point! I removed the validation.

Bobgy · 2022-09-23T12:27:45Z

Thank you for the contribution!

Let me start with high level questions.

Can you clarify the use case for allowed licenses? That seems too flexible as interface, it's probably only useful when very limited license types are allowed.

Would it be simpler asking someone with such flexible use case to directly parse the license CSV file and write custom validation?

becoded · 2022-09-23T16:18:25Z

Currently, only the list of forbidden licenses, defined in google/licenseclassifier, is validated.
And there is no way to change that, so it doesn't leave any room for flexibility in controlling that.
So for that reason, I added the other flags (--exclude_forbidden, --exclude_notice, --exclude_permissive, --exclude_reciprocal, --exclude_restricted, --exclude_unencumbered).
However, these sets are fixed.
I have a case where a subset is allowed and for others, we need special approval.
By allowing to create your own list of allowed licenses --allowed_license_names we are able to verify exactly what we need.

Personally, I don't think it would be simpler to parse the report.
A part of this tool is written to check licenses, it makes sense to me that you can also control what is checked. Otherwise you could argue also that there is no need for the check action as you could also write a parser to check for the forbidden licenses.

Bobgy · 2022-09-26T09:58:59Z

To explain why I'm asking, this tool was built primarily for Google's own use-cases for open source go projects, so I am seeking a balance between maintenance cost vs use-case coverage. I'm asking to learn more about why this feature is useful to you.

I think your use-case convinces me it may be useful to more people. Thank you for explaining that!

Bobgy · 2022-09-26T10:06:25Z

Regarding the interface, do the following changes make it slightly simpler and more consistent?

Make exclude flags one flag that takes multiple values: --disallowed_categories, the description points to https://github.com/google/licenseclassifier/blob/e6a9bb99b5a6f71d5a34336b8245e305f5430f99/license_type.go#L180 for license categories.
Reduce number of words on allowed_license_names: --allowed_licenses.

Also, I think we should add validation and description that, the two flags cannot be used at the same time.

becoded · 2022-09-26T16:03:48Z

I changed the interface. I did name it types instead of categories as that is what is used in multiple places in this repo and in google/licenseclassifier

Bobgy · 2022-09-27T09:22:12Z

nit: update PR description?

Bobgy · 2022-09-23T12:15:19Z

README.md

+* See supported license names: [github.com/google/licenseclassifier](https://github.com/google/licenseclassifier/blob/e6a9bb99b5a6f71d5a34336b8245e305f5430f99/license_type.go#L28)  
+

 Typically, specify the Go package that builds your Go binary.


Can you move this paragraph above? It's more important than exlude options I feel.

Added it to the top of the usage block as it is helpful for all comments.

Bobgy · 2022-09-27T09:29:51Z

check.go


 func init() {
+	checkCmd.Flags().StringSliceVar(&allowedLicenses, "allowed_licenses", []string{}, "list of allowed license names, can't be used in combination with disallowed_types")
+	checkCmd.Flags().StringSliceVar(&disallowedTypes, "disallowed_types", []string{}, "list of disallowed license types, can't be used in combination with allowed_licenses")


Add comment about default behavior?

Bobgy · 2022-09-27T09:33:23Z

check.go

+		case "unencumbered":
+			excludedLicenseTypes = append(excludedLicenseTypes, licenses.Unencumbered)
+		default:
+			fmt.Fprintf(os.Stderr, "Unknown license type '%s' provided", v)


Also print the list of supported types?

Bobgy · 2022-09-27T11:42:55Z

check.go

+			found = true
+		}
+
+		if hasLicenseType && isDisallowedLicenseType(licenseType, disallowedLicenseTypes) {


See previous unmerged PR for a reference of how to handle unknown types: #44

Added support for unknown and added it to the default set.

Didn't implement different exit codes.
In the past, it stopped as soon as one forbidden license was found. In the meantime, it reports all forbidden licenses.

Bobgy · 2022-09-27T11:43:38Z

README.md


 ```shell
-go-licenses check <package> [package...]
+go-licenses check <package> [package...] 


Also comment default behavior is disallow forbidden and unknown licenses?

Added it to the title

hansinator · 2022-09-27T12:19:53Z

hey thank you for this PR and hopefully this gets merged soon, b/c we need to maintain a fork of license checker and it causes trouble with replace directives and deprecated go get / go install behaviour. basically soft-forks of "go install"-ed things are no longer a supported use-case since a few go versions so I'd really appreciate it if this goes upstream

Bobgy

Awesome 👍!
Thank you for the great contribution!

becoded added 3 commits September 21, 2022 14:57

allow to configure the check command

69c546a

update linter version + fix new lint issues

aacc0be

update readme + change flags for consistency

cc8e4e9

Bobgy reviewed Sep 22, 2022

View reviewed changes

Don't validate the license names upfront

6d1fafb

becoded added 2 commits September 26, 2022 17:57

simplify the interface

787e04e

add extra usage documentation

aa5ceef

fix merge conflicts with master

cfa8dfb

Bobgy requested changes Sep 27, 2022

View reviewed changes

Bobgy mentioned this pull request Sep 27, 2022

add cmdline flag to exclude restricted licenses in check subcommand #81

Closed

Bobgy reviewed Sep 27, 2022

View reviewed changes

becoded added 2 commits September 27, 2022 15:39

add support for unknown licenses

1b351d3

move global usage comment

0451e07

becoded requested a review from Bobgy September 28, 2022 11:03

Bobgy approved these changes Sep 28, 2022

View reviewed changes

Bobgy merged commit 5b8f2a2 into google:master Sep 28, 2022

Bobgy mentioned this pull request Oct 6, 2022

Check: return non-zero status code for unknown licenses. #44

Closed

		* See supported license names: [github.com/google/licenseclassifier](https://github.com/google/licenseclassifier/blob/e6a9bb99b5a6f71d5a34336b8245e305f5430f99/license_type.go#L28)


		Typically, specify the Go package that builds your Go binary.

Conversation

becoded commented Sep 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Bobgy commented Sep 23, 2022

Uh oh!

becoded commented Sep 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Bobgy commented Sep 26, 2022

Uh oh!

Bobgy commented Sep 26, 2022

Uh oh!

becoded commented Sep 26, 2022

Uh oh!

Bobgy commented Sep 27, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hansinator commented Sep 27, 2022

Uh oh!

Bobgy left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

becoded commented Sep 21, 2022 •

edited

Loading

becoded commented Sep 23, 2022 •

edited

Loading