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

Jump to bottom

${prompt.text} and ${prompt.secret} double prompting #11564

Closed
djschny opened this issue Jun 9, 2015 · 10 comments · Fixed by #11668
Closed

${prompt.text} and ${prompt.secret} double prompting #11564

djschny opened this issue Jun 9, 2015 · 10 comments · Fixed by #11668
Assignees
@jasontedor
Labels
>bug :Core/Infra/Settings Settings infrastructure and APIs v5.0.0-alpha1

Comments

@djschny
Copy link
Contributor

djschny commented Jun 9, 2015

With the following configuration

node.name: ${prompt.text}

Elasticsearch 1.6.0 prompts you twice. Once for "node.name" and "name". Ultimately it uses the second one for the value of the configuration item:

djschny:elasticsearch-1.6.0 djschny$ ../startElastic.sh 
Enter value for [node.name]: foo
Enter value for [name]: bar
[2015-06-09 16:10:03,405][INFO ][node                     ] [bar] version[1.6.0], pid[4836], build[cdd3ac4/2015-06-09T13:36:34Z]
[2015-06-09 16:10:03,405][INFO ][node                     ] [bar] initializing ...
@clintongormley clintongormley added >bug :Core/Infra/Settings Settings infrastructure and APIs labels Jun 12, 2015
@clintongormley
Copy link

@jaymode please could you take a look

@jaymode jaymode mentioned this issue Jun 15, 2015
Merged
jaymode added a commit to jaymode/elasticsearch that referenced this issue Jul 8, 2015
@jaymode
do not prompt for node name twice
ae1ed34 
We allow setting the node's name a few different ways: the `name` system
property, the setting `name`, and the setting `node.name`. There is an order
of preference to these settings that gets applied, which can copy values from the
system property or `node.name` setting to the `name` setting. When setting
only `node.name` to one of the prompt placeholders, the user would be
prompted twice as the value of `node.name` is copied to `name` prior to
prompting for input. Additionally, the value entered by the user for `node.name`
would not be used and only the value entered for `name` would be used.

This fix changes the behavior to only prompt once when `node.name is set` and
`name` is not set. This is accomplished by waiting until all values have been
prompted and replaced, then the logic for determining the node's name is
executed.

Closes elastic#11564
jaymode added a commit that referenced this issue Jul 8, 2015
@jaymode
do not prompt for node name twice
118b924 
We allow setting the node's name a few different ways: the `name` system
property, the setting `name`, and the setting `node.name`. There is an order
of preference to these settings that gets applied, which can copy values from the
system property or `node.name` setting to the `name` setting. When setting
only `node.name` to one of the prompt placeholders, the user would be
prompted twice as the value of `node.name` is copied to `name` prior to
prompting for input. Additionally, the value entered by the user for `node.name`
would not be used and only the value entered for `name` would be used.

This fix changes the behavior to only prompt once when `node.name is set` and
`name` is not set. This is accomplished by waiting until all values have been
prompted and replaced, then the logic for determining the node's name is
executed.

Closes #11564
jaymode added a commit that referenced this issue Jul 8, 2015
@jaymode
do not prompt for node name twice
0e080d7 
We allow setting the node's name a few different ways: the `name` system
property, the setting `name`, and the setting `node.name`. There is an order
of preference to these settings that gets applied, which can copy values from the
system property or `node.name` setting to the `name` setting. When setting
only `node.name` to one of the prompt placeholders, the user would be
prompted twice as the value of `node.name` is copied to `name` prior to
prompting for input. Additionally, the value entered by the user for `node.name`
would not be used and only the value entered for `name` would be used.

This fix changes the behavior to only prompt once when `node.name is set` and
`name` is not set. This is accomplished by waiting until all values have been
prompted and replaced, then the logic for determining the node's name is
executed.

Closes #11564
mute pushed a commit to mute/elasticsearch that referenced this issue Jul 29, 2015
@jaymode
do not prompt for node name twice
34ad054 
We allow setting the node's name a few different ways: the `name` system
property, the setting `name`, and the setting `node.name`. There is an order
of preference to these settings that gets applied, which can copy values from the
system property or `node.name` setting to the `name` setting. When setting
only `node.name` to one of the prompt placeholders, the user would be
prompted twice as the value of `node.name` is copied to `name` prior to
prompting for input. Additionally, the value entered by the user for `node.name`
would not be used and only the value entered for `name` would be used.

This fix changes the behavior to only prompt once when `node.name is set` and
`name` is not set. This is accomplished by waiting until all values have been
prompted and replaced, then the logic for determining the node's name is
executed.

Closes elastic#11564
@joshuar
Copy link
Contributor

joshuar commented Dec 17, 2015

Looks to be still a problem on 2.1. Regression here? @GlenRSmith and I can confirm this occurs with the 2.1 release archive.

@joshuar joshuar reopened this Dec 17, 2015
@joshuar
Copy link
Contributor

joshuar commented Dec 17, 2015

It seems to be independent of which config item you want to prompt for:

elasticsearch-2.1.0  bin/elasticsearch
Enter value for [cluster.name]: foo
Enter value for [cluster.name]: bar
[2015-12-18 10:48:50,219][INFO ][node                     ] [Wendell Vaughn] version[2.1.0], pid[21231], build[72cd1f1/2015-11-18T22:40:03Z]
[2015-12-18 10:48:50,220][INFO ][node                     ] [Wendell Vaughn] initializing ...
[2015-12-18 10:48:50,280][INFO ][plugins                  ] [Wendell Vaughn] loaded [], sites []
[2015-12-18 10:48:50,304][INFO ][env                      ] [Wendell Vaughn] using [1] data paths, mounts [[/ (/dev/mapper/fedora_josh--xps13-root)]], net usable_space [79.1gb], net total_space [233.9gb], spins? [no], types [ext4]
[2015-12-18 10:48:51,761][INFO ][node                     ] [Wendell Vaughn] initialized
[2015-12-18 10:48:51,762][INFO ][node                     ] [Wendell Vaughn] starting ...
[2015-12-18 10:48:51,902][INFO ][transport                ] [Wendell Vaughn] publish_address {127.0.0.1:9300}, bound_addresses {127.0.0.1:9300}, {[::1]:9300}
[2015-12-18 10:48:51,912][INFO ][discovery                ] [Wendell Vaughn] bar/azOFxYXMQ6muCpGOpqvxSw

@GlenRSmith
Copy link
Contributor

Just confirmed on 2.1.1.

@rjernst
Copy link
Member

rjernst commented Dec 18, 2015

This is different than the original issue. I think what is happening is we first initialize the settings/environment in bootstrap so that we can eg init logging, but then when bootstrap creates the node, it passes in a fresh Settings, and the Node constructor again initializes settings/environment.

@clintongormley
Copy link

@jaymode could you take a look at this please?

@jaymode
Copy link
Member

jaymode commented Jan 19, 2016

As @rjernst said, this is a different issue that the original. BootstrapCLIParser was added which extends CLITool. CLITool prepares the settings and environment which includes prompting since CLITools are usually run outside of the bootstrap process. This causes the first prompt that is ignored. The second prompt that is used, comes from Bootstrap, which is passed to the node.

Part of the issue is that the BootstrapCLIParser sets properties that will change the value of settings. I think we can solve this a few different ways:

  1. Pass in empty settings/null environment for this CLITool. If we try to create a valid environment here then we have to prepare the settings to ensure we parse the paths from the settings for our directories
  2. Do not use the CLITool infrastructure
  3. Prepare the environment in bootstrap, pass to BootstrapCLIParser. Re-prepare the settings/environment, passing in the already prepared settings.

@spinscale @rjernst any thoughts?

@rjernst
Copy link
Member

rjernst commented Jan 19, 2016

I would say preparing the environment once would be the best option, it will just take some refactoring to pass it through. Running prepare already creates Environment twice (the first time so it can try and load the config file, which might have other paths like plugin path or data path). Really I think we should simply not allow paths to be configured in elasticsearch.yml, instead it should only be through sysprops. It might be something that could simplify the settings/env prep, to make this a little easier.

@jasontedor jasontedor assigned jasontedor and unassigned jaymode Feb 24, 2016
@jasontedor
Copy link
Member

I have a fix for this that will come as part of #16579.

This was referenced Feb 24, 2016
Closed
Closed
@jasontedor
Copy link
Member

Closed by #17024.

@rjernst rjernst mentioned this issue Mar 18, 2016
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@jasontedor jasontedor

Labels
>bug :Core/Infra/Settings Settings infrastructure and APIs v5.0.0-alpha1
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Do not prompt for node name twice jaymode/elasticsearch
Simplify parsing of command-line arguments
8 participants
@joshuar @clintongormley @djschny @rjernst @GlenRSmith @pickypg @jaymode @jasontedor