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

OAK-11199 getSubject is supported only if a security manager is allowed #1891

Merged
merged 10 commits into from
Dec 17, 2024

Conversation

mbaedke
Copy link
Contributor

@mbaedke mbaedke commented Dec 9, 2024

No description provided.

@mbaedke mbaedke marked this pull request as draft December 9, 2024 17:26
Copy link

github-actions bot commented Dec 9, 2024

Commit-Check ❌

Commit rejected by Commit-Check.                                  
                                                                  
  (c).-.(c)    (c).-.(c)    (c).-.(c)    (c).-.(c)    (c).-.(c)  
   / ._. \      / ._. \      / ._. \      / ._. \      / ._. \   
 __\( C )/__  __\( H )/__  __\( E )/__  __\( C )/__  __\( K )/__ 
(_.-/'-'\-._)(_.-/'-'\-._)(_.-/'-'\-._)(_.-/'-'\-._)(_.-/'-'\-._)
   || E ||      || R ||      || R ||      || O ||      || R ||   
 _.' '-' '._  _.' '-' '._  _.' '-' '._  _.' '-' '._  _.' '-' '._ 
(.-./`-´\.-.)(.-./`-´\.-.)(.-./`-´\.-.)(.-./`-´\.-.)(.-./`-´\.-.)
 `-´     `-´  `-´     `-´  `-´     `-´  `-´     `-´  `-´     `-´ 
                                                                  
Commit rejected.                                                  
                                                                  
Type message check failed => OAK-11199: Java 23: getSubject is supported only if a security manager is allowed

Changed class name, added tests.
 
It doesn't match regex: ^OAK-\d+\s\S+.*
The commit message must start with 'OAK-<ID> ' followed by some descriptive text
Suggest: Please check your commit message whether it matches above regex

Copy link
Contributor

@reschke reschke left a comment

Choose a reason for hiding this comment

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

a) Compatibility
b) The new package in commons would need a package-info

Otherwise, looks good.

@mbaedke mbaedke marked this pull request as ready for review December 13, 2024 11:29
@reschke
Copy link
Contributor

reschke commented Dec 13, 2024

Recommendation:

  • new package commons.jdkcompat (non-public, "internal" in package-info), export in pom.xml
  • name of util class more specific, like Java23Security

…r is allowed

Introduced new package to allow for separate versioning; added comments.
@mbaedke mbaedke requested a review from reschke December 13, 2024 13:17
Copy link
Contributor

@reschke reschke left a comment

Choose a reason for hiding this comment

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

The changes in commons now look good.

The other parts of the PR are minimal, which is good.

I haven't tested, I assume you did, @mbaedke

The one thing that came to mind is that if we change Java23Security to "Subject", changes would be even less intrusive. Or even "Java23Subject"? (this is bikeshedding, ignore if you want to)

@reschke reschke self-requested a review December 16, 2024 10:15
Copy link
Contributor

@reschke reschke left a comment

Choose a reason for hiding this comment

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

@mbaedke
Copy link
Contributor Author

mbaedke commented Dec 16, 2024

I like Java23Subject. I wouldn't call it Subject at least for the reason that the method signatures are slightly different, which might confuse readers. Of course I tested, but will run integration tests again and look into the coverage issue before merging.

…r is allowed

Changed class name, added tests.
Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
22.8% Coverage on New Code (required ≥ 80%)

See analysis details on SonarQube Cloud

@mbaedke mbaedke merged commit e2ce76c into trunk Dec 17, 2024
3 of 6 checks passed
@mbaedke mbaedke deleted the issue/oak-11199-2 branch December 17, 2024 09:09
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.

2 participants