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

Updates license headers #1829

Merged
merged 2 commits into from
Jun 8, 2022

Conversation

DarshitChanpura
Copy link
Member

@DarshitChanpura DarshitChanpura commented May 6, 2022

Description

Updates license headers

  • Category : Bug fix

Issues Resolved

Check List

  • Commits are signed per the DCO using --signoff

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.

@codecov-commenter
Copy link

codecov-commenter commented May 6, 2022

Codecov Report

Merging #1829 (999305d) into main (ac3eca8) will decrease coverage by 0.01%.
The diff coverage is n/a.

@@             Coverage Diff              @@
##               main    #1829      +/-   ##
============================================
- Coverage     60.89%   60.87%   -0.02%     
  Complexity     3216     3216              
============================================
  Files           256      256              
  Lines         18013    18013              
  Branches       3211     3211              
============================================
- Hits          10969    10966       -3     
- Misses         5465     5466       +1     
- Partials       1579     1581       +2     
Impacted Files Coverage Δ
...ic/auth/http/jwt/AbstractHTTPJwtAuthenticator.java 55.81% <ø> (ø)
...mazon/dlic/auth/http/jwt/HTTPJwtAuthenticator.java 85.71% <ø> (ø)
...t/keybyoidc/AuthenticatorUnavailableException.java 0.00% <ø> (ø)
...th/http/jwt/keybyoidc/BadCredentialsException.java 40.00% <ø> (ø)
...byoidc/HTTPJwtKeyByOpenIdConnectAuthenticator.java 94.11% <ø> (ø)
...azon/dlic/auth/http/jwt/keybyoidc/JwtVerifier.java 85.71% <ø> (ø)
.../dlic/auth/http/jwt/keybyoidc/KeySetRetriever.java 76.82% <ø> (ø)
.../auth/http/jwt/keybyoidc/SelfRefreshingKeySet.java 59.85% <ø> (ø)
...ttp/jwt/oidc/json/OpenIdProviderConfiguration.java 100.00% <ø> (ø)
...ic/auth/http/kerberos/HTTPSpnegoAuthenticator.java 0.00% <ø> (ø)
... and 201 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ac3eca8...999305d. Read the comment docs.

@peternied
Copy link
Member

@DarshitChanpura Any progress on this? I'd recommend, if we are unsure of next steps, close the review until it is in state to review/get feedback and then open a fresh pull request.

@DarshitChanpura
Copy link
Member Author

@opensearch-project/security team, let's get this reviewed and merged and then we can open a follow-up issue to generate new license headers on new file creation.

@DarshitChanpura DarshitChanpura marked this pull request as ready for review June 1, 2022 17:55
@DarshitChanpura DarshitChanpura requested a review from a team June 1, 2022 17:55
Copy link
Member

@peternied peternied left a comment

Choose a reason for hiding this comment

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

Taking this change without being sure that we correctly updated all of the files makes it hard to know if this is the correct pull request to merge. I would rather we solved the problem of detecting incorrect license headers first and then rolled out the fix for the improperly documented files.

I believe that OpenSearch already has a process for doing this, can we see if we can adopt it or a similar process?

@DarshitChanpura
Copy link
Member Author

Taking this change without being sure that we correctly updated all of the files makes it hard to know if this is the correct pull request to merge. I would rather we solved the problem of detecting incorrect license headers first and then rolled out the fix for the improperly documented files.

I believe that OpenSearch already has a process for doing this, can we see if we can adopt it or a similar process?

The license headers are updated according to the step mentioned in the bug attached here. I have replaced all the headers similar to this in all the files to match the header mentioned here.

The follow-up to this would to add automated checks i.e. one which you mentioned in the comment, using a groovy task or an alternative in our project which would need more time.

I did try by adding a task using spotless, but that would replace all the license headers with the new one which is not correct, so ended up removing it and would need an extra set of hands in getting this fix through.

@peternied
Copy link
Member

The follow-up to this would to add automated checks i.e. one which you mentioned in the comment, using a groovy task or an alternative in our project which would need more time.

Could we add a check for the bad license header to make sure we don't add any more violations like these back in? I don't think we need a 'license header corrector' tool

@DarshitChanpura DarshitChanpura force-pushed the license-headers branch 5 times, most recently from f705ecf to 999305d Compare June 3, 2022 01:05
@@ -204,4 +204,12 @@
<property name="severity" value="warning"/>
</module>

<!-- Checks for Headers to not contain old license text -->
<module name="RegexpSingleline">
<property name="format" value="Portions Copyright OpenSearch Contributors"/>
Copy link
Member Author

Choose a reason for hiding this comment

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

For now I'm checking the starting line of the old license header to see if any files contain it, if so checkstyle will throw violation.

@DarshitChanpura
Copy link
Member Author

The follow-up to this would to add automated checks i.e. one which you mentioned in the comment, using a groovy task or an alternative in our project which would need more time.

Could we add a check for the bad license header to make sure we don't add any more violations like these back in? I don't think we need a 'license header corrector' tool

Done

config/checkstyle/sun_checks.xml Outdated Show resolved Hide resolved
@DarshitChanpura
Copy link
Member Author

should we update NOTICE.txt?

Signed-off-by: Darshit Chanpura <[email protected]>
@peternied
Copy link
Member

should we update NOTICE.txt?

I don't think we should modify the NOTICE.txt at this time.

@cliu123 cliu123 merged commit f431ec2 into opensearch-project:main Jun 8, 2022
cliu123 added a commit that referenced this pull request Jun 8, 2022
stephen-crawford pushed a commit to stephen-crawford/security that referenced this pull request Nov 10, 2022
Signed-off-by: Darshit Chanpura <[email protected]>
Signed-off-by: Stephen Crawford <[email protected]>
peternied added a commit that referenced this pull request Dec 6, 2022
Windows build and test support for 1.3

- Use most recent format of CI workflows from main
- Backport #2206
- Supply workarounds for JDK8 incompatible APIs for Encoding / Pattern matching (Thanks @cwperks!)
- Backport only freeport logic from #1638
- Backport #1758
- All updates to TestAuditlogImpl.java from main
  - #2180
  - #1935 
  - #1920
  - #1914
  - #1829 
  - And Targeted test updates for ComplianceAuditlogTest and BasicAuditlogTest to keep up with TestAuditlogImpl.java updates

Signed-off-by: Peter Nied <[email protected]>
Signed-off-by: Stephen Crawford <[email protected]>
Signed-off-by: Stephen Crawford <[email protected]>
Co-authored-by: Stephen Crawford <[email protected]>
wuychn pushed a commit to ochprince/security that referenced this pull request Mar 16, 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.

[BUG] Incorrect license headers
4 participants