osv-scanner icon indicating copy to clipboard operation
osv-scanner copied to clipboard

✨ Add --exclude-packges Flag to osv-scanner CLI

Open toddtee opened this issue 2 years ago • 5 comments

This PR adds an --exclude-packages flag to the osv-scanner CLI. This flag allows users to specify a list of regex patterns to which matches of collected package names will be matched against.

If the packages are found to match the regex patterns, they will be excluded from the query to the OSV database.

Exclusions work for both lockfile and SBOM package definitions.

Excluded-packages flag will address issue #151.

toddtee avatar Nov 16 '23 04:11 toddtee

Docs look good with a couple of comments.

hayleycd avatar Nov 22 '23 00:11 hayleycd

Hi @oliverchang, thanks for checking out my PR!

Could we understand if a config file would also work for your use case? Or are there reasons why you'd prefer a CLI flag?

Ideally, I think having the exclusions parameter both as a CLI flag and a configuration parameter would be ideal.

I like the idea of having the CLI flag as I personally believe that CLI tools should have good levels of interactivity for a human user. For example, if I as a developer want to quickly update a project and I have a particular package that is giving me issues that I am blocked on updating or don't want to query for whatever reason, it is nice to have the flag as a quick option to interact with the tool as I need it. Or I could even use the flag to test the scan results for when I am developing the regex patterns that will eventually end up in the configuration file! 🤣

However... let's now talk automation, not human interaction. If I incorporate the osv-scanner as part of my project's CI/CD pipeline or automated security checks, having the flag available is good, however having a configuration parameter is better. That way, maintainers of the project can configure what exclusions they like and perhaps even have comments in the config file as to why they are there. I see the configuration parameter for more permanent and/or governing exclusion purposes.

TL;DR, I see the CLI flag for quick developer use for once-offs or short term purposes and the configuration file more for automated processes and more permanent solutions.

I don't know how challenging incorporating the configuration file into my current offering would be in the short term...

However, as I am a supporter of one PR including one change only. I would propose that this PR maintain adding the functionality of a CLI flag exclusively, and perhaps create a related Issue and follow-up PR to add exclusions as a configuration option as well.

The benefit of this is the tool can still offer the ability to exclude packages from the query in the short term and deliver value to those that need it. A configuration parameter will extend that functionality offering further.

I hope that answers your question 🙏

toddtee avatar Nov 22 '23 00:11 toddtee

Hi @oliverchang, thanks for checking out my PR!

Could we understand if a config file would also work for your use case? Or are there reasons why you'd prefer a CLI flag?

Ideally, I think having the exclusions parameter both as a CLI flag and a configuration parameter would be ideal.

I like the idea of having the CLI flag as I personally believe that CLI tools should have good levels of interactivity for a human user. For example, if I as a developer want to quickly update a project and I have a particular package that is giving me issues that I am blocked on updating or don't want to query for whatever reason, it is nice to have the flag as a quick option to interact with the tool as I need it. Or I could even use the flag to test the scan results for when I am developing the regex patterns that will eventually end up in the configuration file! 🤣

Thanks, that makes sense. We've generalised shied away from having multiple ways to achieve the same thing out of a desire to keep the CLI simple to understand, but having a way to quickly iterate/test does make sense.

Could we perhaps rename the flag to --exclude-packages though? --exclude is a little ambiguous on its own.

@another-rex @G-Rath Do you think there's value in maintaining this as a CLI flag?

However... let's now talk automation, not human interaction. If I incorporate the osv-scanner as part of my project's CI/CD pipeline or automated security checks, having the flag available is good, however having a configuration parameter is better. That way, maintainers of the project can configure what exclusions they like and perhaps even have comments in the config file as to why they are there. I see the configuration parameter for more permanent and/or governing exclusion purposes.

Big +1 to this. Having this stored as the repo would help maintain state around exclusions and reduce duplicate triage efforts for the project.

TL;DR, I see the CLI flag for quick developer use for once-offs or short term purposes and the configuration file more for automated processes and more permanent solutions.

I don't know how challenging incorporating the configuration file into my current offering would be in the short term...

However, as I am a supporter of one PR including one change only. I would propose that this PR maintain adding the functionality of a CLI flag exclusively, and perhaps create a related Issue and follow-up PR to add exclusions as a configuration option as well.

Yep! I've filed https://github.com/google/osv-scanner/issues/670 for this.

The benefit of this is the tool can still offer the ability to exclude packages from the query in the short term and deliver value to those that need it. A configuration parameter will extend that functionality offering further.

I hope that answers your question 🙏

oliverchang avatar Nov 23 '23 04:11 oliverchang

@oliverchang, I think --exclude-packages is better as it is more explicit to what the functionality actually is. I have updated the flag and documentations to reflect this. 👍

toddtee avatar Nov 23 '23 04:11 toddtee

Do you think there's value in maintaining this as a CLI flag?

I'm not sure - I'm not getting super strong "omg yes this'd be great" vibes from my gut, but that doesn't meant it still wouldn't be useful.

However...

For example, if I as a developer want to quickly update a project and I have a particular package that is giving me issues that I am blocked on updating or don't want to query for whatever reason, it is nice to have the flag as a quick option to interact with the tool as I need it

I'm not sure that is as useful as you think: since we're agreed that it's best to capture this in config long-term, in this situation you'd be running the scanner locally in most cases, meaning you could just mentally ignore the results too - I know that is not as nice as having them omitted but not sure if its "support a new flag forever" nice.

I could even use the flag to test the scan results for when I am developing the regex patterns that will eventually end up in the configuration file!

That does seem like a useful reason to have the flag, though I don't think it would be that much overhead to just be using the config file; that would also mean less risk that a specific shell might get confused by the regexp.


I don't think this should be taken as "never will we ever have this flag", but it feels like it would probably be best to start life via the config file

G-Rath avatar Nov 27 '23 18:11 G-Rath

This pull request has not had any activity for 60 days and will be automatically closed in two weeks

github-actions[bot] avatar Jul 20 '24 18:07 github-actions[bot]

Automatically closing stale pull request

github-actions[bot] avatar Aug 03 '24 19:08 github-actions[bot]