-
Couldn't load subscription status.
- Fork 25.6k
Initialization class as argument to EntitlementAgent #127815
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
Initialization class as argument to EntitlementAgent #127815
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. A couple of thoughts I had while reviewing...
| final Class<?> initClazz; | ||
| try { | ||
| initClazz = Class.forName("org.elasticsearch.entitlement.initialization.EntitlementInitialization"); | ||
| initClazz = Class.forName(agentArgs); |
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.
Making the arguments just be the name of the class, rather than some kind of option flags, doesn't allow much evolution in the future, but then, we are in control of all of this code and change it in the future if need be, just like we did this time, so it doesn't actually matter much.
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.
I had exactly the same conversation in my head :D wondering if I should do that, and thinking we can change this any time anyway, so I decided to let it like this for now. Unless you or someone else feels strongly about this.
| } | ||
|
|
||
| private static String findAgentJar() { | ||
| public static String findAgentJar() { |
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.
I'm not sure why this became public, but I presume that will become clear in an upcoming PR.
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.
This can be package private, right? The TestEntitlementBootstrap can be in the same entitlement bootstrap package (split packages are ok when using the classpath)
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.
Yes sorry this change slipped in (see the linked PR).
If split packages are OK for tests (as we use the classpath) I will change them the package of TestEntitlementBootstrap etc. and make this (and other method/classes) package-private then.
Preliminary step for test entitlement initialization, extracted from elastic#127814
Preliminary step for test entitlement initialization, extracted from elastic#127814
Preliminary step for test entitlement initialization, extracted from elastic#127814
Preliminary step for test entitlement initialization, extracted from elastic#127814
Preliminary step for test entitlement initialization, extracted from elastic#127814
Preliminary step for test entitlement initialization, extracted from #127814