-
Notifications
You must be signed in to change notification settings - Fork 24.5k
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
Allow keystore add-file to handle multiple settings #54240
Allow keystore add-file to handle multiple settings #54240
Conversation
Pinging @elastic/es-core-infra (:Core/Infra/Settings) |
Today the keystore add-file command can only handle adding a single setting/file pair in a single invocation. This incurs the startup costs of the JVM many times, which in some environments can be expensive. This commit teaches the add-file keystore command to accept adding multiple settings in a single invocation.
617dfda
to
d9bf373
Compare
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.
Looks good, but I think there is an edge case that needs to be handled.
if (argumentValues.size() == 0) { | ||
throw new UserException(ExitCodes.USAGE, "Missing setting name"); | ||
} | ||
String setting = argumentValues.get(0); | ||
if (argumentValues.size() % 2 != 0) { |
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.
Don't we need to also ensure size > 1? Otherwise I think we silently ignore giving the add-file command with no arguments?
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's handled on the previous if
? If size < 1
then it must be == 0
and we bail?
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.
The if
currently on line 58 of this diff.
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 can't believe I missed that. :shame:
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
Today the keystore add-file command can only handle adding a single setting/file pair in a single invocation. This incurs the startup costs of the JVM many times, which in some environments can be expensive. This commit teaches the add-file keystore command to accept adding multiple settings in a single invocation.
Today the keystore add-file command can only handle adding a single setting/file pair in a single invocation. This incurs the startup costs of the JVM many times, which in some environments can be expensive. This commit teaches the add-file keystore command to accept adding multiple settings in a single invocation.
Today the keystore add-file command can only handle adding a single setting/file pair in a single invocation. This incurs the startup costs of the JVM many times, which in some environments can be expensive. This commit teaches the add-file keystore command to accept adding multiple settings in a single invocation.
Relates #54191