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

feat(license): add new flag for classifier confidence level #4073

Merged
merged 14 commits into from
Apr 24, 2023

Conversation

thevibegod
Copy link
Contributor

@thevibegod thevibegod commented Apr 15, 2023

Description

  • Add new flag --license-confidence-level that accepts a float value(default 0.9)
  • Make use of the above flag's value in Classify method of classifier.go.

Related issues

Checklist

  • I've read the guidelines for contributing to this repository.
  • I've followed the conventions in the PR title.
  • I've added tests that prove my fix is effective or that my feature works.
  • I've updated the documentation with the relevant information (if needed).
  • I've added usage information (if the PR introduces new options)

@CLAassistant
Copy link

CLAassistant commented Apr 15, 2023

CLA assistant check
All committers have signed the CLA.

Copy link
Contributor

@DmitriyLewen DmitriyLewen left a comment

Choose a reason for hiding this comment

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

Hello @thevibegod
Thanks for your work!

Resolve conflicts(pull of main branch should help), please.

I added some comments. Take a look, please.

Also i think we need to add more information about confidence level to docs. (default value, how value affects to number of licenses, etc...)

Comment on lines 58 to 59
--license-confidence-level float specify classifier confidence level (default 0.9)
--license-full eagerly look for licenses in source code headers and license files
Copy link
Contributor

Choose a reason for hiding this comment

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

if i remember correctly - k8s mode doesn't support find licenses.
Looks like we can remove this -

LicenseFlagGroup: flag.NewLicenseFlagGroup(),

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Made the above mentioned changes.

Comment on lines 49 to 50
--license-confidence-level float specify classifier confidence level (default 0.9)
--license-full eagerly look for licenses in source code headers and license files
Copy link
Contributor

Choose a reason for hiding this comment

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

same

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Made the above mentioned changes.

@@ -50,13 +50,16 @@ type gomodAnalyzer struct {

// go.mod/go.sum in dependencies
leafModParser godeptypes.Parser

opt analyzer.AnalyzerOptions
Copy link
Contributor

Choose a reason for hiding this comment

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

We use only 1 value. We don't need to move all options

Suggested change
opt analyzer.AnalyzerOptions
classifierConfidenceLevel float64

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Made the above mentioned changes.

FileChecksum bool
Offline bool
FileChecksum bool
ClassifierConfidenceLevel float64
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure about this changes.

For licensing and dpkg packages we can use Init functions as for dpkg copyright files.
In this case we move this value to analyzers

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Made the above mentioned changes.

@thevibegod
Copy link
Contributor Author

@DmitriyLewen Thanks for reviewing and providing the suggestions. Have made all the above mentioned changes and also updated the docs with examples. Please review them and let me know if there are any more changes to be done.

Copy link
Contributor

@DmitriyLewen DmitriyLewen left a comment

Choose a reason for hiding this comment

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

Thanks for your work!
LGTM.

@DmitriyLewen
Copy link
Contributor

@thevibegod can you run mage docs:generate to update CLI docs and fix test?

@thevibegod
Copy link
Contributor Author

@thevibegod can you run mage docs:generate to update CLI docs and fix test?

@DmitriyLewen I have updated the docs.

@DmitriyLewen
Copy link
Contributor

cool! thanks!

@knqyf263 knqyf263 merged commit 0650e0e into aquasecurity:main Apr 24, 2023
8 checks passed
@knqyf263
Copy link
Collaborator

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Config flag for confidence level in the license scanner
5 participants