Add license headers to files. #511
Conversation
|
Do we really need these license headers, though? I guess it mattered more in the past when Zipkin was a formal Apache Incubator project, but now it's an independent organization that simply chooses to license its software under ASL 2. There's no legal requirement for including license headers, it's more like a recommendation. At best, including the headers could give some value in theory, but the Apache license is already bundled in the project anyways. The main issue with license headers is that they tend to be forgotten about/unmaintained, and only serve as tech debt. Anyways, that's my 2c. |
|
@eirslett Thanks for quick reply, you were right. and this PR is just a suggestion. |
|
It's pretty common now adays to have license headers so I think it could make sense to have them here too, though no strong feeling about it. But do feel strongly that we can't add license headers without also adding tooling to ensure new files have the license header as part of the build. |
|
@anuraaga Maybe we can add header rule in |
|
Yeah I think that would be fine. Let's hear opinions from other maintainers on their thoughts before adding it :) |
|
any input @adriancole, @petermetz
|
|
My drive by 2p: I agree with @eirslett that it isn't purely needed. If we were to do this, I like using the shortened one-line comments as it reduces the impact on people depending on us. We should also for the sake of sanity think similarly between here and lens (cc @tacigar ) though that isn't required either. Regardless, the date isn't really correct, usually we get the date range from git, not sure options available, but then this isn't super important. |
@jcchavezs I agree with @anuraaga regarding the tooling: |
|
@dengliming any idea on how address @anuraaga and @petermetz's concern regarding tooling? |
@jcchavezs I've seen checks like this enforced while sending a PR to webpack recently. Never looked into the details, but if nobody on this thread is aware of any existing tools then I'd start there and just see what they've got and take it from there. |
|
@jcchavezs Sorry for late reply. I haven't thought of a good way yet. @petermetz I'd really appreciate it if you could help. Thanks. |
@dengliming Don't have the bandwidth right now unfortunately. Apart from the pointers I've given above I have just another link that may help with implementing pre-commit hook checks if that's what you'd want. |
| "radix": ["error", "as-needed"] | ||
| }, | ||
| "radix": ["error", "as-needed"], | ||
| "header/header": [2, "line", [{"pattern": " Copyright 2020 The OpenZipkin Authors; licensed to You under the Apache License, Version 2.0."}]]}, |
There was a problem hiding this comment.
how do we want to handle this date.. manually change it? In other projects we are deriving this from git commit info, so that it says like 2016-2020 on file change.
(feel free to merge without addressing this question)
There was a problem hiding this comment.
In other projects we are deriving this from git commit info.
That‘s great. and I'm a little confused. Do we really have to keep copyright year and then change it once a year? Personally I suggest we remove copyright Year like other projects. open-telemetry/opentelemetry-java : )
There was a problem hiding this comment.
I suspect there's something stronger by saying the year range. However, I'd suggest either do it right or leave it out. I wouldn't necessarily do anything just because otel does though :D
There was a problem hiding this comment.
Thanks for sharing your thought. This is only a suggestion, for reference only. : ).
In other projects we are deriving this from git commit info.
We can try to do this. I know maven plugin can do this (not sure js). It would be better If you can give me a link?
There was a problem hiding this comment.
cool. here's the code used in the maven plugin, if that helps.. https://github.com/mycila/license-maven-plugin/blob/e4675b61308ccd6bbf0fd2bd6a1aa309973ea048/license-maven-plugin-git/src/main/java/com/mycila/maven/plugin/license/git/CopyrightRangeProvider.java#L79-L80
There was a problem hiding this comment.
note if we do this, we need a well commented thing on jobs to do full, not shallow git clone. This is a pitfall of using the git range, as it can be inaccurate if for example, you only get the last 5 commits.
There was a problem hiding this comment.
let's merge this to help avoid rebase land
@dengliming thanks so much and thanks rest for the feedback!
|
Thanks! |
for #484
@jcchavezs Can you help to review these changes?