Skip to content

Conversation

@jaymode
Copy link
Member

@jaymode jaymode commented Jun 5, 2015

In #10918, we introduced the prompt placeholders. These had a different format
than our existing placeholders. This changes the prompt placeholders to follow the
format of the existing placeholders.

Relates to #11455

Note: This PR is targeted to 1.x to make porting to 1.x and 1.6 easier. Will manually port to master.

Copy link
Contributor

Choose a reason for hiding this comment

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

do we really want to ignore missing for prompt?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes we do.

This is still messy for 1.6. At the time this is called, we haven't prompted the user so the PlaceholderResolver isn't able to resolve prompts so they'll always be missing and we need to ignore them. The shouldRemoveMissingPlaceholder method is added so that we don't set the value to an empty string; instead we leave the ${prompt.text} value, which is resolved later on by the InternalSettingsPreparer. We also can't add the prompting here without adding another Map since a placeholder could be in a system property and we add those to the settings multiple times in the InternalSettingsPreparer.

When we implement #11455, we will need to fix this...

Copy link
Contributor

Choose a reason for hiding this comment

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

ok

@uboness
Copy link
Contributor

uboness commented Jun 5, 2015

LGTM

@uboness uboness removed the review label Jun 5, 2015
In elastic#10918, we introduced the prompt placeholders. These were had a different format
than our existing placeholders. This changes the prompt placeholders to follow the
format of the existing placeholders.

Relates to elastic#11455
@jaymode jaymode merged commit 594c830 into elastic:1.x Jun 6, 2015
@clintongormley clintongormley changed the title make prompt placeholders consistent with existing placeholders Make prompt placeholders consistent with existing placeholders Jun 8, 2015
@clintongormley clintongormley added >enhancement :Core/Infra/Settings Settings infrastructure and APIs labels Jun 8, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

:Core/Infra/Settings Settings infrastructure and APIs >enhancement v1.6.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants