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

Allow keystore add to handle multiple settings #54229

Merged
merged 4 commits into from
Mar 26, 2020

Conversation

jasontedor
Copy link
Member

Today the keystore add command can only handle adding a single setting/value 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 keystore command to accept adding multiple settings in a single invocation.

Closes #54191

Today the keystore add command can only handle adding a single
setting/value 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 keystore command to accept adding multiple
settings in a single invocation.
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra (:Core/Infra/Core)

@@ -42,13 +46,13 @@
private final OptionSpec<String> arguments;

AddStringKeyStoreCommand() {
super("Add a string setting to the keystore", false);
this.stdinOption = parser.acceptsAll(Arrays.asList("x", "stdin"), "Read setting value from stdin");
super("Add string settings to the keystore", false);
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be worth updating the equivalent text in RemoveSettingKeyStoreCommand? It still talks about removing "a setting" from the keystore, but it already supports multiple settings.

Copy link
Member Author

Choose a reason for hiding this comment

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

I will handle this in a follow up.

Copy link
Member Author

Choose a reason for hiding this comment

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

I opened #54244.


`-s, --silent`:: Shows minimal output.

`--stdin`:: When used with the `add` parameter, you can pass the setting value
through standard input (stdin). See <<add-string-to-keystore>>.
`--stdin`:: When used with the `add` parameter, you can pass the settings values
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we document -x too?

Copy link
Contributor

Choose a reason for hiding this comment

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

I notice we only have -f without --force as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for noticing these. Since these are orthogonal to this change, I will handle them in a follow up.

Copy link
Member Author

Choose a reason for hiding this comment

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

I opened #54242.

@bpintea
Copy link
Contributor

bpintea commented Mar 25, 2020

7.7 being feature frozen, I was wondering if the v7.7.0 label should be removed, or if this should be treated as a blocker or a fix -- considering the tracking for the release notes. Thanks!

@jasontedor jasontedor requested a review from pugnascotia March 25, 2020 21:34
Copy link
Member

@rjernst rjernst left a comment

Choose a reason for hiding this comment

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

LGTM

`add <settings>`:: Adds settings to the keystore. Multiple setting names can be
specified as arguments to the `add` command. By default, you are prompted for
the values of the settings. If the keystore is password protected, you are also
prompted to enter the password. If the setting already exists in the keystore,
Copy link
Member

Choose a reason for hiding this comment

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

the setting -> a setting

@jasontedor jasontedor merged commit e8e8b16 into elastic:master Mar 26, 2020
jasontedor added a commit that referenced this pull request Mar 26, 2020
Today the keystore add command can only handle adding a single
setting/value 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 keystore command to accept adding multiple
settings in a single invocation.
jasontedor added a commit that referenced this pull request Mar 26, 2020
Today the keystore add command can only handle adding a single
setting/value 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 keystore command to accept adding multiple
settings in a single invocation.
@jasontedor jasontedor deleted the keystore-add-multiple-settings branch March 26, 2020 02:59
@jasontedor
Copy link
Member Author

@bpintea This is a late addition to the feature list for 7.7.0 because it's aimed at addressing some issues on Cloud, and we don't want to wait for a fix for those until 7.8.0, which would delay it being in their hands for awhile.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow keystore command to add / remove an arbitrary number of settings at the time
6 participants