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

Refer to CNCF allowlist for 3rd-party licenses approved for inclusion in CNCF repos #2506

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

trentm
Copy link

@trentm trentm commented Jan 10, 2025

This came up in open-telemetry/opentelemetry-js#5305 which is looking to include some ISC-licensed code in opentelemetry-js.git.

Refs: #2504


Some notes:

Comment on lines 210 to 211
Apache 2.0 License. The CNCF maintains a list of licenses approved for inclusion
in a CNCF codebase [here](https://github.com/cncf/foundation/blob/main/allowed-third-party-license-policy.md#approved-licenses-for-allowlist).
Copy link
Member

Choose a reason for hiding this comment

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

I think the wording should indicate that there are further conditions that need to be met in order for the third party code to be usable (namely the 'third party folder' and popularity requirements)

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe instead of specifying those here we can link to CNCF recommendations for code attribution and state that they should also be observed?

Copy link
Author

Choose a reason for hiding this comment

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

that there are further conditions that need to be met

I'm a little wary of extending the scope here -- the current state allows MIT/BSD but doesn't add this requirement -- but fair. The requirement from that CNCF doc that I have specific questions about:

It is either (A) stored unmodified in a designated third-party folder

Looking at open-telemetry/opentelemetry-js#5305 that originally motivated this. That PR includes two test files verbatim from the node-semver 3rd-party dependency:

and one test file adapted from node-semver:

Q: Do you think it would be sufficient to pull those two files into a separate subfolder, say "test/common/third-party/fixtures" rather than the current "test/common/fixtures/" (which has other fixture files)? Then we could include the node-semver license file in that subfolder.

(I can also move this discussion over to the original PR or to #2504 if that is more helpful.)

Copy link
Author

Choose a reason for hiding this comment

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

commit 41b543a. PTAL.

Copy link
Member

Choose a reason for hiding this comment

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

I wasn't previously aware of the "third party folder" requirement 🤔

Copy link
Author

Choose a reason for hiding this comment

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

I think that guidance was written for considering inclusion of whole 3rd-party libraries, but that's totally a guess.

Some random other examples in OTel repos:

// Copyright The OpenTelemetry Authors
// SPDX-License-Identifier: Apache-2.0

// Keep the original Uber license.

// Copyright (c) 2017 Uber Technologies, Inc.
//
// Permission is hereby granted, ...

Copy link
Member

Choose a reason for hiding this comment

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

we have many occurrences in the Java Instrumentation repo of 3rd party code that we've imported: https://github.com/search?q=repo%3Aopen-telemetry%2Fopentelemetry-java-instrumentation+%22Includes+work+from%22+language%3AJava&type=code

luckily Apache is by far the most popular license in the Java world 😅

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.

5 participants