Include licenses in packages #882

jsoriano · 2022-06-30T11:13:35Z

Include a new LICENSE.txt file when building packages that include the source.license field in their manifests.

Follow up of elastic/package-spec#355.
Part of elastic/package-spec#298.

jsoriano · 2022-06-30T11:14:25Z

go.mod

@@ -152,3 +152,5 @@ require (
 	sigs.k8s.io/structured-merge-diff/v4 v4.2.1 // indirect
 	sigs.k8s.io/yaml v1.3.0 // indirect
 )
+
+replace github.com/elastic/package-spec => github.com/elastic/package-spec v1.12.2-0.20220629092545-9ee240e557a5


To be removed before merging, once elastic/package-spec#355 is included in the used package-spec.

mtojek

I think that you may want to take a look also at the create command and add an option to select licenses there.

mtojek · 2022-06-30T11:21:56Z

internal/packages/packages.go

@@ -110,7 +110,10 @@ type PackageManifest struct {
 	Owner           Owner            `config:"owner" json:"owner" yaml:"owner"`
 	Description     string           `config:"description" json:"description" yaml:"description"`
 	License         string           `config:"license" json:"license" yaml:"license"`
-	Categories      []string         `config:"categories" json:"categories" yaml:"categories"`
+	Source          struct {
+		License string `config:"license" json:"license" yaml:"license"`


Do you plan to deprecate the license field in this PR?

It is deprecated in the package-spec.
I think we are not doing anything with deprecations in elastic-package, do you have a proposal for something that we could do here?

I would drop a comment in the source in this place as both fields are modeled. Frankly speaking, it's another argument for switching to a common place with business/model structures.

Frankly speaking, it's another argument for switching to a common place with business/model structures.

Yep, another +1 for something like elastic/package-spec#322.

internal/builder/packages.go

mtojek · 2022-06-30T11:24:45Z

internal/builder/packages.go

+	}
+	if license := m.Source.License; license != "" {
+		logger.Debugf("Write license text for %q", license)
+		err := licenses.WriteTextToFile(license, filepath.Join(destinationDir, "LICENSE.txt"))


Is there an option to use your own license? Will it break the building logic here?

Not at the moment, do we want to support it?

If we want, we could relax the check of unknown licenses, and include whatever the developer places in the source.license field and the LICENSE.txt file.

(it will break the current logic because it assumes that source.license if set, is set to one of the values enumerated in the spec)

Passing this to @akshay-saraswat. From developer's point of view, we may end up with an edge case, when a developer/team/customer needs to slightly modify the LICENSE text. For example, add an extra paragraph.

mtojek · 2022-06-30T11:25:55Z

internal/builder/packages.go

+	if options.buildDir == "" {
+		destinationDir, err = BuildPackagesDirectory(options.PackageRoot)
+		if err != nil {
+			return "", errors.Wrap(err, "can't locate build directory")
+		}
+	} else {
+		destinationDir, err = buildPackagesDirectory(options.buildDir, options.PackageRoot)
+		if err != nil {
+			return "", errors.Wrap(err, "can't locate build directory")
+		}


Why exactly do you need this if-else block? It looks like it complicated the current logic.

I added options.buildDir to have a predictable place where packages are built for testing.

I can move this logic to the new internal method. I didn't modify the behaviour or interface of BuildPackagesDirectory because it is a exposed method, though it is in an internal package.

Moved logic to the current BuildPackagesDirectory and changed the other use we have of this.

internal/builder/packages.go

elasticmachine · 2022-06-30T11:42:51Z

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS

Expand to view the summary

Build stats

Start Time: 2022-06-30T15:28:28.984+0000
Duration: 30 min 26 sec

Test stats 🧪

Test	Results
Failed	0
Passed	776
Skipped	0
Total	776

🤖 GitHub comments

To re-run your PR in the CI, just comment with:

/test : Re-trigger the build.

elasticmachine · 2022-06-30T11:43:00Z

🌐 Coverage report

Name	Metrics % (`covered/total`)	Diff
Packages	100.0% (`33/33`)	💚
Files	67.5% (`81/120`)	👍 1.121
Classes	62.874% (`105/167`)	👍 0.911
Methods	49.926% (`337/675`)	👍 0.155
Lines	33.064% (`3025/9149`)	👎 -0.1
Conditionals	100.0% (`0/0`)	💚

internal/builder/packages.go

mtojek · 2022-06-30T13:27:36Z

internal/licenses/_static/Apache-2.0.txt

+      same "printed page" as the copyright notice for easier
+      identification within third-party archives.
+
+   Copyright [yyyy] [name of copyright owner]


Should we render this data or remove the entry?

This comment elastic/package-spec#367 (comment) applies here.

mtojek

Recent changes look good to me, but will hold my "approval" until we clarify this point.

jsoriano · 2022-07-18T10:10:41Z

@mtojek I am thinking on closing this, and leaving developers the responsibility of properly licensing their code. Reasons:

As you mentioned, developers may require slightly variations of the license text.
Some licenses require to add additional headers or things like that to be properly added, so we would be providing a partial solution unless we do everything tools like go-licenser tools do.
This is a common practice. Developers use to use different tools to manage the licensing of their code.
We can still add more validations or tooling in the future.

The plan would still be:

source.license is an optional field that can be used to indicate the license of the code, it is responsibility of the developer to apply it correctly. It is not strongly validated, as similar fields in other packages as docker images or PIP are not validated.
Still merge Add definition for the license file package-spec#367 to formalize where the license code should reside, we don't do any validation on this.

mtojek · 2022-07-18T10:46:50Z

SGTM, maybe you can focus on the spec profiles and forbid using deprecated properties, or enable format_version related logic.

jsoriano added 4 commits June 30, 2022 12:02

Add licenses package

91b16d3

Add license text

a986578

Use latest package-spec

df372b5

Add tests

7018b2a

jsoriano requested a review from a team June 30, 2022 11:13

jsoriano self-assigned this Jun 30, 2022

jsoriano commented Jun 30, 2022

View reviewed changes

jsoriano mentioned this pull request Jun 30, 2022

Add License and Subscription in integration details section elastic/package-spec#298

Closed

4 tasks

mtojek reviewed Jun 30, 2022

View reviewed changes

Refactor logic to look for package build directory

510abc9

jsoriano mentioned this pull request Jun 30, 2022

Add definition for the license file elastic/package-spec#367

Merged

2 tasks

mtojek reviewed Jun 30, 2022

View reviewed changes

Use parameters instead of hidden attribute

82da4f7

mtojek reviewed Jul 4, 2022

View reviewed changes

jsoriano closed this Jul 18, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Include licenses in packages #882

Include licenses in packages #882

jsoriano commented Jun 30, 2022

jsoriano Jun 30, 2022

mtojek left a comment

mtojek Jun 30, 2022

jsoriano Jun 30, 2022

mtojek Jun 30, 2022

jsoriano Jun 30, 2022

mtojek Jun 30, 2022

jsoriano Jun 30, 2022

jsoriano Jun 30, 2022

mtojek Jun 30, 2022 •

edited

Loading

mtojek Jun 30, 2022

jsoriano Jun 30, 2022

jsoriano Jun 30, 2022

elasticmachine commented Jun 30, 2022 •

edited

Loading

Build stats

Test stats 🧪

elasticmachine commented Jun 30, 2022 •

edited

Loading

mtojek Jun 30, 2022

jsoriano Jun 30, 2022

mtojek left a comment

jsoriano commented Jul 18, 2022 •

edited

Loading

mtojek commented Jul 18, 2022

Include licenses in packages #882

Include licenses in packages #882

Conversation

jsoriano commented Jun 30, 2022

Choose a reason for hiding this comment

mtojek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mtojek Jun 30, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented Jun 30, 2022 • edited Loading

💚 Build Succeeded

Build stats

Test stats 🧪

🤖 GitHub comments

elasticmachine commented Jun 30, 2022 • edited Loading

🌐 Coverage report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mtojek left a comment

Choose a reason for hiding this comment

jsoriano commented Jul 18, 2022 • edited Loading

mtojek commented Jul 18, 2022

mtojek Jun 30, 2022 •

edited

Loading

elasticmachine commented Jun 30, 2022 •

edited

Loading

elasticmachine commented Jun 30, 2022 •

edited

Loading

jsoriano commented Jul 18, 2022 •

edited

Loading