Skip to content
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

Add --ignore option #46

Merged
merged 7 commits into from
Apr 13, 2022
Merged

Add --ignore option #46

merged 7 commits into from
Apr 13, 2022

Conversation

at-wat
Copy link
Contributor

@at-wat at-wat commented Nov 27, 2020

Fixes #88

Packages beginning with the specified path will be ignored.
Imported packages by ignored packages are still checked.


In my case, I would like to use go-licenses to check library licenses used in our product system.
--ignore will be used to ignore internal private packages with proprietary license.

@google-cla google-cla bot added the cla: yes Contributor license agreement signed (https://cla.developers.google.com) label Nov 27, 2020
@andoks
Copy link

andoks commented Jan 19, 2021

Tested this, and it seems to work Ok. I would perhaps have preferred a way to ignore 1st-party packages instead (packages in module being scanned), but this approach is more flexible, and makes this possible by ignoring module name

@lavrd
Copy link
Contributor

lavrd commented Apr 6, 2021

What about this pull request? Looks really good.

@EizTimor
Copy link

Any updates here?
This would be very useful.

@hansinator
Copy link

plz merge. I need this, too

Packages beginning with the specified path will be ignored.
Imported packages by ignored packages are still checked.
@Bobgy
Copy link
Collaborator

Bobgy commented Apr 10, 2022

Hi people, I'm a new maintainer for go-licenses now. From the number of comments and thumb ups, I can see that this is requested by a lot.

My main concerns for merging this is that -- IMO, such kind of configurations likely grow in size, e.g. people may also need to override some packages with non-auto detectable licenses, so I want to make sure the current proposed argument syntax is compatible with that future use-case.

To my knowledge and suggested by @drigz, viper is a good option to allow configuring from both a command line argument and a config file symmetrically. Therefore, I just want to confirm the option's array syntax is compatible with cobra and viper.

@at-wat
Copy link
Contributor Author

at-wat commented Apr 11, 2022

To my knowledge and suggested by @drigz, viper is a good option to allow configuring from both a command line argument and a config file symmetrically. Therefore, I just want to confirm the option's array syntax is compatible with cobra and viper.

https://go.dev/play/p/QCK8-GwWV82
Cobra with viper can handle string array in the same way.

I didn't noticed there is StringSlice in pflag.
I'll update the PR to use pflag.StringSlice.

@at-wat
Copy link
Contributor Author

at-wat commented Apr 11, 2022

My main concerns for merging this is that -- IMO, such kind of configurations likely grow in size, e.g. people may also need to override some packages with non-auto detectable licenses, so I want to make sure the current proposed argument syntax is compatible with that future use-case.

I'm using my branch of go-licenses for more than a year in my company's product repositories to check all external dependencies have proper license.
I use --ignore option to exclude company internal packages with proprietary license.
(#46 (comment) seems having a same motivation.)
We haven't seen any other cases to add more configs.

In this kind of closed project use cases,

  • if there is a repository expressing a license but can't be detected, google/licenseclassifier should be updated (*)
  • if a repository doesn't have open license, it means it's unsafe to legally use it. Possible action of go-licenses users: (*)
    • Ignore the package if the package is individually licensed (including organization internal private packages)
    • Ask package owner to add license (*)
    • Unuse the package (*)
    • (Ignore the package and accept license risks)

In open source project use cases, above with (*) may be chosen.
So, I think there aren't many additional configs to be requested.

@drigz
Copy link
Member

drigz commented Apr 11, 2022

Thanks for simplifying with StringSlice! IMO supporting a config file is out-of-scope of this PR but if needed in future, Viper should support this (judging from status of spf13/viper#200).

@Bobgy
Copy link
Collaborator

Bobgy commented Apr 12, 2022

My main concerns for merging this is that -- IMO, such kind of configurations likely grow in size, e.g. people may also need to override some packages with non-auto detectable licenses, so I want to make sure the current proposed argument syntax is compatible with that future use-case.

I'm using my branch of go-licenses for more than a year in my company's product repositories to check all external dependencies have proper license.
I use --ignore option to exclude company internal packages with proprietary license.
(#46 (comment) seems having a same motivation.)
We haven't seen any other cases to add more configs.

In this kind of closed project use cases,

  • if there is a repository expressing a license but can't be detected, google/licenseclassifier should be updated (*)

Another possibility is that the license is too complex for go-licenses -- dual licenses, more than one licenses etc. So we need human intervention. This is where I think the config may grow.

  • if a repository doesn't have open license, it means it's unsafe to legally use it. Possible action of go-licenses users: (*)
    • Ignore the package if the package is individually licensed (including organization internal private packages)
    • Ask package owner to add license (*)
    • Unuse the package (*)
    • (Ignore the package and accept license risks)

In open source project use cases, above with (*) may be chosen.
So, I think there aren't many additional configs to be requested.

Agreed with everything else!

Copy link
Collaborator

@Bobgy Bobgy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! The simplified PR looks great!

Only some nitpickings

licenses/library.go Show resolved Hide resolved
main.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@Bobgy Bobgy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you also add a section in https://github.com/google/go-licenses/blob/master/README.md to introduce this new feature?

main.go Outdated Show resolved Hide resolved
@at-wat
Copy link
Contributor Author

at-wat commented Apr 13, 2022

Added a section to introduce the --ignore flag: https://github.com/at-wat/go-licenses/blob/add-ignore-option/README.md#ignoring-packages

@at-wat at-wat requested a review from Bobgy April 13, 2022 01:42
Copy link
Collaborator

@Bobgy Bobgy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks perfect! Thank you for this great contribution!

cc @drigz do you want to have another look?

@Bobgy
Copy link
Collaborator

Bobgy commented Apr 13, 2022

Also cc @wlynch, do you want to have a look?

@drigz
Copy link
Member

drigz commented Apr 13, 2022

LGTM

@Bobgy Bobgy merged commit 5b654af into google:master Apr 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Contributor license agreement signed (https://cla.developers.google.com)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ability to ignore unlicensed packages
7 participants