Include licenses in packages by jsoriano · Pull Request #882 · elastic/elastic-package

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

 	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

 	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

Conversation

jsoriano commented Jun 30, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mtojek left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mtojek Jun 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

elasticmachine commented Jun 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

💚 Build Succeeded

Build stats

Test stats 🧪

🤖 GitHub comments

Uh oh!

elasticmachine commented Jun 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🌐 Coverage report

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mtojek left a comment

Choose a reason for hiding this comment

Uh oh!

jsoriano commented Jul 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mtojek commented Jul 18, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

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