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

[RFC] Add exception to distinguish a `systemctl show` fail from others #804

Closed
wants to merge 3 commits into from

Conversation

dgdavid added 2 commits Aug 21, 2018
Raise an specific exception for a `systemctl show` timeout
Which allows to distinguish an error refreshing the unit (usually a
service) from an error in the rest of actions.
@dgdavid

This comment has been minimized.

Copy link
Member Author

commented Aug 21, 2018

Guys, this PR it is only to discuss it this changes are enough to solve the https://trello.com/c/XZUCetog/160-create-new-exception-for-failure-when-refreshing-a-service or we should take the opportunity to improve the design of Yast::Systemctl#execute as we were talking in the #yast freenode channgel.

If so, could you please give me a hand in order to not break everything related to Yast::Systemctl#execute?

@dgdavid

This comment has been minimized.

Copy link
Member Author

commented Aug 21, 2018

Additionally, I must say that I do not want to merge this PR, because in my opinion it is better to finish #799 first and do the changes in a new branch from master after merge it.

errors.none? && reset && refresh!
result = errors.none?

result && reset && refresh

This comment has been minimized.

Copy link
@jreidinger

jreidinger Aug 21, 2018

Member

I prefer more explicit if result and not hide it inside boolean expression.

This comment has been minimized.

Copy link
@dgdavid

dgdavid Aug 21, 2018

Author Member

I agree

rescue Yast::SystemctlError
register_error(:active)
rescue => e
register_error(:active) if e.kind_of?(Yast::SystemctlError)

This comment has been minimized.

Copy link
@jreidinger

jreidinger Aug 21, 2018

Member

Hmm, I do not like it. It basically catch all errors instead of systemctl ones, is it correct?

This comment has been minimized.

Copy link
@dgdavid

dgdavid Aug 21, 2018

Author Member

It seem to me, according to the expected result when #perform_action is called. If I am wrong, the other approach could be

def perform_action
...
rescue Yast::CouldNotRefreshUnitError
...
rescue Yast::SystemctlError
...
end

This comment has been minimized.

Copy link
@joseivanlopez

joseivanlopez Aug 21, 2018

Contributor

I expect that #perform_action only catches refresh exception. The idea is to not fail if the action was performed but refresh fails, so I would do:

rescue Yast::SystemctlRefreshError
  true

This comment has been minimized.

Copy link
@jreidinger

jreidinger Aug 21, 2018

Member

better or at least use rescue Yast::SystemctlError, Yast::CouldNotRefreshUnitError => e so it don't catch other problems and silent them.

This comment has been minimized.

Copy link
@joseivanlopez

joseivanlopez Aug 21, 2018

Contributor

In other words, #perform_action should continue raising the SystemcltError when the action is not performed.

This comment has been minimized.

Copy link
@dgdavid

dgdavid Aug 21, 2018

Author Member

Okey, I moved register_error to #save method.

@@ -3,6 +3,14 @@
require "ostruct"

module Yast
# Represent a fail when systemctl command tries to refresh the service
class CouldNotRefreshUnitError < StandardError

This comment has been minimized.

Copy link
@jreidinger

jreidinger Aug 21, 2018

Member

just thinking about it, why not having some reasonable ancestor for all that systemd errors?

This comment has been minimized.

Copy link
@jreidinger

jreidinger Aug 21, 2018

Member

or service errors?

This comment has been minimized.

Copy link
@dgdavid

dgdavid Aug 21, 2018

Author Member

Make sense to me, as long as we decide to cover more scenarios adding more specific exceptions.

This comment has been minimized.

Copy link
@joseivanlopez

joseivanlopez Aug 21, 2018

Contributor

Yes, I agree with @jreidinger. I would create a new exception based on existing SystemctlError, for example:

class SystemctlRefreshError < SystemctlError; end

Using a subclass we ensure that all code catching current SystemctlError will continue catching the new exception SystemctlRefreshError.

def refresh!
@properties = show
@error = properties.error
properties
rescue Yast::SystemctlError
raise Yast::CouldNotRefreshUnitError, self

This comment has been minimized.

Copy link
@jreidinger

jreidinger Aug 21, 2018

Member

shouldn't it set also @error when show failed? Otherwise I worry we will be in an inconsistent state.

This comment has been minimized.

Copy link
@dgdavid

dgdavid Aug 21, 2018

Author Member

Good catch.

I am not sure about what to do.

@joseivanlopez, thoughts?

This comment has been minimized.

Copy link
@joseivanlopez

joseivanlopez Aug 21, 2018

Contributor

Something like this? @error = "Fails to refresh unit" ?

This comment has been minimized.

Copy link
@dgdavid

dgdavid Aug 21, 2018

Author Member

Okey, thank you

@jreidinger

This comment has been minimized.

Copy link
Member

commented Aug 21, 2018

@dgdavid fine for me to having it after merge as it can need long discussion about how exception structure for system service should look like.

@dgdavid dgdavid force-pushed the feature/improve-system-service-raised-errors branch from ccf7171 to 2ae740e Aug 21, 2018

register_error(:active)
false
rescue Yast::CouldNotRefreshUnitError
true

This comment has been minimized.

Copy link
@jreidinger

jreidinger Aug 21, 2018

Member

and what happens when result is false so action failed and also following refresh failed?

This comment has been minimized.

Copy link
@dgdavid

dgdavid Aug 21, 2018

Author Member

Ouch! Nice catch. Let me think about it.

@dgdavid

This comment has been minimized.

Copy link
Member Author

commented Aug 21, 2018

After discuss this a little more deep offline, we concluded that for now it is not really necessary to distinguish that exception because, for the shake of consistency, the #perform_action method must be return false as soon as something went wrong.

So, the only change that we will to do, in a new PR, is to rescue the SystemctlError also in the #save_start_mode.

@dgdavid dgdavid closed this Aug 21, 2018

@imobachgs imobachgs deleted the feature/improve-system-service-raised-errors branch Sep 14, 2018

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