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

Do not reference values for filtered settings #48066

Merged
merged 5 commits into from
Oct 23, 2019

Conversation

danhermann
Copy link
Contributor

@danhermann danhermann commented Oct 15, 2019

Validation exceptions are often logged so the value of filtered properties (of any data type) shouldn't be included.

@danhermann danhermann added the :Core/Infra/Settings Settings infrastructure and APIs label Oct 15, 2019
@elasticmachine
Copy link
Collaborator

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

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

@danhermann
Copy link
Contributor Author

Thanks, @rjernst. Any opinion on whether the same logic should be applied to typed settings such as int, float, date?

@rjernst
Copy link
Member

rjernst commented Oct 15, 2019

Seems like it should apply to any settings marked as filtered, regardless of type.

@danhermann
Copy link
Contributor Author

👍 I'll make the same changes for all the types, then.

@danhermann
Copy link
Contributor Author

@elasticmachine update branch

@danhermann
Copy link
Contributor Author

@elasticmachine run elasticsearch-ci/packaging-sample

@danhermann
Copy link
Contributor Author

@rjernst, my second commit on this PR removes any mention of values from validation errors on all typed settings with the Property.Filtered flag. Settings without that flag are unchanged.

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.

Thanks, looks good. Just a comment on some tests.

Settings build = Settings.builder().put("foo.bar", "I am not a boolean").build();
try {
settingUpdater.apply(build, Settings.EMPTY);
fail("not a boolean");
Copy link
Member

Choose a reason for hiding this comment

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

Please use expectThrows instead of try/fail/catch

try {
predicateSettingUpdater.apply(Settings.builder().put("foo.bar.1.value", "1").put("foo.bar.2.value", "2").build(),
Settings.EMPTY);
fail("not accepted");
Copy link
Member

Choose a reason for hiding this comment

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

same here, use expectThrows

@danhermann
Copy link
Contributor Author

@elasticmachine update branch

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.

5 participants