-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Followup fix for #1162: Add support for not (re)starting server after cloud-setup-management. #1330
Conversation
if self.syscfg.env.noStart == False: | ||
self.syscfg.svo.stopService("cloudstack-management") |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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-start
option to cloudstack-setup-management. It is necessary for further script execution under systemd, otherwise the service aborts.