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.

Sign up for GitHub

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

Jump to bottom

KAFKA-12593: Fix Apache License headers #10452

Merged
merged 2 commits into from
Apr 1, 2021
Merged

KAFKA-12593: Fix Apache License headers #10452

merged 2 commits into from
Apr 1, 2021

Conversation

vvcephei
Copy link
Contributor

@vvcephei vvcephei commented Mar 31, 2021 •
edited
Loading

The current headers in the streams-scala module, the gradle wrapper,
and some of the ducktape tests differ from the required headers for this project.

  • add a header style check for scala files
  • apply the correct header
  • move the original copyright notices to the NOTICE file.

Committer Checklist (excluded from commit message)

  • Verify design and implementation
  • Verify test coverage and CI build status
  • Verify documentation (including upgrade notes)

@vvcephei
Copy link
Contributor Author

Hey @debasishg and @seglo ,

I hope you've been well.

Since you were the original contributors of the streams-scala module #4756, I wanted to ping you to see if you're available to review some fixes for a problem we've recently become aware of in the license header.

It turns out that the license header should not have contained extra copyright notices, and I would like to remove them, but I'm unsure if I'm allowed to. I added a section to the NOTICE file to capture the historical copyright information, but this seems kind of clunky.

As the original contributors, what do you think about just dropping the notices?

Please see https://lists.apache.org/thread.html/r2df54c11c10d3d38443054998bc7dd92d34362641733c2fb7c579b50%40%3Cdev.kafka.apache.org%3E for more information.

Thanks,
-John

vvcephei
vvcephei commented Mar 31, 2021
View reviewed changes
NOTICE
Comment on lines +14 to +16
The streams-scala (streams/streams-scala) module was donated by Lightbend and the original code was copyrighted by them:
Copyright (C) 2018 Lightbend Inc. <https://www.lightbend.com>
Copyright (C) 2017-2018 Alexis Seigneurin.
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'm not totally comfortable just dropping those notices, so I added this note here. Not sure if it's any better, though.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's better to preserve attribution. Aside from what's required from Apache2 license, what goes into NOTICE is up to us so I think this is a fine location for this.

Copy link
Member

Choose a reason for hiding this comment

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

I agree. We should preserve attribution if we can. "Community first".

build.gradle
@@ -46,6 +46,7 @@ spotless {
scala {
target 'streams/**/*.scala'
scalafmt("$versions.scalafmt").configFile('checkstyle/.scalafmt.conf')
licenseHeaderFile 'checkstyle/java.header', 'package'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This will enforce nothing like this happens again by applying the same check we have for Java via checkstyle.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

One correction, we still don't check python/markdown/gradle/etc. files, so don't let your guard down!

Still, we're now covered for java and scala.

streams/streams-scala/NOTICE
Comment on lines -1 to -3
Kafka Streams Scala
Copyright (C) 2018 Lightbend Inc. <https://www.lightbend.com>
Copyright (C) 2017-2018 Alexis Seigneurin.
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'm pretty sure there's not supposed to be extra NOTICE files in sub-directories of the project.

Copy link
Contributor

Choose a reason for hiding this comment

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

It would definitely not be common/standard

streams/streams-scala/src/main/scala/org/apache/kafka/streams/scala/FunctionConversions.scala
Comment on lines -2 to -4
* Copyright (C) 2018 Lightbend Inc. <https://www.lightbend.com>
* Copyright (C) 2017-2018 Alexis Seigneurin.
*
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These were the blocks that had been wrongly added to the header.

ewencp
ewencp approved these changes Mar 31, 2021
View reviewed changes
Copy link
Contributor

@ewencp ewencp left a comment

Choose a reason for hiding this comment

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

LGTM, I think besides standardizing the Apache header it is also important to preserve information about lineage and attribution, which I think this accomplishes.

@vvcephei vvcephei changed the title KAFKA-12593: Fix header in streams-scala KAFKA-12593: Fix Apache License headers Mar 31, 2021
@vvcephei vvcephei force-pushed the kafka-12593-fix-copyright branch from 2ef8bba to d13ab4d Compare March 31, 2021 21:55
ableegoldman
ableegoldman approved these changes Mar 31, 2021
View reviewed changes
Copy link
Contributor

@ableegoldman ableegoldman left a comment

Choose a reason for hiding this comment

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

Thanks for tackling this

@vvcephei vvcephei merged commit 4ed7f2c into apache:trunk Apr 1, 2021
@vvcephei vvcephei deleted the kafka-12593-fix-copyright branch April 1, 2021 15:38
vvcephei added a commit that referenced this pull request Apr 1, 2021
@vvcephei
KAFKA-12593: Fix Apache License headers (#10452)
ecd7750 
* Standardize license headers in scala, python, and gradle files.
* Relocate copyright attribution to the NOTICE.
* Add a license header check to `spotless` for scala files.

Reviewers: Ewen Cheslack-Postava <ewencp@apache.org>, Matthias J. Sax <mjsax@apache.org>, A. Sophie Blee-Goldman <ableegoldman@apache.org
vvcephei added a commit that referenced this pull request Apr 1, 2021
@vvcephei
KAFKA-12593: Fix Apache License headers (#10452)
aa5a27b 
* Standardize license headers in scala, python, and gradle files.
* Relocate copyright attribution to the NOTICE.
* Add a license header check to `spotless` for scala files.

Reviewers: Ewen Cheslack-Postava <ewencp@apache.org>, Matthias J. Sax <mjsax@apache.org>, A. Sophie Blee-Goldman <ableegoldman@apache.org
vvcephei added a commit that referenced this pull request Apr 1, 2021
@vvcephei
KAFKA-12593: Fix Apache License headers (#10452)
4d1c4d9 
* Standardize license headers in scala, python, and gradle files.
* Relocate copyright attribution to the NOTICE.
* Add a license header check to `spotless` for scala files.

Reviewers: Ewen Cheslack-Postava <ewencp@apache.org>, Matthias J. Sax <mjsax@apache.org>, A. Sophie Blee-Goldman <ableegoldman@apache.org
@vvcephei
Copy link
Contributor Author

vvcephei commented Apr 1, 2021

Cherry-picked to 2.8, 2.7, and 2.6. I doubt anyone's going to do another 2.5 patch release.

ijuma added a commit to ijuma/kafka that referenced this pull request Apr 4, 2021
@ijuma
Merge remote-tracking branch 'apache-github/trunk' into reduce-produc…
1621ca0 
…e-allocations-lz4

* apache-github/trunk: (243 commits)
  KAFKA-12590: Remove deprecated kafka.security.auth.Authorizer, SimpleAclAuthorizer and related classes in 3.0 (apache#10450)
  KAFKA-3968: fsync the parent directory of a segment file when the file is created (apache#10405)
  KAFKA-12283: disable flaky testMultipleWorkersRejoining to stabilize build (apache#10408)
  MINOR: remove KTable.to from the docs (apache#10464)
  MONOR: Remove redudant LocalLogManager (apache#10325)
  MINOR: support ImplicitLinkedHashCollection#sort (apache#10456)
  KAFKA-12587 Remove KafkaPrincipal#fromString for 3.0 (apache#10447)
  KAFKA-12426: Missing logic to create partition.metadata files in RaftReplicaManager (apache#10282)
  MINOR: Improve reproducability of raft simulation tests (apache#10422)
  KAFKA-12474: Handle failure to write new session keys gracefully (apache#10396)
  KAFKA-12593: Fix Apache License headers (apache#10452)
  MINOR: Fix typo in MirrorMaker v2 documentation (apache#10433)
  KAFKA-12600: Remove deprecated config value `default` for client config `client.dns.lookup` (apache#10458)
  KAFKA-12952: Remove deprecated LogConfig.Compact (apache#10451)
  Initial commit (apache#10454)
  KAFKA-12575: Eliminate Log.isLogDirOffline boolean attribute (apache#10430)
  KAFKA-8405; Remove deprecated `kafka-preferred-replica-election` command (apache#10443)
  MINOR: Fix docs for end-to-end record latency metrics (apache#10449)
  MINOR Replaced File with Path in LogSegmentData. (apache#10424)
  KAFKA-12583: Upgrade netty to 4.1.62.Final
  ...
seglo
seglo reviewed Apr 6, 2021
View reviewed changes
Copy link
Member

@seglo seglo left a comment

Choose a reason for hiding this comment

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

@vvcephei I apologize for the delay in getting back to you. My GH notifications were being forwarded to my Lightbend email and I was on vacation for the past week. Debasish is also on leave.

The attribution in the NOTICE LGTM 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mjsax mjsax mjsax approved these changes

@seglo seglo seglo left review comments

@ableegoldman ableegoldman ableegoldman approved these changes

@ewencp ewencp ewencp approved these changes

Assignees
No one assigned
Labels
streams
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@vvcephei @ewencp @seglo @ableegoldman @mjsax