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

Fixes the linting rules to accept only the approved copyright headers #1373

Merged
merged 3 commits into from
Mar 25, 2022

Conversation

AMoo-Miki
Copy link
Collaborator

@AMoo-Miki AMoo-Miki commented Mar 23, 2022

Signed-off-by: Miki [email protected]

Description

The approved copyright headers are either the short, two-liner AL2 one for new files, or the longer version for modified files that probably already have a copyright header; a split copyright header is not.

This also changes the existing headers:

  • New files with no open source origins: Short 2-liner block
  • Modified or Unmodified files with open source origins: Large header block

Issues Resolved

#1382

Check List

  • New functionality includes testing.
    • All tests pass
      • yarn test:jest
      • yarn test:jest_integration
      • yarn test:ftr
  • New functionality has been documented.
  • Commits are signed per the DCO using --signoff

@AMoo-Miki AMoo-Miki requested a review from a team as a code owner March 23, 2022 18:51
@kavilla
Copy link
Member

kavilla commented Mar 23, 2022

is it failing because the license header needs to be updated for other files now?

.eslintrc.js Outdated Show resolved Hide resolved
@AMoo-Miki
Copy link
Collaborator Author

is it failing because the license header needs to be updated for other files now?

The test should have only validated the changed files and not all files. However, after discussing with others, we decided that I will update all the copyright headers.

@AMoo-Miki AMoo-Miki force-pushed the fix-lic-lint-rule branch 2 times, most recently from 30e1510 to e6a0de0 Compare March 24, 2022 18:45
@ashwin-pc
Copy link
Member

Do we need to remove the modified section from each of the unmodified files? If we do modify them in future but forget to add the modification header, we will once again have an incorrect license header. What are your thoughts on removing the OSD_UNMODIFIED_HEADER all together and just have the header be either the new header or the modified one?

@AMoo-Miki
Copy link
Collaborator Author

AMoo-Miki commented Mar 24, 2022

Do we need to remove the modified section from each of the unmodified files?

I think you mean "add". Yes, contributors modifying a file with the unmodified header will need to add the extra lines.

If we do modify them in future but forget to add the modification header, we will once again have an incorrect license header.

Yes. Code reviewers will need to be vigilant. Or, we would need to come up with a custom license validation script. I have a script that I built to partially handle these headers which could be repurposed to do this in the future.

What are your thoughts on removing the OSD_UNMODIFIED_HEADER all together and just have the header be either the new header or the modified one?

What I learned from this previous failures of the method we currently use with the header validations, if we removed OSD_UNMODIFIED_HEADER, all the files that use that header currently will cause failures.

@ashwin-pc
Copy link
Member

What I learned from this previous failures of the method we currently use with the header validations, if we removed OSD_UNMODIFIED_HEADER, all the files that use that header currently will cause failures.

True, but once we update all the headers like you are doing right now to only use the modified header, we should no longer have any incorrect headers. Then OSD_UNMODIFIED_HEADER becomes unnecessary.

@AMoo-Miki
Copy link
Collaborator Author

AMoo-Miki commented Mar 25, 2022

True, but once we update all the headers like you are doing right now to only use the modified header, we should no longer have any incorrect headers. Then OSD_UNMODIFIED_HEADER becomes unnecessary.

Ashwin's concerns were about the potential for error when contributors update a file that had the "unmodified" headers and forgot to update it. We found a compromise that simplifies the headers a lot and addresses Ashwin's concern. The updated guidance is:

  • New files with no open source origins: Short 2-liner block
  • Modified or Unmodified files with open source origins: Large header block (which says "any modifications ...")

@kavilla kavilla linked an issue Mar 25, 2022 that may be closed by this pull request
New files with no open source origins:
  * Short 2-liner block
Modified or Unmodified files with open source origins:
  * Large header block

Signed-off-by: Miki <[email protected]>
@AMoo-Miki AMoo-Miki force-pushed the fix-lic-lint-rule branch 2 times, most recently from eea6b56 to a98870d Compare March 25, 2022 16:39
Copy link
Contributor

@tmarkley tmarkley 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 Miki!

@kavilla
Copy link
Member

kavilla commented Mar 25, 2022

Our builds get built with a NOTICE.txt.

Is this still valid:

OpenSearch
Copyright 2022 OpenSearch Contributors

Copy link
Member

@kavilla kavilla left a comment

Choose a reason for hiding this comment

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

Just one question but:

ooooo          .oooooo.    ooooooooooooo ooo        ooooo 
`888'         d8P'  `Y8b   8'   888   `8 `88.       .888' 
 888         888                888       888b     d'888  
 888         888                888       8 Y88. .P  888  
 888         888     ooooo      888       8  `888'   888  
 888       o `88.    .88'       888       8    Y     888  
o888ooooood8  `Y8bood8P'       o888o     o8o        o888o 

@AMoo-Miki
Copy link
Collaborator Author

Our builds get built with a NOTICE.txt.

Is this still valid:

OpenSearch
Copyright 2022 OpenSearch Contributors

I will dig up some correspondence about this and deal with it in a separate PR.

@AMoo-Miki AMoo-Miki merged commit d7004dc into opensearch-project:main Mar 25, 2022
@opensearch-trigger-bot
Copy link
Contributor

The backport to 1.x failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-1.x 1.x
# Navigate to the new working tree
cd .worktrees/backport-1.x
# Create a new branch
git switch --create backport/backport-1373-to-1.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 d7004dc5b0392477fdd54ac66b29d231975a173b
# Push it to GitHub
git push --set-upstream origin backport/backport-1373-to-1.x
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-1.x

Then, create a pull request where the base branch is 1.x and the compare/head branch is backport/backport-1373-to-1.x.

tmarkley pushed a commit to tmarkley/OpenSearch-Dashboards that referenced this pull request Mar 26, 2022
* Fixes the linting rules to accept only the approved copyright headers
* Fixes the license headers
  New files with no open source origins:
    * Short 2-liner block
  Modified or Unmodified files with open source origins:
    * Large header block
* Updates the snapshots that changed with updates to the license headers

Signed-off-by: Miki <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] Inconsistent license headers
4 participants