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

Improve License Header Check #78

Merged

Conversation

tfarrep
Copy link
Contributor

@tfarrep tfarrep commented Aug 10, 2022

Description

Improves the license header check to only check for the SPDX header instead of the older header with explicit grants to Elasticsearch. This involves separating the old header block into two in all C# files to keep the checker happy.

I've split this into separate commits to see the "real" changes easier.
The replacement inside the C# files was entirely "mechanical" via:

perl -i -p0e 's|license\.\n\*\n\* Modifications|license.\n*/\n/*\n* Modifications|s' $(find . -iname '*.cs')

Issues Resolved

Closes #69

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

```sh
perl -i -p0e 's|license\.\n\*\n\* Modifications|license.\n*/\n/*\n* Modifications|s' $(find . -iname '*.cs')
```

Signed-off-by: Thomas Farr <[email protected]>
@tfarrep tfarrep requested review from a team and Yury-Fridlyand as code owners August 10, 2022 01:37
@tfarrep
Copy link
Contributor Author

tfarrep commented Aug 10, 2022

The "true" diff can be checked locally, ignoring lines that are solely *, /*, or */:

git diff --ignore-matching-lines '^(\*|/\*|\*/)$' main fix/license-header-check

Copy link
Member

@joshuali925 joshuali925 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

The problem is that with approach (1) some of those changes (lines 6-8 in screenshot) are required as it means breaking one block comment into two. Otherwise it'd just be (2).

breaking comments into two blocks shouldn't be an issue

@Yury-Fridlyand
Copy link
Collaborator

@joshuali925, @dblock, should we add license header to non-code files? e.g. bash scripts, workflows and so on?

@dblock
Copy link
Member

dblock commented Aug 17, 2022

@joshuali925, @dblock, should we add license header to non-code files? e.g. bash scripts, workflows and so on?

Yes, I believe that's the right thing to do. But don't go crazy, like I rarely put those in GHA workflows for example. It only matters when people copy stuff.

Copy link

@wbeckler wbeckler left a comment

Choose a reason for hiding this comment

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

LGTM

@wbeckler wbeckler merged commit 6703cd4 into opensearch-project:main Aug 31, 2022
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.

[PROPOSAL] Improving the license header check
5 participants