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

[chore] add license-check #7589

Merged
merged 3 commits into from
May 3, 2023

Conversation

codeboten
Copy link
Contributor

First pass at cleaning up the license in files.

@codeboten codeboten requested review from a team and dmitryax May 1, 2023 22:58
@codecov
Copy link

codecov bot commented May 1, 2023

Codecov Report

Patch and project coverage have no change.

Comparison is base (5255945) 91.23% compared to head (d8da8a6) 91.23%.

❗ Current head d8da8a6 differs from pull request most recent head 1ae03e6. Consider uploading reports for the commit 1ae03e6 to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #7589   +/-   ##
=======================================
  Coverage   91.23%   91.23%           
=======================================
  Files         296      296           
  Lines       14472    14472           
=======================================
  Hits        13203    13203           
  Misses       1004     1004           
  Partials      265      265           
Impacted Files Coverage Δ
config/confighttp/compression.go 84.11% <ø> (ø)
confmap/internal/mapstructure/encoder.go 100.00% <ø> (ø)
extension/auth/client.go 100.00% <ø> (ø)
extension/auth/server.go 100.00% <ø> (ø)
internal/cgroups/cgroup.go 88.88% <ø> (ø)
internal/cgroups/cgroups.go 92.00% <ø> (ø)
internal/cgroups/errors.go 0.00% <ø> (ø)
internal/cgroups/mountpoint.go 100.00% <ø> (ø)
internal/cgroups/subsys.go 92.30% <ø> (ø)
internal/iruntime/mem_info.go 100.00% <ø> (ø)
... and 2 more

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@dmitryax
Copy link
Member

dmitryax commented May 1, 2023

We already have addlicense and checklicense targets in Makefile.common. Do we really need another one?

I think the problem with the existing checklicense is that it checks only for presence of the license but doesn't compare the header line with what we want. I think we need to remove -check from https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/main/Makefile.Common#L131 and use the diff approach that we have for other generated stuff. WDYT?

@codeboten
Copy link
Contributor Author

We already have addlicense and checklicense targets in Makefile.common. Do we really need another one?

We likely don't need another one. I just used the check license from the opentelemetry-go-build-tools repo. I don't care which way we end up going, but i do like the simplicity of the check w/ awk

@codeboten
Copy link
Contributor Author

codeboten commented May 2, 2023

After testing this out, i don't think addlicense will tell us if a license is different than what's expected. I ran it on the repo as is, and although it added licenses to any missing files, it left these in place (note the 2 spaces after Copyright):

// Copyright  The OpenTelemetry Authors

It's possible I misused addlicense though

@codeboten
Copy link
Contributor Author

After discussing this w/ @dmitryax, we agreed that it makes sense to check the license using the same check as the opentelemetry-go-build-tools repo, as addlicense doesn't check the formatting of the license.

I've updated checklicense to use awk instead. PTAL

Copy link
Member

@bogdandrutu bogdandrutu left a comment

Choose a reason for hiding this comment

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

LGTM on the fixes, I will let you folks @dmitryax and @codeboten to decide which tool to use.

@@ -1,4 +1,18 @@
#!/bin/bash -ex
#
# Copyright The OpenTelemetry Authors
Copy link
Member

Choose a reason for hiding this comment

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

Should this be before the first line?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I push the copyright notice after as shellcheck complains

Screen Shot 2023-05-03 at 8 08 15 AM

Alex Boten added 3 commits May 3, 2023 08:32
First pass at cleaning up the license in files.

Signed-off-by: Alex Boten <aboten@lightstep.com>
Signed-off-by: Alex Boten <aboten@lightstep.com>
Signed-off-by: Alex Boten <aboten@lightstep.com>
@codeboten codeboten merged commit 8b27d6b into open-telemetry:main May 3, 2023
@codeboten codeboten deleted the codeboten/license-check branch May 3, 2023 15:53
@github-actions github-actions bot added this to the next release milestone May 3, 2023
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.

3 participants