Skip to content
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 Service.call method and improve restarting of services #242

Merged
merged 9 commits into from May 30, 2014

Conversation

Projects
None yet
2 participants
@vmoravec
Copy link
Contributor

commented May 6, 2014

Currently it's not possible to conveniently call all unit commands from Service module as they are defined by SystemdUnit. This proposal brings following API:

Service.call("reload-or-restart", "sshd") # returns true or false

# in case the command has failed with some error
Service.error # some useful message

The error is logged into y2log.
I also added improved behaviour for SystemdService#restart to reflect the partial results.

@jreidinger

This comment has been minimized.

Copy link
Member

commented May 13, 2014

well, implementation differs from proposal as it need to pass :reload_or_restart. So call to_sym or change documentation.

@@ -50,6 +50,17 @@ def initialize
@error = ""
end

# Send whatever command you need to call for a specific service
# Command name must be a member of instance methods defined in SystemdUnit class

This comment has been minimized.

Copy link
@jreidinger

jreidinger May 13, 2014

Member

specify what happen if it is not instance method.

return super unless installation_system?

# Return false if service was not stopped succesffuly
return false unless stop

This comment has been minimized.

Copy link
@jreidinger

jreidinger May 13, 2014

Member

really? I think it is not backward compatible, as with restart you except running service afterwards and some service failed to stop, because it is e.g. dead adn you want to have it running. What you implement is try_restart see http://www.freedesktop.org/software/systemd/man/systemctl.html#restart%20PATTERN...

@jreidinger

This comment has been minimized.

Copy link
Member

commented May 28, 2014

@vmoravec ping, what is status?

@vmoravec

This comment has been minimized.

Copy link
Contributor Author

commented May 28, 2014

@jreidinger I'm going to push updated code by tomorrow

end

it "raises error if command name or service name parameter is missing" do
expect{ Service.call('start') }.to raise_error

This comment has been minimized.

Copy link
@jreidinger

jreidinger May 30, 2014

Member

you say that it failed if command name or service name is missing, but test only one case. In fact there should be three cases with A, B and A&B missing. But I do not see why to test it as it is common exception of ruby to raise if there is argument number missmatch

This comment has been minimized.

Copy link
@vmoravec

vmoravec May 30, 2014

Author Contributor

Agree, I will remove this test case.

expect { Service.call('make-coffee', 'sshd') }.to raise_error
end

it "returns the result of the original result of the command call" do

This comment has been minimized.

Copy link
@jreidinger

jreidinger May 30, 2014

Member

implementation do not behave like this statement. If so then you need to return response of call, instead you check if it failed and otherwise it return zero. It can be problem with methods like status, not?

This comment has been minimized.

Copy link
@vmoravec

vmoravec May 30, 2014

Author Contributor

Correct, I overlooked the status methods, will adapt the call method to reflect that.

@vmoravec

This comment has been minimized.

Copy link
Contributor Author

commented May 30, 2014

@jreidinger I have updated the code to reflect your comments, the Service.call returns now the original result.

@@ -20,6 +20,23 @@ def stub_service_with method, result
stub_services
end

describe ".call" do

This comment has been minimized.

Copy link
@jreidinger

jreidinger May 30, 2014

Member

OK, now it looks good, just one case missing here. What it do when service is not found? ( in code is failure, what it in fact mean from API side? )

This comment has been minimized.

Copy link
@vmoravec

vmoravec May 30, 2014

Author Contributor

The Service.call('reload', 'not-existing-service') will return false. If you are pointing to a missing test example, it's mssing here, will add one.

This comment has been minimized.

Copy link
@jreidinger

jreidinger May 30, 2014

Member

OK, thanks

This comment has been minimized.

Copy link
@vmoravec

vmoravec May 30, 2014

Author Contributor

Test example added

@jreidinger

This comment has been minimized.

Copy link
Member

commented May 30, 2014

LGTM

vmoravec added a commit that referenced this pull request May 30, 2014

Merge pull request #242 from vmoravec/fix-restart
Add Service.call method and improve restarting of services

@vmoravec vmoravec merged commit 33eb2a9 into yast:master May 30, 2014

@vmoravec vmoravec deleted the vmoravec:fix-restart branch May 30, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.