-
Notifications
You must be signed in to change notification settings - Fork 25.6k
Removed unused "else" branch for no entitlements/no SM #122354
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
Removed unused "else" branch for no entitlements/no SM #122354
Conversation
|
Pinging @elastic/es-core-infra (Team:Core/Infra) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
| ); | ||
| } else if (RuntimeVersionFeature.isSecurityManagerAvailable()) { | ||
| } else { | ||
| assert RuntimeVersionFeature.isSecurityManagerAvailable(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An assert won't run in production/snapshot code, so if eg someone testing a snapshot or RC tries running with JDK24, they will get a nasty error from trying to use SM below. Could we make this more explicit with a helpful error message?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That like will always be true unless we make a change to code - the change in your PR ensures it. I added the assert to protect against ourselves (if we change something, and open the possibility of having neither, things in CI would start to fail).
I can make the change if you want of course, but no one should ever see that error message.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps I'm still missing something: wouldn't a user that say uses a custom JVM of 24 on a snapshot of 9.0 beta hit this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We already guarantee we use entitlements when we have a JVM 24:
final boolean useEntitlements = RuntimeVersionFeature.isSecurityManagerAvailable() == false || entitlementsExplicitlyEnabled;
(initPhase1)
So with 24 we always hit the entitlements branch.
The assert is here in case we change that logic and we don't realize it, leaving us uncovered again, but it is not strictly needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, thank you! That's what I was missing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
We are already defaulting to entitlements for JDK24+ in #119885; this PR removes some dead code we had left in bootstrap.