Provide --summary flag to generate the license summary file
#103
Conversation
commands/deps_resolve.go
Outdated
| return err | ||
| if summaryTplPath != "" { | ||
| if outDir == "" { | ||
| return fmt.Errorf("please provide the output directory to write the license summary file") |
There was a problem hiding this comment.
outDir is for -o, I think it's not related to -s, if you do this, users have to use -o if they want to use -s, i.e. they can't use -s without -o.
I thought we can simply generate the LICENSE file in the same directory as LICENSE.tpl
There was a problem hiding this comment.
Update to split these two flags.
commands/deps_resolve.go
Outdated
| } | ||
| _, err = file.WriteString(summary) | ||
| if err != nil { | ||
| logger.Log.Errorf("failed to write summary file, %v", err) |
There was a problem hiding this comment.
Return the error and handle it from the caller method
pkg/deps/golang.go
Outdated
| @@ -119,7 +125,10 @@ func (resolver *GoModResolver) ResolvePackageLicense(module *packages.Module, re | |||
| } | |||
| identifier, err := license.Identify(module.Path, string(content)) | |||
| if err != nil { | |||
| return err | |||
| if declareLicense == nil { | |||
There was a problem hiding this comment.
If the users already declare this license I don't think we need to license.Identify it, this gives the users ability to override the license if we make something wrong
pkg/deps/summary.go
Outdated
| LicenseID string // License ID | ||
| } | ||
|
|
||
| func GenerateDependencyLicenseFilename(result *Result) string { |
There was a problem hiding this comment.
This method is not used in --summary flag, it's used in --output flag, why put it here (summary.go)?
There was a problem hiding this comment.
Because I provide the Location field into the template, It could help for getting generated license file path.
There was a problem hiding this comment.
Read my comment https://github.com/apache/skywalking-eyes/pull/103/files#r866676214 , you are coupling 2 flags that they should be able to work independently
There was a problem hiding this comment.
Rollback to the previous version.
pkg/deps/summary.go
Outdated
| licenseContent, err := license.GetLicenseContent(head.License.SpdxID) | ||
| if err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
head.License.SpdxID is not required so it might be empty, users can use other forms to specify their license, not just SpdxID. You need to take care the case where SpdxID is not set
There was a problem hiding this comment.
Sure, If the head.License.SpdxID is not set, should I just throw a new error? or another config could get the license?
There was a problem hiding this comment.
Sure, If the
head.License.SpdxIDis not set, should I just throw a new error? or another config could get the license?
You can use this method to get the license content, but notice only use this as a fallback when the users don't specify spdxID
skywalking-eyes/pkg/header/config.go
Line 171 in 985866c
commands/deps_resolve.go
Outdated
| @@ -70,6 +86,12 @@ var DepsResolveCommand = &cobra.Command{ | |||
| } | |||
| } | |||
|
|
|||
| if summaryTpl != nil { | |||
| if err := writeSummary(&report); err != nil { | |||
| logger.Log.Warnf("write summary file error: %v", err) | |||
There was a problem hiding this comment.
What about
| logger.Log.Warnf("write summary file error: %v", err) | |
| return err |
There was a problem hiding this comment.
I think it's better to perform before writing licenses if we need to throw the exception, it could make the command more consistent.
README.md
Outdated
| {{.LicenseContent }} | ||
| {{ range .Groups }} | ||
| ======================================================================== | ||
| {{.Name}} licenses |
There was a problem hiding this comment.
Do you think it'd be more specific if we rename {{.Name}} to something like {{.LicenseSpdxID}} or {{.LicenseName}}? Because there is another {{.Name}} inside the {{.Deps}},
There was a problem hiding this comment.
I think it could be better to rename it to {{.LicenseID}}? WDYT?
Let it same with the {{.LicenseID}} inside the {{.Deps}}. For now, they are the same.
There was a problem hiding this comment.
I think it could be better to rename it to
{{.LicenseID}}? WDYT?
{{.LicenseID}} is good to me
pkg/deps/summary.go
Outdated
| // the license id of the project | ||
| var headerContent string | ||
| if head.License.SpdxID != "" { | ||
| headerContent, _ = license.GetLicenseContent(head.License.SpdxID) |
There was a problem hiding this comment.
I don't think the error can be ignored, if the users specify an unknown SpdxID (by mistake), we should notify that
There was a problem hiding this comment.
Updated. If GetLicenseContent could not be found, then throw an error.
kezhenxu94
changed the title
--summary flag to generate the license summary file
Follow apache/skywalking#8992, I have done two things:
--summaryto help users generate the license summary file by template file.licensesconfig under thedependencyto declare which license is could not be recognized.