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

maint: Update licenses #1244

Merged
merged 9 commits into from
Jul 24, 2024
Merged

maint: Update licenses #1244

merged 9 commits into from
Jul 24, 2024

Conversation

TylerHelmuth
Copy link
Contributor

@TylerHelmuth TylerHelmuth commented Jul 23, 2024

Which problem is this PR solving?

  • Updates licenses
  • Re-enables license verification

Closes #1242

Short description of the changes

  • ran update-licenses
  • updated verify-licenses to run the logic again

@TylerHelmuth TylerHelmuth requested a review from a team as a code owner July 23, 2024 18:02
Makefile Outdated Show resolved Hide resolved
@codeboten
Copy link
Contributor

Only in LICENSES: crypto
Common subdirectories: temp/github.com and LICENSES/github.com
Common subdirectories: temp/golang.org and LICENSES/golang.org
Common subdirectories: temp/google.golang.org and LICENSES/google.golang.org
Common subdirectories: temp/go.opentelemetry.io and LICENSES/go.opentelemetry.io
Common subdirectories: temp/gopkg.in and LICENSES/gopkg.in
Common subdirectories: temp/go.uber.org and LICENSES/go.uber.org
Only in LICENSES: LICENSE
Only in LICENSES: vendor
LICENSES directory must be updated. Run make update-licenses

Copy link
Contributor

@JamieDanielson JamieDanielson left a comment

Choose a reason for hiding this comment

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

Added a question but nothing blocking

@@ -68,16 +68,16 @@ clean:

.PHONY: install-tools
install-tools:
go install github.com/google/go-licenses@v1.0.0
go install github.com/google/go-licenses/v2@v2.0.0-alpha.1
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want to use an alpha version here? Also wondering if we should separate this into an env var further up so it's easier to spot and change (similar to the dockerize versions), vs in the middle of the command. No strong opinion here though.

Copy link
Contributor

Choose a reason for hiding this comment

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

This tool has been slow to evolve with many changes in the 1.x versions; we were stuck on 1.0 because of problems with later versions. I'd personally rather see us standardize on 2.x even if it's alpha.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think there would be a need to dynamically update the liceses version and as Kent said, they release versions very slowly. Let's go with this as is for now, and add the env var later if needed.

@TylerHelmuth TylerHelmuth added the merge at will Reviewer can merge the PR once reviewed. label Jul 23, 2024
@MikeGoldsmith MikeGoldsmith merged commit bd6b9c1 into main Jul 24, 2024
5 checks passed
@MikeGoldsmith MikeGoldsmith deleted the update-licenses branch July 24, 2024 09:44
Copy link

@friedrichg friedrichg Aug 20, 2024

Choose a reason for hiding this comment

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

This got rid of the main LICENSE file for the repo @TylerHelmuth @codeboten. Was that intended?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge at will Reviewer can merge the PR once reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Re-enable licenses check
6 participants