Followup fix for #1162: Add support for not (re)starting server after cloud-setup-management. - #1330
Followup fix for #1162: Add support for not (re)starting server after cloud-setup-management.#1330ProjectMoon wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
so if nostart is false (start is true ??) the service is stopped. Am I understanding this wrong? it seems like we don't want this here and not unconditionally either but only if nostart is true.
There was a problem hiding this comment.
This is the original behavior of the script, before --no-start was added. When --no-start is NOT specified, the script tries to make sure that the service is stopped before starting it again.
|
LGTM, this change fixes the logic regarding the expected state of the service prior to running the script. |
|
complaining about a license, false positive: server/test/com/cloud/event/ActionEventUtilsTest.java |
|
LGTM, pinging @remibergsma |
|
2xLGTM, no test results. Did anyone verify this works? |
|
@remibergsma @ProjectMoon did himself, as I read it. This is a systemd installation script it won't be touched by the integration tests. You are not satisfied with this? |
|
@DaanHoogland This is not systemd, this is a python script to configure management. Why don't we build an environment and try this script with its parameters. Even if there's no integration test, we should still test/verify the changes IMHO. |
|
I know, I said 'systemd install script'. I am not taking the time to test this now. I can ask @davidamorimfaria . David? |
|
Perhaps I can make a small example test that starts cloudstack-setup-management under systemd with both |
|
LGTM, let's merge after some tests |
It will only run the stop command when --no-start is specified.
|
PR updated to latest 4.6. |
|
Also, forgot I was making a test case for this. Will update my agenda to get this done. |
|
Link to logs Folder (search by build_no): https://www.dropbox.com/sh/yj3wnzbceo9uef2/AAB6u-Iap-xztdm6jHX9SjPja?dl=0 ACS CI BVT RunSumarry: Failed tests:
Skipped tests: Passed test suits: |
|
Team, Where are we currently on this PR? |
|
Right now I suppose I still need to make a unit test. And I guess also re-open against 4.7. |
|
@ProjectMoon thanks, please open against master; unlikely we'll do 4.7.x 4.8.x releases |
|
We are still accepting bug fixes in 4.7 and 4.8, but all features should be added to master. Anything merged into 4.7 or 4.8 will be forward merged up through master. I agree, this probably should be opened against master. |
|
Closing to open against master. |
[CLOUDSTACK-9645] Followup fix for #1162: Add support for not (re)starting server after cloud-setup-management.New version against #1330, opened against master. ``` Moving stop of management in config inside if statement. It will only run the stop command when --no-start is specified. ``` * pr/1566: CLOUDSTACK-9645: Moving stop of management in config inside if statement. Signed-off-by: Rohit Yadav <rohit.yadav@shapeblue.com>
This is a follow-up fix for pull request #1162, which added a
--no-startoption to cloudstack-setup-management. It is necessary for further script execution under systemd, otherwise the service aborts.