Enable TTY password OS tests, plus refactoring (#57759) #58200
Conversation
Two keystore tests were unintentionally ignored when the password-protected keystore work was merged. I've reënabled those tests here. I've also refactored the test methods a little bit to reduce the API surface: instead of having a "startElasticsearchTtyPassword" method and a "startElasticsearchStandardInputPassword" method, I've made a single "startElasticsearch" method with a "useTty" boolean argument.
williamrandolph
added
>test
|
Pinging @elastic/es-core-infra (:Core/Infra/Packaging) |
Centos 6 uses a version of expect that kills the elasticsearch process when it tries to daemonize. I will fix this in future work but for now I'm replacing it with a todo.
| @@ -244,9 +244,10 @@ private static void verifyDefaultInstallation(Installation es, Distribution dist | |||
| // requires the "expect" utility to be installed | |||
| List<String> command = new ArrayList<>(); | |||
| command.add("sudo -E -u %s %s -p %s"); | |||
| if (daemonize) { | |||
| command.add("-d"); | |||
There was a problem hiding this comment.
I'm not sure we should be removing daemonize here, since the method still takes in the flag. Wouldn't this cause any tests using tty but not expecting failure to fail?
There was a problem hiding this comment.
What's going on here is that I want spawn -ignore HUP to let the process keep running in the background after the expect program ends. This is how it works on centos 7, and that's why the tests are passing after this change: it's as if a keyboard user is running without daemonization.
On centos 6, it seems that when expect terminates, it's terminating Elasticsearch too when the -d flag is added.
But I agree that the code shouldn't be merged in this state. I think I can improve it in a couple of ways, including not taking and disregarding a boolean flag in this method.
There was a problem hiding this comment.
I've restored the "daemonize" block, but put a TODO/ignore on the test that would cause the problem. Hopefully the specific issue will be clear to anyone who works on the code. I'm hoping to find a different workaround after more investigation.
mark-vieira
added
Team:Delivery
Two keystore tests were unintentionally ignored when the
password-protected keystore work was merged. I've reënabled those tests
here.
I've also refactored the test methods a little bit to reduce the API
surface: instead of having a "startElasticsearchTtyPassword" method and
a "startElasticsearchStandardInputPassword" method, I've made a single
"startElasticsearch" method with a "useTty" boolean argument.