Include repository license in package when available#920
Include repository license in package when available#920jsoriano merged 2 commits intoelastic:mainfrom
Conversation
mtojek
left a comment
There was a problem hiding this comment.
I know that some packages are using NOTICE.txt to carry license and Kibana shows that file. I don't know what's the difference between NOTICE.txt and LICENSE.txt, but maybe this is something we need to sort out too.
internal/builder/packages.go
Outdated
| return errors.Wrap(err, "failure while looking for license in repository") | ||
| } | ||
|
|
||
| logger.Debugf("License text found in %q will be included in package", sourceLicensePath) |
There was a problem hiding this comment.
This is the only thing logged now for packages in the integrations repo. Is this so relevant that requires higher logging level than other things? 🙂
$ elastic-package build --zip
Build the package
README.md file rendered: /home/jaime/gocode/src/github.com/elastic/integrations/packages/apache/docs/README.md
2022/08/02 16:00:17 INFO License text found in "/home/jaime/gocode/src/github.com/elastic/integrations/LICENSE.txt" will be included in package
Package built: /home/jaime/gocode/src/github.com/elastic/integrations/build/packages/apache-1.5.0.zip
Done
| dir, err := findRepositoryRootDirectory() | ||
| if errors.Is(err, os.ErrNotExist) { | ||
| return "", errors.New("package can be only built inside of a Git repository (.git folder is used as reference point)") | ||
| } |
There was a problem hiding this comment.
Does it change the implementation logic or is it just refactoring?
There was a problem hiding this comment.
Just refactoring, this is basically a copy of what there was in the loop looking for .git.
| @@ -157,6 +159,11 @@ func BuildPackage(options BuildOptions) (string, error) { | |||
| return "", errors.Wrap(err, "copying package contents failed") | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
Please leave logger.Debugf here for consistency with other steps (clear target, encode dashboards, resolve fields).
There was a problem hiding this comment.
Here it does different things depending on the situation, so it will log two messages for the same thing, do we still want the log message here?
There was a problem hiding this comment.
Ok, message added, now it looks like this:
When copied from the repo:
2022/08/02 15:59:57 DEBUG Copy license file if needed
2022/08/02 15:59:57 INFO License text found in "/home/jaime/gocode/src/github.com/elastic/integrations/LICENSE.txt" will be included in package
When already available in the package:
2022/08/02 16:01:32 DEBUG Copy license file if needed
2022/08/02 16:01:32 DEBUG License file in the package will be used
internal/builder/packages.go
Outdated
| func copyLicenseTextFile(licensePath string) error { | ||
| _, err := os.Stat(licensePath) | ||
| if err == nil { | ||
| // License is already there, nothing to do. |
There was a problem hiding this comment.
Even if it does nothing? 🙂
| return nil | ||
| } | ||
|
|
||
| func copyLicenseTextFile(licensePath string) error { |
There was a problem hiding this comment.
I guess that you don't check if the license content has changed?
There was a problem hiding this comment.
What do you mean? The file is copied as is (as any other file in the package).
There was a problem hiding this comment.
For example, the check command fails if README.md is outdated due to changed fields.
There was a problem hiding this comment.
But this content in the README is generated and has to be kept on sync with the fields definitions. Nothing is generated in the license files, they are copied as they are, as other files in packages.
💚 Build Succeeded
Expand to view the summary
Build stats
Test stats 🧪
🤖 GitHub commentsTo re-run your PR in the CI, just comment with:
|
🌐 Coverage report
|
There was a discussion about this here: elastic/package-spec#298 (comment) |
If the package doesn't include a
LICENSE.txtfile, look for one in the repository, and include it if found.Implements elastic/package-spec#298 (comment).