-
Notifications
You must be signed in to change notification settings - Fork 24.8k
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
Migrate SysV init tests from bats to java packaging #51077
Conversation
This commit converts the sysv init tests from bats tests into the java packaging tests. Since it is the last oss specific test, the bats oss test task is also removed. relates elastic#46005
Pinging @elastic/es-core-infra (:Core/Infra/Packaging) |
public static void filterDistros() { | ||
assumeTrue("rpm or deb", distribution.isPackage()); | ||
assumeTrue(Platforms.isSysVInit()); | ||
assumeTrue(Platforms.isSystemd()); |
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.
Why do we need this check?
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.
It actually should be assumeFalse. This is necessary because some systems have both sysv init and systemd, where sysv commands are a thin layer over systemd. This assume is meant to ensure we only test on systems with true sysv init support. Without it the tests will fail as we are testing behavior specifically in our init.d script, which would not be run in the hybrid systems.
sh.run("service elasticsearch stop"); | ||
} | ||
|
||
public void assertStatus(int code, String msg) { |
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 this used?
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.
Leftover from a previous iteration. Removed.
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 found on thing that looks like it needs updating. Otherwise I think this is good.
|
||
@Override | ||
public void startElasticsearch() throws Exception { | ||
// since some systems have both sysv and systemd, we need to be explicit here to use sysv |
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 looks like we skip this when systemd is around.
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.
Indeed this comment is out of date. I do like being more epxlicit here and using the service
calls directly , rather than relying on the parent class to decide to use sysv.
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 apart from the out-of-date comment.
This commit converts the sysv init tests from bats tests into the java packaging tests. Since it is the last oss specific test, the bats oss test task is also removed. relates elastic#46005
This commit converts the sysv init tests from bats tests into the java
packaging tests. Since it is the last oss specific test, the bats oss
test task is also removed.
relates #46005