-
Notifications
You must be signed in to change notification settings - Fork 25k
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
Add package test for bad data.path setting from #23981 #24029
Conversation
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.
Left two comments
# WARNING: This testing file must be executed as root and can | ||
# dramatically change your system. It removes the 'elasticsearch' | ||
# user/group and also many directories. Do not execute this file | ||
# unless you know exactly what you are doing. |
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.
Hmm... wonder if we should attempt something like https://stackoverflow.com/questions/28887046/how-to-tell-if-i-am-inside-a-vagrant-host#28909819 inside of our Vagrantfiles?
Then we could abort if someone tried running this outside of Vagrant
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.
We certainly could try it.
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.
As a followup?
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.
As a followup?
Sure, a followup sounds fine to me
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 opened an issue so I don't forget. I've left it as adoptme for now because my stack is getting high. #24137
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.
But I'll leave it on my list and take it when I can if no one else gets to it first.
} | ||
|
||
@test "[BAD data.path] start" { | ||
start_elasticsearch_service green |
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.
Is there any way we can ensure that starting ES here does use default.path.data
?
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.
Like, when you don't configure anything? We kind of do that here. Depends on what you mean though.
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 mean, if in the future we stop putting -Edefault.path.data=/var/lib/elasticsearch
in the startup scripts and instead put path.data: /var/lib/elasticsearch
into elasticsearch.yml
during installation, this wouldn't exercise the bug
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 see. I'll see about reproducing this from the tar package as well then.
@dakrone - I pushed an update that adds a test that explicitly uses |
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, thanks Nik, I think with Jason's recent changes that fix the bug, you should be able to remove the TODOs and put the proper asserts back
Thanks for reviewing @dakrone! I've flipped the condition and verified locally. I'll merge and cherry-pick soon. |
Adds a packaging test that would have detected #23981. The test configures path.data and validates that nothing ends up in default.path.data.
Adds a packaging test that would have detected #23981. The test configures path.data and validates that nothing ends up in default.path.data.
Adds a packaging test that would have detected #23981. The test configures path.data and validates that nothing ends up in default.path.data.
Adds a packaging test that would have detected #23981.